cosmo: to/from table

[nstarman]

Registers Astropy Table into Cosmology's to/from_format I/O, allowing
a Cosmology instance to be parsed from or converted to a Table instance.
Also adds the __astropy_table__ method allowing Table(cosmology).

Requires #12209 Merged!

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• 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 "When to rebase and squash commits".
• 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.
• 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 a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
• 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.

[pep8speaks]

Hello @nstarman 👋! 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 12:35:13 UTC

[taldcroft]

@nstarman - is this ready? I see it is still showing as a Draft.

[nstarman]

It's ready for review, but it's dependent on another PR (see top comment), which needs to be merged first.
I think the other PR is very uncontroversial. It's mostly refactoring some of my test suite.

[taldcroft]

One big picture comment at this point. I went to the current RTD for this PR and was see that neither JSON nor ECSV formats are registered by default. I thought that was the main driver of this whole long journey, to provide an interoperable serialization format for cosmology.

The code for JSON is already in the example, and there used to be code for ECSV (and it's trivial anyway), so is that planned for the next quick PR before 5.0?

Other comments on the docs:

• If you do get JSON and ECSV as built-ins, then I would put the pickle serialization at the end. I.e. don't lead off with the undesirable solution since half the people will stop reading there and use pickle.
• Building on that, the first thing in Getting Started are examples that don't actually work, which is unsatisifying.
• The first example with Cosmology.read.list_formats() is also not what the user will see when they try this. How does the user actually get to seeing the mypackage format? One possibility is to bundle mypackage into the tests, so then the user has access via astropy.cosmology.tests.mypackage.
• Sphinx typo resulting in Now this :class:QTable can be used

[taldcroft]

I'll take another pass, but for now I didn't see anything obviously off in the code and the testing looks quite solid.

[taldcroft]

I just saw the Cosmology, the Expansion project. Wow, amazing work and great to see your ambitious plans laid out like this. (And clever project title). But I don't see ECSV in there except as a closed WIP from early on.

[nstarman]

One big picture comment at this point. I went to the current RTD for this PR and was see that neither JSON nor ECSV formats are registered by default. I thought that was the main driver of this whole long journey, to provide an interoperable serialization format for cosmology.

Good spot. Yes, that is still the plan. In the (now-closed) omnibus PR #11559 one of the important topics of discussion was https://xkcd.com/927/ on competing standards. I spent a while looking over different formats, e.g. Cobaya versus Cosmosis, versus MontePython and they are all a bit confusing and muddled and depend entirely on their internal units.
So I think the takeaway is that there is definitely a need for ECSV with it's lossless encoding, but perhaps/probably not JSON.

The code for JSON is already in the example, and there used to be code for ECSV (and it's trivial anyway), so is that planned for the next quick PR before 5.0?

Yes for the ECSV. Not yet for the JSON, for the above reasons.

• If you do get JSON and ECSV as built-ins, then I would put the pickle serialization at the end. I.e. don't lead off with the undesirable solution since half the people will stop reading there and use pickle.

I actually think pickle is a good way to serialize a cosmology for short-term storage. ie passing an object between threads. I think I'll just note that there are superior solutions for persistent storage.

• Building on that, the first thing in Getting Started are examples that don't actually work, which is unsatisifying.

Good point! now operational. not tested in the docs, but thoroughly elsewhere.

• The first example with Cosmology.read.list_formats() is also not what the user will see when they try this. How does the user actually get to seeing the mypackage format? One possibility is to bundle mypackage into the tests, so then the user has access via astropy.cosmology.tests.mypackage.

A good point. I'll try moving it.

Edit: See #12321 for fixes to the above.

[taldcroft]

@nstarman - your replies above all look good to me. I see a merge conflict just popped up...

[mhvk]

This looks great! My comments can be safely ignored...

[mhvk]

In principle, one could index by other things than str - maybe reverse this? E.g.,

if index is not None:
    try:
        index = operator.index(index)
    except TypeError:
        if table.indices:
            index = table.loc_indices[index]
        else:
            table = table[table['name'] == index]
            index = None

(Note different suggestion for finding the name - for duplicates, I think one should fail)

[mhvk]

Actually, on second thought all of this seems too much duplication of table indexing. Not sure any is really necessary, but if doing it I'd keep it really simple and just allow the selection by name and perhaps a simple numerical index (though I'd personally sooner allow input of Row objects and let people do table[num])

[nstarman]

I have a Row reader PR scheduled for v5.1! though if this PR goes in soon maybe we can get that one in as well. It's ready, I just have too many PRs for v5.0 😰

[nstarman]

#12313

[mhvk]

Overall, yes, but do remember these tests get run many times, so energy use may not be quite negligible (though incomparably less than bitcoin for incomparably more value!)

[nstarman]

Yeah, I realized that boilerplate comment is also wrong. I use the private functions across the cosmology/io subpackage and this is also one of the few places the built in Cosmology realizations are specifically tested. Actually quite useful!

[taldcroft]

OK, with #12321 available as a follow-on, this looks good to me. The comments from @mhvk were marked as optional, so I'm approving this now and recommend merging.

[taldcroft]

NB the Python 3.10 failure looks unrelated.

[nstarman]

Rebased on fixes to most of the CI just to check everything's kosher.
Edit: everything expected passed given #12324 is still a problem.

Thanks @taldcroft and @mhvk for the reviews!