Cosmo: to/from yaml by nstarman · Pull Request #12279 · astropy/astropy

nstarman · 2021-10-22T18:22:18Z

Cosmology parse from and convert to yaml string.

requires: #12209

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.

github-actions · 2021-10-22T18:23:09Z

👋 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?

mhvk

This looks good, so approving.

Just in case, though: Are you sure you want to automatically register every user-defined cosmology subclass? For ESCV, I thought the idea was to be rather conservative and only accept known-safe classes. But if this is mostly for ASDF, then perhaps it is different. Let me ping @taldcroft in case.

taldcroft · 2021-11-02T21:46:13Z

Just in case, though: Are you sure you want to automatically register every user-defined cosmology subclass?

The goal is that the astropy YAML load is safe and only accepts known built-in astropy classes. Looking back at this (it's been a while), I'm trying to remember the security model for subclasses in built-in cases that use add_multi_representer.

nstarman · 2021-11-02T21:48:06Z

the astropy YAML load is safe and only accepts known built-in astropy classes.

Is the loader not safe to arbitrary code execution and other shenanigans?
Why limit to only built-ins?

I can revert to manually registering classes.
I'll just add some documentation to the I/O that custom cosmology classes need manual registration.

taldcroft · 2021-11-02T22:49:42Z

This requires getting someone to import your package and then read in a YAML file you've sent, but it seems somewhat credible:

In [31]: class EvilPlanck18(FlatLambdaCDM):
    ...:     def __new__(cls,  *args, **kwargs):
    ...:         print('HACK!')
    ...:         return super().__new__(cls, *args, **kwargs)
    ...: 

In [34]: x = EvilPlanck18(10, 10)
HACK!

In [35]: t = dump(x)

In [36]: t
Out[36]: '!__main__.EvilPlanck18\nH0: 10\nNeff: 3.04\nOb0: null\nOm0: 10\nTcmb0: !astropy.units.Quantity\n  unit: !astropy.units.Unit {unit: K}\n  value: 0.0\nm_nu: !astropy.units.Quantity\n  unit: !astropy.units.Unit {unit: eV}\n  value: 0.0\nmeta: !!python/tuple []\nname: null\n'

In [37]: load(t)
HACK!

taldcroft · 2021-11-02T22:50:57Z

~~And I don't remember exactly why, but~~ [1] as mentioned Quantity is not like that:

In [19]: class EvilQuantity(u.Quantity):
    ...:     def __new__(cls, *args, **kwargs):
    ...:         print('EVIL!')
    ...:         return super().__new__(cls, *args, **kwargs)
    ...: 
    ...: 

In [20]: EvilQuantity(1.0)
EVIL!
Out[20]: <EvilQuantity 1.>

In [21]: x = EvilQuantity(1.0)
EVIL!

In [22]: t = dump(x)

In [23]: t
Out[23]: "!astropy.units.Quantity\nunit: !astropy.units.Unit {unit: ''}\nvalue: 1.0\n"

In [24]: load(t)
Out[24]: <Quantity 1.>

[1] - checked the code and now I see how that happens. The representer always uses a fixed tag '!astropy.units.Quantity' regardless of the subclass.

nstarman · 2021-11-02T22:54:19Z

Alright. Time to get rid of auto-registration! Thanks @mhvk and @taldcroft

nstarman · 2021-11-03T01:34:38Z

Auto-registration is gone, leaving the question whether to publicly scope the function register_cosmology_yaml.
There is currently no means for a custom cosmology to be registered and I/O with YAML.

taldcroft · 2021-11-03T10:27:52Z

+    def setup_class(self):
+        self.functions = {"to": to_yaml, "from": from_yaml}
+
+    @pytest.fixture(scope="class", autouse=True)
+    def setup(self):
+        """Setup and teardown for tests."""
+        yield  # run tests


@nstarman - I see that you and @mhvk are big fans of layers of abstraction and layers of fixtures in testing. I will just say that it makes it quite a lot harder for independent reviewers to understand what the tests are actually doing, and so I'm not that enthusiastic about the approach. I've had a hard time understanding the tests from @mhvk and same applies here.

Simple is better than complex. Having the final "test" of a feature boil down to one or two lines may seem elegant, but I cannot understand the test code directly without reading and understanding the upstream base classes and fixtures.

What is different in these tests from simply looping over each of the built-in cosmology classes and doing dump load to_format and from_format tests? This might avoid or simplify the special-case handling of if tuple(cosmo._init_signature.parameters.values())[-1].kind == 4:.

Haha. Guilty as charged. Apologies for that.
This test suite is structured so that

Descriptors on Cosmology, like meta/read/write/from_format/to_format are parents to TestCosmologyso that all tests for the descriptor are run on every Cosmology subclass, like TestFLRW and TestFlatLambdaCDM

For the I/O descriptors, the tests for each format, like "yaml", are parents to the descriptor test so that the format tests are run for each Cosmology subclass.

So the inheritance looks like is TestFLRW(TestCosmology(ToFromFormatTestMixin(ToFromMappingTestMixin , ToFromYAMLTestMixin)))
Where ToFromFormatTestMixin tests the descriptors to/from_format and is fed the tests from each registered format.

What is different in these tests from simply looping over each of the built-in cosmology classes and doing dump load to_format and from_format tests? This might avoid or simplify the special-case handling of if tuple(cosmo._init_signature.parameters.values())[-1].kind == 4:.

Most of these tests are just looping over each of the built-in cosmology classes. But no built-in cosmology has a kwargs in __init__ (that's the tuple(cosmo._init_signature.parameters.values())[-1].kind == 4).
I test this case because a fair number of things were broken in non-intuitive ways, like clone and I/O with metadata, especially when there's partial information allowed like a cosmology <-> dict. I added the kwargs tests a number of PRs ago and fixed things accordingly. Having kwargs necessitates a slightly different treatment of metadata, so it needs to be special cased.
Except I don't think this is the case for yaml unless someone is editing the YAML strings and adding extra args/kwargs! I have a TODO! on the partial information test because I think this quite unlikely. If someone ever hand-assembles a valid Cosmology yaml string, then I'll write the test!
To end a long explanation, tuple(cosmo._init_signature.parameters.values())[-1].kind == 4 is not necessary for this test. I'm removing the check.

taldcroft · 2021-11-03T10:35:42Z

+        # tests are different if the last argument is a **kwarg
+        if tuple(cosmo._init_signature.parameters.values())[-1].kind == 4:
+            got = from_format(yml, format="yaml")


What's going on here? Needs a better comment in any case.

See above. This isn't necessary for this test, but in general metadata is handled differently if the cosmology has kwargs.

astropy/astropy/cosmology/io/mapping.py

Lines 104 to 114 in 73c57e1

# deal with remaining params. If there is a **kwargs use that, else

# allow to transfer to metadata. Raise TypeError if can't.

lastp = tuple(cosmology._init_signature.parameters.values())[-1]

if lastp.kind == 4: # variable keyword-only

ba.arguments[lastp.name] = params

elif move_to_meta: # prefers current meta, which was explicitly set

meta = ba.arguments["meta"] or {} # (None -> dict)

ba.arguments["meta"] = {**params, **meta}

elif params:

raise TypeError(f"there are unused parameters {params}.")

# else: pass # no kwargs, no move-to-meta, and all the params are used

is the reason. Many of the formats route through mapping.

But not YAML in the same way because it is currently assumed that no one is messing with the YAML string, adding arguments.

I plan on "public" scoping the test suite hopefully for v5.1 release. I've structured the tests so that it should be very easy for 3rd party packages and contributors. Tests for new I/O are a short mixin. Tests for new Cosmologies have a simple inheritance from the parent class tests.
But I definitely need to increase the density of in-line comments and docstrings and explanations!

@nstarman

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

pep8speaks · 2021-11-03T15:53:11Z

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-11-03 21:16:28 UTC

@nstarman

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

nstarman added this to the v5.1 milestone Oct 22, 2021

github-actions Bot added the cosmology label Oct 22, 2021

nstarman force-pushed the cosmo_tofrom_yaml branch 4 times, most recently from aea2951 to 924623f Compare October 22, 2021 19:18

nstarman added the whatsnew-needed label Oct 22, 2021

nstarman force-pushed the cosmo_tofrom_yaml branch from 924623f to 01baa15 Compare October 23, 2021 04:01

nstarman force-pushed the cosmo_tofrom_yaml branch 4 times, most recently from f27616f to 07fbc6c Compare November 2, 2021 02:25

nstarman marked this pull request as ready for review November 2, 2021 15:55

nstarman requested a review from mhvk November 2, 2021 15:55

nstarman commented Nov 2, 2021

View reviewed changes

Comment thread astropy/cosmology/core.py Resolved

nstarman requested a review from eslavich November 2, 2021 16:04

mhvk approved these changes Nov 2, 2021

View reviewed changes

nstarman force-pushed the cosmo_tofrom_yaml branch from 07fbc6c to 6ac43ca Compare November 3, 2021 01:32

nstarman force-pushed the cosmo_tofrom_yaml branch from 6ac43ca to 0336c95 Compare November 3, 2021 01:41

taldcroft reviewed Nov 3, 2021

View reviewed changes

Register yaml into Cosmology to/from I/O

51943f0

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

nstarman mentioned this pull request Nov 3, 2021

TST: linkcheck failed with node class 'meta' is already registered #12382

Closed

nstarman force-pushed the cosmo_tofrom_yaml branch from 0336c95 to 89828fd Compare November 3, 2021 15:53

nstarman force-pushed the cosmo_tofrom_yaml branch 2 times, most recently from d12ead7 to c0dc4c1 Compare November 3, 2021 18:28

Address comments from review

17afb28

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

nstarman force-pushed the cosmo_tofrom_yaml branch from c0dc4c1 to 17afb28 Compare November 3, 2021 21:16

nstarman merged commit c8e8a65 into astropy:main Nov 3, 2021

nstarman deleted the cosmo_tofrom_yaml branch November 3, 2021 21:47

nstarman mentioned this pull request Nov 3, 2021

fix bug where redshift not recognized as unit #12390

Merged

9 tasks done

	# deal with remaining params. If there is a **kwargs use that, else
	# allow to transfer to metadata. Raise TypeError if can't.
	lastp = tuple(cosmology._init_signature.parameters.values())[-1]
	if lastp.kind == 4: # variable keyword-only
	ba.arguments[lastp.name] = params
	elif move_to_meta: # prefers current meta, which was explicitly set
	meta = ba.arguments["meta"] or {} # (None -> dict)
	ba.arguments["meta"] = {params, meta}
	elif params:
	raise TypeError(f"there are unused parameters {params}.")
	# else: pass # no kwargs, no move-to-meta, and all the params are used

Conversation

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

Uh oh!

Checklist for package maintainer(s)

Uh oh!

github-actions Bot commented Oct 22, 2021

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Nov 2, 2021

Uh oh!

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

Uh oh!

Uh oh!

taldcroft commented Nov 2, 2021

Uh oh!

taldcroft commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Nov 2, 2021

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Comment last updated at 2021-11-03 21:16:28 UTC

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nstarman commented Oct 22, 2021 •

edited

Loading

nstarman commented Nov 2, 2021 •

edited

Loading

taldcroft commented Nov 2, 2021 •

edited

Loading

nstarman commented Nov 3, 2021 •

edited

Loading

nstarman Nov 3, 2021 •

edited

Loading

pep8speaks commented Nov 3, 2021 •

edited

Loading