cosmo: to/from table by nstarman · Pull Request #12213 · astropy/astropy

nstarman · 2021-09-29T04:00:53Z

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 · 2021-09-29T04:01:00Z

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 · 2021-10-26T16:05:25Z

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

nstarman · 2021-10-26T16:06:36Z

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.

nstarman · 2021-10-26T16:24:14Z

Rebased now that #12209 is merged.

nstarman · 2021-10-27T16:50:36Z

@taldcroft. Rebased now that registries are separated.
Edit: sigh. Just trying to fix a RTD error. Got/getting a ref link wrong.
Edit: retriggering because RTD timed out.

taldcroft · 2021-10-28T11:19:56Z

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 · 2021-10-28T11:30:50Z

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 · 2021-10-28T15:12:26Z

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 · 2021-10-28T17:37:42Z

@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 · 2021-10-29T01:30:37Z

+        if not table.indices:  # no indexing column, find by string match
+            index = np.where(table['name'] == index)[0][0]
+        else:  # has indexing column
+            index = table.loc_indices[index]  # need to convert to row index (int)


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)

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])

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 😰

mhvk · 2021-10-29T01:37:12Z

+    These are not public API and are discouraged from use, in favor of
+    ``Cosmology.to/from_format(..., format="<format>")``, but should be tested
+    regardless b/c 3rd party packages might use these in their Cosmology I/O.
+    Also, it's cheap to test.


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!)

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 · 2021-10-29T09:55:15Z

NB the Python 3.10 failure looks unrelated.

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

@nstarman

Allows for ``Table(cosmo)``. Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

nstarman · 2021-10-29T12:35:34Z

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!

nstarman added the Enhancement label Sep 29, 2021

nstarman added this to the v5.0 milestone Sep 29, 2021

nstarman changed the title ~~Cosmo tofrom table~~ cosmo: to/from table Sep 29, 2021

github-actions Bot added cosmology Docs labels Sep 29, 2021

nstarman force-pushed the cosmo_tofrom_table branch 4 times, most recently from 7441ce3 to dc65c18 Compare September 29, 2021 04:10

nstarman marked this pull request as draft September 29, 2021 05:43

nstarman mentioned this pull request Sep 29, 2021

cosmo: improve mapping io and tests #12209

Merged

9 tasks done

nstarman added the whatsnew-needed label Sep 30, 2021

nstarman force-pushed the cosmo_tofrom_table branch 4 times, most recently from b67343c to f8818c5 Compare October 14, 2021 02:32

nstarman force-pushed the cosmo_tofrom_table branch from f8818c5 to 4fddf3e Compare October 19, 2021 17:14

nstarman requested review from adrn and taldcroft October 19, 2021 18:52

nstarman mentioned this pull request Oct 21, 2021

Cosmology to/from Model #12269

Merged

9 tasks done

nstarman force-pushed the cosmo_tofrom_table branch 5 times, most recently from 99f08df to 2bc0700 Compare October 23, 2021 20:46

nstarman force-pushed the cosmo_tofrom_table branch from 2bc0700 to c4b3e68 Compare October 26, 2021 16:23

nstarman force-pushed the cosmo_tofrom_table branch 3 times, most recently from d4f1b77 to 8ac2902 Compare October 27, 2021 20:18

taldcroft reviewed Oct 28, 2021

View reviewed changes

Comment thread docs/whatsnew/5.0.rst Resolved

Comment thread docs/whatsnew/5.0.rst Resolved

nstarman force-pushed the cosmo_tofrom_table branch from 8ac2902 to c1cd0c1 Compare October 28, 2021 13:29

nstarman force-pushed the cosmo_tofrom_table branch from 77103db to c1cd0c1 Compare October 28, 2021 17:23

nstarman mentioned this pull request Oct 28, 2021

Cosmo read/write ECSV #12321

Merged

9 tasks done

nstarman force-pushed the cosmo_tofrom_table branch 2 times, most recently from a0f583a to bc3cafb Compare October 28, 2021 18:02

nstarman requested a review from taldcroft October 28, 2021 18:26

mhvk reviewed Oct 29, 2021

View reviewed changes

nstarman force-pushed the cosmo_tofrom_table branch 2 times, most recently from dcac4cb to 2404228 Compare October 29, 2021 03:51

nstarman requested a review from mhvk October 29, 2021 05:29

taldcroft approved these changes Oct 29, 2021

View reviewed changes

nstarman added 4 commits October 29, 2021 08:34

Make base class for IO tests

7944d20

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

Register in “table” for Cosmology <-> QTable

7c3c11b

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

Add method __astropy_table__

5cf7d5e

Allows for ``Table(cosmo)``. Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

better table indexing errors

3e8ba26

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

nstarman force-pushed the cosmo_tofrom_table branch from 2404228 to 3e8ba26 Compare October 29, 2021 12:35

nstarman added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Oct 29, 2021

nstarman merged commit 9f8ecd5 into astropy:main Oct 29, 2021

nstarman deleted the cosmo_tofrom_table branch October 29, 2021 12:50

Conversation

nstarman commented Sep 29, 2021 • edited by taldcroft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for package maintainer(s)

Uh oh!

pep8speaks commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-10-29 12:35:13 UTC

Uh oh!

taldcroft commented Oct 26, 2021

Uh oh!

nstarman commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Oct 26, 2021

Uh oh!

nstarman commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taldcroft commented Oct 28, 2021

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

taldcroft commented Oct 28, 2021

Uh oh!

nstarman commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taldcroft commented Oct 28, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Oct 29, 2021

Uh oh!

nstarman commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nstarman commented Sep 29, 2021 •

edited by taldcroft

Loading

pep8speaks commented Sep 29, 2021 •

edited

Loading

nstarman commented Oct 26, 2021 •

edited

Loading

nstarman commented Oct 27, 2021 •

edited

Loading

nstarman commented Oct 28, 2021 •

edited

Loading

nstarman commented Oct 29, 2021 •

edited

Loading