Leave maxmags column in ACA Review Table

[taldcroft]

Description

For some forgotten reason, during initial development I decided to cut out the maxmags column from the ACAReviewTable. This is an important part of the catalog for any comprehensive review (razl), so this PR puts that column back in the table. More precisely it removes the code that deletes that column from the proseco table.

Interface impacts

The ACA review table will have an extra column. Any tests that depend on the exact repr of such a table may need to be updated but the sparkles tests themselves did not need modification.

In theory this might impact MATLAB tools. One option is to leave the maxmags column in ACACheckTable and take it back out in ACAReviewTable. In fact that was my first implementation, but if it doesn't actually break anything I'd rather go with the current version.

Testing

Unit tests

• Mac

(ska3) ➜  sparkles git:(dont-remove-maxmags-column) git rev-parse HEAD                                           
e1a9d66e178e8b74cbd60b8486033c854d8dd193
(ska3) ➜  sparkles git:(dont-remove-maxmags-column) pytest
====================================================== test session starts ======================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 103 items                                                                                                             

sparkles/tests/test_checks.py ............................................................................                [ 73%]
sparkles/tests/test_find_er_catalog.py .....                                                                              [ 78%]
sparkles/tests/test_review.py ..................                                                                          [ 96%]
sparkles/tests/test_yoshi.py ....                                                                                         [100%]

===================================================== 103 passed in 33.82s ======================================================

Independent check of unit tests by [REVIEWER NAME]

• [PLATFORM]:

Functional tests

FEB0724A ouput.

[jeanconn]

Regarding the history here, I think maxmag was just pulled from the review table because we were focused on the sparkles display.

#23 (comment)

#26

[jskrist]

I have confirmed that the MATLAB tools are not using this key, and does not have any issues generating star catalogs or roll suggestions with this PR.

[jeanconn]

I'm not concerned about updating the tests but figure a new new column in the sparkles html report might count as a separate interface change (even if that html is just from the ACAReviewTable). Actually do we need that column in the report or only in the Table? My first thought was that you'd remove it from the representation at

sparkles/sparkles/core.py

Line 913 in 8cc0145

catalog = "\n".join(self.pformat(max_width=-1, max_lines=-1))

but if you want it in there for part of review that's fine.

[taldcroft]

So I guess we have to be sure the sparkles report looks OK.

[jeanconn]

OK, I copied the output I'd looked at to https://icxc.cfa.harvard.edu/aspect/test_review_outputs/sparkles-pr204/FEB0724A_sparkles/ and updated the functional testing with it.