Split io.ascii CDS/MRT into distinct CDS and MRT classes - #12301

Hmm, am I missing something in my test setup? Getting in the local run now

        with pytest.raises(UserWarning):
            t.write(out, format='ascii.mrt')
            lines = out.getvalue().splitlines()
            i_bbb = lines.index('=' * 80)
            lines = lines[i_bbb:]  # Select Byte-By-Byte section and later lines.
            # Check the written table.
>           assert lines == exp_output.splitlines()
E           AssertionError: assert ['===========...me     ', ...] == ['===========...of name', ...]
E             At index 5 diff: '  1-  7  A7     ---    name    Description of name     ' != ' 1- 7  A7     ---    name    Description of name'
E             Left contains 6 more items, first extra item: '104-107  F4.1   arcmin DEm     Declination (arcmin)    '
E             Full diff:
E               [
E                '================================================================================',
E                'Byte-by-byte Description of file: table.dat',
E                '--------------------------------------------------------------------------------',...
E             
E             ...Full output truncated (51 lines hidden), use '-vv' to show

lib/python3.9/site-packages/astropy/io/ascii/tests/test_cds.py:476: AssertionError

The full output from the writer looks like

Title:
Authors:
Table:
================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
  1-  7  A7     ---    name    Description of name     
  9- 24  A16    ---    Unknown Description of Unknown  
 26- 28  F3.1   min    RAm     Right Ascension (minute)
 30- 44  F15.12 s      RAs     Right Ascension (second)
     46  A1     ---    DE-     Sign of Declination     
 47- 50  F5.1   deg    DEd     Declination (degree)    
 52- 55  F4.1   arcmin DEm     Declination (arcmin)    
 57- 71  F15.12 arcsec DEs     Declination (arcsec)    
 73- 76  F4.1   h      RAh     Right Ascension (hour)  
 78- 80  F3.1   min    RAm     Right Ascension (minute)
 82- 96  F15.12 s      RAs     Right Ascension (second)
     98  A1     ---    DE-     Sign of Declination     
 99-102  F5.1   deg    DEd     Declination (degree)    
104-107  F4.1   arcmin DEm     Declination (arcmin)    
109-123  F15.12 arcsec DEs     Declination (arcsec)    
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
HD81809 330.564 -61.6596 2.0 15.450000000007 -61.0 39.0 34.599996000001 22.0 2.0 15.450000000007 -61.0 39.0 34.599996000001

so has indeed the duplicate columns.

@dhomeier I thought this was an upgrade of the CDS/MRT writer (splitting the two formats) as we had talked about in #11897 (and #12130) independent of the Azure pipeline failure. Does this also integrate a fix for it?

I have a feeling that we may want @dhomeier 's #12302 first and then pull that over into this one?

Coincidentally, @aaryapatil and I had a discussion with @Suyog7130 not too long ago and we talked about how to continue with his remaining PRs. We decided to wait until after the 5.0 release to not run into conflicts with other urgent PRs. I'm just saying this to make clear that we did not abandon the the project; but of course we welcome @taldcroft doing those changes here!

Does this also integrate a fix for it?

No, you are completely right, I had not been following the development in #11897 very closely and just saw this falling to earth in the midst of all the discussion on the azure failures. I think @pllim has given a good explanation why the test does not fail on CI.

See #12302 (comment) for my argument that this PR should go in first. I think doing it the opposite way round will result in a nightmare rebase. My first step to create mrt.py was a copy and then git add, when perhaps if I had done a git mv it would have been possible / easy to rebase (assuming git is clever enough to track the file name change, but I'm not sure on that).

We decided to wait until after the 5.0 release to not run into conflicts with other urgent PRs.

I think this is an urgent PR for 5.0 because otherwise after that doing the splitting represents an API change and will break existing code that was using cds to write.

Yes, I agree both that merging them in numerical order ;-) should be easier, and the API as defined here would better be finalised for 5.0.
#12302 is fairly well contained within the write and related functions, and most of the volume is just the change in expected output.
This looks a bit longer to review, but as @taldcroft pointed out, a lot of that is also just code moved between files. :)

The only irreconcilable bit is likely to be the changes I made in the test expected output where I turned them all into lists of strings. They were passing before because of trailing whitespace in the strings, and my editor just chopped all those immediately. I figured other people might have that (whitespace autotrim) enabled and have the same problem.

Yck, not sure of the easiest way forward but open to any suggestions.

Yes, I ran into a similar thing when style-check would accept them in the tests (with the #noqa W291 at least) but not in the rst docs.
Might just copy the expected blocks from #12302 (which have the different coordinate format anyway) back over them again...

Might just copy the expected blocks from #12302 ..

That will work and hopefully be not too painful. My process was to temporarily put from pprint import pprint in the test module, then run pytest with --pdb. For each failure do pprint(lines) (or whatever) and copy that into the test script, and then reformat.

In any case I'm going to do the tests and changelog right now.

FutureWarning: Parquet format '2.0' pseudo version is deprecated, use '2.4' or '2.6' for fine-grained feature selection is unrelated and being addressed in a different PR. FYI.

Actually looks like #12300 is merged, so rebasing should fix that unrelated failure.

Hello @taldcroft 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-10-29 16:58:27 UTC

OK, rebased and added test coverage for MRT reading and a news fragment. This fragment is an exact copy of that for #11897 which should make them into one in the final changelog.

To help slightly with the review, here is the diff of the current cds.py in this PR and the previous release version from 4.3.1. Basically no functional changes except the hooks to allow MRT or CDS subformats. There is one substantive change with the addition of a ValueError if no byte-by-bytes section is found.

(astropy) ➜  astropy git:(io-ascii-mrt) git show v4.3.1:astropy/io/ascii/cds.py > cds.py-v4.3.1     
(astropy) ➜  astropy git:(io-ascii-mrt) opendiff cds.py-v4.3.1 astropy/io/ascii/cds.py
(astropy) ➜  astropy git:(io-ascii-mrt) diff cds.py-v4.3.1 astropy/io/ascii/cds.py 
27a28,29
>     _subfmt = 'CDS'
> 
39,40c41,42
<             raise ValueError('Unrecognized CDS format "{}" for column "{}"'.format(
<                 col.raw_type, col.name))
---
>             raise ValueError('Unrecognized {} format "{}" for column "{}"'.format(
>                 self._subfmt, col.raw_type, col.name))
45c47
<         Initialize the header Column objects from the table ``lines`` for a CDS
---
>         Initialize the header Column objects from the table ``lines`` for a CDS/MRT
98a101,102
>         else:
>             raise ValueError('no line with "Byte-by-byte Description" found')
121c125
<                     col.unit = None  # "---" is the marker for no unit in CDS table
---
>                     col.unit = None  # "---" is the marker for no unit in CDS/MRT table
149c153
<                         # CDS tables can use -, --, ---, or ---- to mark missing values
---
>                         # CDS/MRT tables can use -, --, ---, or ---- to mark missing values
173a178
>     _subfmt = 'CDS'
177c182
<         """Skip over CDS header by finding the last section delimiter"""
---
>         """Skip over CDS/MRT header by finding the last section delimiter"""
187c192
<             raise core.InconsistentTableError('No CDS section delimiter found')
---
>             raise core.InconsistentTableError(f'No {self._subfmt} section delimiter found')
309c314
<         """Not available for the Cds class (raises NotImplementedError)"""
---
>         """Not available for the CDS class (raises NotImplementedError)"""

🤞 on tests, but I think this is ready for review @dhomeier @aaryapatil @hamogu .

Merging my cds.py into your mrt.py went as smoothly as it comes, so no concerns from that side!
I am busy with another task for today, but will see if I can add a proper review later in the evening.

Ping @dhomeier - I think @hamogu was looking for a final review from you on this PR. Will you be able to get to it today?

This was passing all tests except Azure in fffac0b.

Sorry, somewhat too many releases and feature freezes for one day!