Handle Metadata in io.ascii MRT Tables - #17279

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

I just finished a first attempt at writing the metadata.

• The current implementation does not keep track of how the lines are split in the input file.
• When writing, the MAX_SIZE_README_LINE is used to wrap text.
• All continuation lines are indented with 4 spaces for now, but that would be easy to change.
• The author list is split only between authors, I'm using chained str.replace to replace spaces with numbers to avoid wrapping within authors. It feels a bit like a hack, but it works for now.

I still need to add proper tests. Once that is done, I'll mark this as ready for review!

Thanks a lot for working on this @vandalt !
I'm requesting a review from myself as I expect to be working rather intensely on MRT support in astropy in the coming months so it feels like I should give it a deep look at some point, if only to better familiarize myself.
However I'm not an expert yet on this region of the code base, so I don't think I can answer all your questions at the moment, but I do have one for you: do we already have a reason to go for OrderedDict ? I'm aware there are limitations with the builtin dict (which is why #10888 is still opened), but do we know that dict isn't sufficient her ?

Thanks for the feedback @neutrinoceros !

The only reason I went for OrderedDict is that the docs for Table mention that meta is an OrderedDict by default, so I went for that. Also, I had missed that dict is now officially ordered by default in Python 3.7+. I'll try switching to dict, everything should work.

Oh, if that's how we document it, we should keep an OrderedDict here. Sorry about my misleading question !

I am encountering issues due to the various ways metadata other than byte-by-byte description can be specified in the header, namely article title, author names, and table titles

• The CDS standard has a given way to describe the metadata, as in cdsFunctional.dat
• The MRT standard is not as explicit in its description, but the example at the bottom of this page and the paragraph just above seems to imply that we expect the Title:, Authors: and Table: fields with the format shown. This is what we have in example file cds.dat, for example.

My current implementation supports only the latter case, as I was focusing on MRT tables. However, this means that now there are some tests failing when trying to read metadata from cdsFunction.dat with the MRT reader, for example.

What would be the best path forward:

1. Only support tables with the MRT-formatted metadata (as in cds.dat) with the MRT writer and update the tests and documentation to reflect this, then the CDS-formatted metadata can be handled in the CDS reader.
2. Support both the MRT-formatted and CDS-formatted metadata with the MRT reader, and update the MRT code I implemented to reflect this.
3. Other suggestions?

I feel like option 1 above separates the two formats nicely, but I might be missing some context about what is expected from the two standards. @taldcroft your suggestions would be welcome, since I think it was you who initially separated MRT and CDS in two classes.

Thanks!

It's things like this where @taldcroft and I always felt that the CDS and the MRT format are different. But then we recently talked to the AAS journal data editors and my understanding is that Gus Muench (who works as data editor for the AAS journal and makes the MRT tables they publish - @augustfly ) implied that they are not intended to be different.
I'll point him by email to this conversation and ask for input; but if they really are different, then we need to have two different (and very similar) formats.

I realize I may have mis-communicated here, and maybe we are in a deadlock because of it. So, just in case you guys are waiting for my review, I should explicitly state that I don't normally review PRs marked as draft unless explicitly requested (by someone else). In effect, I'm waiting for either the PR to be undrafted or that someone else asks me to review.
Sorry for this mishap !

Hi! Sorry, the issue is not miscommunication on your side, but lack of time on my end (I'm teaching this semester). I hope I can get back to this around May. However, if someone wants to take over the PR before that, please feel free to do so :)

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

Hi humans 👋 - this pull request hasn't had any new commits for approximately 1 year, 11 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

It's been a while, and maybe @vandalt still doesn't have time for this, but now I do. Thomas, please let me know if you'd like me to pick it up for you. I could just keep your branch alive by resolving merge conflicts or take over if you want.

I indeed don't have time for this right now sorry.

But @neutrinoceros please do take over if you have time for it :) I would love to see this feature implemented.

Thanks!

Thanks !
So I made an attempt are rebasing the PR but the conflicts are gigantic in size so I'm considering this approach might not be tracktable. Considering the final diff is also quite sizeable and the original history is still there, maybe I'd be better off just re-issuing this piecemeal in separate PRs but leaving this one pristine. This will also be a good opportunity for me to walk through the changes and edit them as needed. Whatever the approach this might be fiddly and time consuming. I personally think it is worth a try, but I'd like @hamogu to weigh in before I dive into it.

Rebasing is probably a waste of time because so much has changed since and some of the changes are invasive. By that I mean that they rename files for tests which makes is hard for git to match up any more recent changes in the old files to the changes here.
However, I think that it could save time to copy over some added tests (for example "def test_roundtrip_mrt_meta():" which has a real, but carefully selected table with notes, a long author list broken over several lines etc. [but that's an CDS table, and we don't need to write CDS; right now we're working on MRT. Only useful if this is close enough for the MRT reader/writer to handle this example]). But even in that case, it would be easier to copy and paste the text because cherry-picking commits will suffer from the diverging histories. If that's done @vandalt can always get credit through the "co-authoered-by" markup for that particular commit.

The work could be split in two: Metadata in MRT and metadata in CDS. While they are very similar, the AAS journals (MRT) can be slightly different, so this could be developed for MRT first and applied to CDS later as a separate PR (that should be simple, since astropy only read CDS, while is reads and writes MRT). @vandalt reported problems with the CDS tests in this PR due to the slightly different format. I suggest to punt on that for now and get one format working first. We can contact the AAS journal data editor for clarification of the MRT format if we need to and we'll leave CDS for a separate PR.

I do think reading and writing this metadata is important, and I also hope it's not too complicated if we stick to MRT for now. Better to get one format done, than waiting for the perfect solution to both of them. The low-hanging fruit is to map notes and title/author/etc on the top to long strings in the table.meta.

This PR goes further (e.g. split author names into a list of names rather than one long string) but I'm not sure that has a lot of use cases so it's definitely not high priority and I suggest a later add-on PR.
@vandalt @neutrinoceros Do you agree?

@hamogu Yes, all of this sounds good to me!

I agree that copy-pasting the relevant parts in a new PR is likely the easiest way forward for @neutrinoceros and I'm really not fussed about credit/co-authorship, so whatever works best :)

Thanks!