Cosmo: Parameters, equality, equivalence by nstarman · Pull Request #12136 · astropy/astropy

nstarman · 2021-09-02T03:54:29Z

Cosmology parameters are now descriptors, defaulting to getting the private attribute (e.g. "_name") but allowing for custom getter methods.
Cosmological equality is refactored to check the class, Parameters, the name.
This closes a potential bug where Cosmologies of different classes but had the same parameters could be considered equal.
A new method, is_equivalent, is added to check Parameter equivalence, so a FlatLambdaCDM and flat LambdaCDM are equivalent.
__repr__ is generalized to use Parameter, correcting some bugs. Now only Cosmology has __repr__ defined.
_float_or_None is removed.
~~The new descriptor info is added~~ moved to subsequent PR.

TODO:

update changelog
documentation
update implementation of is_equivalent

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.

mhvk

@nstarman - I like the ideas here very much but have some comments on the implementation (which may help to simplify it).

Also, while it is a different module, we do already have a Parameter class for astropy.models - might it make sense to use a different name?

nstarman · 2021-09-03T22:22:17Z

@mhvk, thanks for the detailed comments!
I'm pushing a partially-complete set of edits. I still need to sort out the is_equivalent methods and write more tests, but I think this addresses most of the above.
Re. the name Parameter and astropy.modeling, I actually like the similarity. I have a followup PR in the works for Cosmology.to_format("astropy.model") -> Model with cosmology's Parameter -> astropy.modeling.Parameter. They mean much the same thing.

pep8speaks · 2021-09-03T22:22:37Z

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-09-22 18:48:49 UTC

mhvk · 2021-09-04T00:00:57Z

On Parameter - indeed, quite similar! I guess for a model one would also need to define what exactly the input and output are, so one would have a set of possible models for each Cosmology. Should be quite neat! May be good to do a quick check that the interface is sufficiently similar that one Parameter can (almost) substitute for another.

nstarman · 2021-09-04T01:15:56Z

one would also need to define what exactly the input and output are, so one would have a set of possible models for each Cosmology.

As I have it currently, one of the mandatory arguments to to_format is the method name on the cosmology — to_format("astropy.model", "<method name>"). By introspecting the signature its easy to tell if the method takes 1 or 2 redshifts. Thankfully all methods return 1 output. The Parameters are to instantiate the cosmology and attached method for .evaluate. It would be nice to be able to fix model parameters... still a WIP.

nstarman · 2021-09-06T05:44:30Z

@mhvk, there are a number of new things since last you reviewed. I think the important ones are .info, the refactored is_equivalent, and Parameter validation. I also enabled descriptor decorator stacking, so m_nu can be lazily evaluated and cached.

Apparently I didn't sort out the strange error in the parallel build. Somehow sys.stdout is getting closed?!?
Also todo: update the changelog. Kind of a lot changed. But it's all interconnected, so I don't really want to break up this PR. I think this will exceed the recommended 5 line length cap.

mhvk

Haven't quite finished reviewing the tests, but am running out of steam, so I'll just put this out. Overall, it looks nice. My main suggestion would be to use Parameter.__set__ for one-off setting (i.e., only works if _attr_name_private does not yet exist), which can then include validation. I think the current Parameter.validate sequence is unusual and likely to be forgotten in user subclasses.

mhvk · 2021-09-06T18:25:55Z

+        if self._parent is None:
+            return super().__repr__()
+        out = StringIO()
+        print(self.__call__(option="parameters", out=out), file=out)


This does the print(..., file=out) twice, once in __call__ and once here!

Ahh. I'll look at that again. I patterned this off of an example in Table, I think. info for Cosmology was sitting in a different branch I was working on for a while... I forget exactly what everything is meant to do.

info isn't necessary for this PR, so I moved it to a subsequent PR.
But yes, thanks! I fixed the print problem.

mhvk · 2021-09-06T19:30:42Z

    def __init__(self, name="cosmology with kwargs", meta=None, **kwargs):
        super().__init__(name=name, meta=meta, **kwargs)

+    def __equiv__(self, other):


This makes me wonder whether it is really useful to make this an abstractmethod. Could the default just call __eq__? (Which is allowed to return NotImplemented as well)

I actually think the default should be what I implement for FLRW. Cosmology has no parameters, but any subclass will, so that equivalency check will be for 1) the same class and 2) the same parameters.
~~I refactored __eq__ to use __equiv__ to minimize code duplication. It always uses Cosmology.__equiv__, because self.__equiv__ is overwritten in subclasses, like FlatLambdaCDM~~

mhvk · 2021-09-06T19:33:59Z

+
+    def test_has_expected_attributes(self, info):
+        """Test ``CosmologyInfo`` has all the expected attributes."""
+        assert info._stats == []


I'd generally not test private attributes, but rather (try to) test their effects.

nstarman · 2021-09-13T20:26:54Z

Thanks @mhvk for the detailed review. I think I addressed all points and have moved a fair bit of the bells and whistles to future PRs.

mhvk

As I'm on the train from London to The Netherlands, my review is a bit less thoughtful than I would've liked, but hopefully something useful. Overall, this looks nice!

mhvk · 2021-09-19T11:33:33Z

+    an ABC and subclasses must override some methods.
+    """
+
+    @abc.abstractmethod


Maybe because of this. But is it actually used?

Yeah, that's one reason. FLRWSubclassTest is used here for all the FLRW-subclass tests, like TestLambdaCDM.
And if someone wanted to write their own FLRW subclass and use the test suite, then they too would have to define a setup_class().

When I start migrating the tests from test_cosmology to the new test suite I'll add

@abc.abstractmethod def test_w(self, cosmo): """`FLRW.w` is abstract and subclasses must provide a test."""

mhvk

Now looks very good. Still a few comments you may want to consider - but nothing to stop merging, so approved!

mhvk · 2021-09-23T12:45:07Z

+        # classes, different parameters. (1) & (3) => True, (2) & (4) => False.
+
        if not isinstance(other, Cosmology):
            return False


Should this not still allow other to try? In principle a non-Cosmology other might have everything in place...

Effectively, this would mean that self.__equiv__ should do the instance check (which of course the base class here already does by insisting it is the same class), i.e., this stanza should just be removed.

SGTM. It would be very cool to allow equivalence checks against 3rd-party cosmologies using to_format(). I made a card for this. https://github.com/astropy/astropy/projects/7#card-69385769

Essentially if it fails the class check it will try other = Cosmology.from_format(other) and then proceed

mhvk · 2021-09-23T12:55:35Z

-            True if all immutable fields are equal, False otherwise.
+            True if cosmologies are equivalent, False otherwise.
        """
+        # The options are: 1) same class & parameters; 2) same class, different


General thought given the comment below about python: I think generally python for things like self == other will only call other.__eq__(self) if other is a strict subclass of self. In principle, you could do the same here, though I don't know if there is much benefit to it.

~~Related note, I should add __req__.~~
Doesn't exist

How does Numpy do 1 == array if python only "call[s] other.__eq__(self) if other is a strict subclass of self" ?

Sorry, I was unclear: if the two are different classes, both get called; but if isinstance(other, type(self)) and type(other) is not type(self), then only other.__eq__(self) is called - since presumably, the subclass knows how to deal with the superclass. See https://docs.python.org/3/reference/datamodel.html#object.__radd__

(Actually, that suggests self.__eq__(other) may still get called, but just second - not completely sure!!)

Thanks for the explanation! I'll make a card for followup on this. Seems good to have it implemented exactly like python, but lower priority because it already works as expected.

https://github.com/astropy/astropy/projects/7#card-69390093

@nstarman

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

@nstarman

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

@nstarman

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

@nstarman

Parameters store units and equivalencies and have a new method “validate” to make a quantity from the value using the stored unit and equivalencies. This method is safe for non-Parameters. Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

@nstarman

Co-authored-by: Marten van Kerkwijk <[email protected]> Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

@nstarman

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

@nstarman

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

nstarman · 2021-09-23T15:54:12Z

Thanks @mhvk for the detailed conversation! This PR is definitely the better for it and I think one of the 3 most foundational PRs I've worked on though far as a @astropy/cosmology maintainer. Lots of avenues for expansion.

mhvk · 2021-09-23T16:37:15Z

This was a really nice cleanup & improvement!

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

github-actions Bot added cosmology testing labels Sep 2, 2021

nstarman force-pushed the cosmo_param_property branch 2 times, most recently from e64d637 to dcc1127 Compare September 2, 2021 04:33

nstarman added the Enhancement label Sep 2, 2021

nstarman requested a review from mhvk September 2, 2021 04:48

nstarman force-pushed the cosmo_param_property branch 2 times, most recently from 4c1a293 to e6dacc2 Compare September 2, 2021 18:29

mhvk reviewed Sep 2, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_property branch from e6dacc2 to ca9ffa5 Compare September 3, 2021 22:22

nstarman mentioned this pull request Sep 3, 2021

Pep8 doesn't speak to the walrus #12147

Closed

nstarman force-pushed the cosmo_param_property branch 8 times, most recently from c9f8c8e to fb68914 Compare September 6, 2021 04:42

nstarman requested a review from mhvk September 6, 2021 04:57

nstarman added the whatsnew-needed label Sep 6, 2021

nstarman force-pushed the cosmo_param_property branch 2 times, most recently from c035b00 to 7f46f5c Compare September 6, 2021 19:21

mhvk reviewed Sep 6, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_property branch from bc5d445 to fe04cea Compare September 9, 2021 20:05

nstarman requested a review from mhvk September 9, 2021 21:00

nstarman force-pushed the cosmo_param_property branch from fe04cea to 39ce4ac Compare September 10, 2021 04:27

nstarman requested a review from adrn September 10, 2021 20:59

nstarman force-pushed the cosmo_param_property branch 3 times, most recently from 7c5f09a to 2ed5282 Compare September 13, 2021 17:33

nstarman force-pushed the cosmo_param_property branch from 3463550 to 7a46c94 Compare September 17, 2021 19:19

mhvk reviewed Sep 19, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_property branch 4 times, most recently from 86c0cd3 to b351ef6 Compare September 22, 2021 18:48

nstarman requested a review from mhvk September 22, 2021 18:50

mhvk approved these changes Sep 23, 2021

View reviewed changes

nstarman and others added 7 commits September 23, 2021 10:56

Mixin class to check and enforce flatness

67be768

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

Parameterize

4dfa1f0

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

generalize repr using Parameter

1efd8bf

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

Add method is_equivalent to compare Cosmologies

3997a07

Co-authored-by: Marten van Kerkwijk <[email protected]> Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

documentation

82ea689

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

make m_nu lazy

a6928bb

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

nstarman force-pushed the cosmo_param_property branch from b351ef6 to a6928bb Compare September 23, 2021 14:56

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

nstarman merged commit 242977d into astropy:main Sep 23, 2021

nstarman deleted the cosmo_param_property branch September 23, 2021 15:56

bmorris3 mentioned this pull request Jan 8, 2023

Mention lightkurve in OOP intro astropy/astropy-workshop#221

Merged

Conversation

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

Uh oh!

Checklist for package maintainer(s)

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nstarman commented Sep 3, 2021

Uh oh!

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

Uh oh!

Comment last updated at 2021-09-22 18:48:49 UTC

Uh oh!

mhvk commented Sep 4, 2021

Uh oh!

nstarman commented Sep 4, 2021

Uh oh!

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

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Sep 12, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nstarman commented Sep 13, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Sep 20, 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!

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

nstarman commented Sep 2, 2021 •

edited

Loading

pep8speaks commented Sep 3, 2021 •

edited

Loading

nstarman commented Sep 6, 2021 •

edited

Loading

nstarman Sep 12, 2021 •

edited

Loading

nstarman Sep 20, 2021 •

edited

Loading

nstarman Sep 23, 2021 •

edited

Loading

nstarman Sep 23, 2021 •

edited

Loading