Cosmo: validating and setting Parameters by nstarman · Pull Request #12190 · astropy/astropy

nstarman · 2021-09-23T20:36:30Z

Parameter value validation and allowing parameters to be "fixed"/derived, e.g. setting Ode0 in FlatFLRWMixin.

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-23T20:36:34Z

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:38:59 UTC

mhvk

I like how this is looking, but looking at some of the after-setting tests that remain, I wonder whether it might make things easier if one had two options:

A verifier, that just returns True if a value is OK, or False otherwise (with the default __set__ raising on False); could then do things like verifier=lambda x: x>=0. That said, in-line I'm suggesting fvalidate=float, so perhaps that has its uses too...
A setter where the user simply is in full control (but with the once-only still enforced by __set__). The advantage is that this is expected to exist from things like (lazy)property.

Mostly bringing this up since it would be good to get this right.

mhvk · 2021-09-24T12:31:17Z

p.s. I'm wondering a bit whether it would make sense to in fact inherit from lazyproperty and add the validation (and override __set__, but still using super()). One nice bit with lazyproperty is that it was made thread-safe...

nstarman · 2021-10-28T23:42:14Z

Rebased to fix merge conflicts.

mhvk

Some comments, but while going I realized I am perhaps reviewing at the wrong point, since you still refer to getter here while this is no longer allowed. My sense would be to similarly disallow setter and just use validate - at which point Parameter probably does not need to inherit from property after all...

@nstarman

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

@nstarman

so can have pre-registered options 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]>

mhvk

Looks good now! A few small comments, mostly that I think one more piece can be removed.

p.s. My tendency would be to ask permission to still get this in 5.0, so the new Parameter does not have to be changed.

mhvk · 2021-11-02T20:40:42Z

+        Function to get the value from instances of the cosmology class.
+        If None (default) returns the corresponding private attribute.
+        Often not set here, but as a decorator with ``getter``.
+    fvalidate : callable[[object, object, Any], Any] or {'default', 'float'}, optional


List of pre-defined validators is incomplete... My tendency would be to just write str here.

SGTM. I'm adding a property registered_validators(self): return self._registry_validators.keys() so it's easy to discover what's a valid str.

made it a classproperty

mhvk · 2021-11-02T20:43:46Z

+
+    _registry_validators = {}
+
+    def __init__(self, fget=None, fvalidate="default", doc=None, *,


I don't think fget is used at all anymore (and if it is, would not do anything), so remove!?

That was a big oversight. Thanks for catching!

mhvk · 2021-11-02T20:54:26Z


+        # now set m_nu Parameter (Note the value is calculated and not
+        # influenced by this value)
+        self.m_nu = m_nu


This part still feels slightly odd. Somehow it seems cleaner to just do the calculation and set the private property. But no big deal!

I could just do self.m_nu = ... With the Ellipsis it feels less confusing.

astrofrog · 2021-11-03T12:12:56Z

I am ok with backporting this as it fixes an API being introduced in 5.0. However please make sure the CI is all green before merging, there have been too many CI breakages in the last week 😅. The CircleCI builds might need restarting.

mhvk

Looks good, but still some missing pieces in the documentation - can they be doctested?
Also, I'd not do the new classproperty - that is developer information so find to refer to a private attribute.

mhvk · 2021-11-03T15:39:10Z

+    # Parameter details
+
+    @m_nu.validator
+    def m_nu(self, param, _):


See comment above. The validator for Ob0, in contrast, feels just right.

mhvk · 2021-11-03T15:44:37Z

+
+        return register
+
+    @classproperty


I'd not add this - it is useful for developers only and those can be pointed to the private attribute.

mhvk · 2021-11-03T15:48:00Z

-        @lazyproperty  # so only does expensive computation once
-        def m_nu(self):
-            """ Mass of neutrino species"""
+        @m_nu.setter


This is not correct any more, I think!

@nstarman

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

mhvk

This looks all great! Just one remaining suggestion for the docs (plus a very small typo). I'll approve in the meantime.

mhvk · 2021-11-03T19:51:07Z

+                return value
            ...
-            return u.Quantity(numass, self.__class__.m_nu.unit)
+            return value


I'd suggest to copy the whole validator, as right now it misses the point that you can access the cosmology instance easily.

mhvk · 2021-11-03T19:51:47Z

+auto-locking), so ``self.H0 = H0`` will use this setter and put the value on
+"._H0". The advantage of this method over direct assignment to the private
+attribute is the use of validators. |Parameter| allows for custom value
+validators, using the method-decorator ``validator``, that can check a values


a values -> a value's

mhvk · 2021-11-03T19:53:11Z

 ``Cosmology.from_format.help("mapping")``.

 .. code-block:: python
-    :emphasize-lines: 12,13


Seems a bit unrelated but fine to get it in too!

nstarman · 2021-11-03T21:26:10Z

Thanks @mhvk for the multiple rounds of review! This refactoring of Parameter is a significant improvement over the original implementation. And thanks @astrofrog for getting this in v5.0!

@nstarman

use the simple unit munger. Maybe in future compact all the long neutrino calculations into a breakout m_nu validator Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

…190-on-v5.0.x Backport PR #12190 on branch v5.0.x (Cosmo: validating and setting Parameters)

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

github-actions Bot added the cosmology label Sep 23, 2021

nstarman force-pushed the cosmo_param_fixed branch 2 times, most recently from 134c62c to 9052da9 Compare September 23, 2021 20:40

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

nstarman force-pushed the cosmo_param_fixed branch from 124da15 to cb70288 Compare September 24, 2021 00:58

mhvk reviewed Sep 24, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_fixed branch from cb70288 to a658c00 Compare September 29, 2021 05:40

nstarman mentioned this pull request Sep 30, 2021

cosmo: transfer Cosmology tests and test each method. #12210

Merged

9 tasks done

nstarman force-pushed the cosmo_param_fixed branch 10 times, most recently from 6554129 to 3005a30 Compare October 14, 2021 02:55

nstarman requested review from adrn and mhvk October 19, 2021 19:30

nstarman force-pushed the cosmo_param_fixed branch from 3005a30 to 95d3cf3 Compare October 28, 2021 23:41

nstarman force-pushed the cosmo_param_fixed branch from 95d3cf3 to 3e61d72 Compare October 28, 2021 23:55

mhvk reviewed Oct 29, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_fixed branch 2 times, most recently from a9d9b30 to 84185e9 Compare October 29, 2021 21:57

nstarman requested a review from mhvk October 29, 2021 22:01

nstarman added 3 commits November 1, 2021 22:00

remove getter and validator in favor of setter

c6871f4

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

move Parameter to own file

1f47831

so can have pre-registered options Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

Un-property Parameter

8caa417

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

nstarman force-pushed the cosmo_param_fixed branch from 23bf916 to 8caa417 Compare November 2, 2021 02:00

enable cosmology units when reading from a file

bd1be52

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

mhvk reviewed Nov 2, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_fixed branch 2 times, most recently from baa43c6 to bc1e7dc Compare November 3, 2021 01:12

nstarman requested a review from astrofrog November 3, 2021 01:19

nstarman added the backport-v5.0.x label Nov 3, 2021

nstarman modified the milestones: v5.1, v5.0 Nov 3, 2021

nstarman force-pushed the cosmo_param_fixed branch from bc1e7dc to 0f6f8e9 Compare November 3, 2021 01:24

mhvk reviewed Nov 3, 2021

View reviewed changes

remove fget completely

ce3e883

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

nstarman force-pushed the cosmo_param_fixed branch from 0f6f8e9 to 95549de Compare November 3, 2021 18:19

nstarman requested a review from mhvk November 3, 2021 18:30

mhvk approved these changes Nov 3, 2021

View reviewed changes

nstarman force-pushed the cosmo_param_fixed branch from 95549de to 93a4d29 Compare November 3, 2021 21:24

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

change m_nu validation

2fa1897

use the simple unit munger. Maybe in future compact all the long neutrino calculations into a breakout m_nu validator Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

nstarman force-pushed the cosmo_param_fixed branch from 93a4d29 to 2fa1897 Compare November 3, 2021 21:38

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

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Nov 3, 2021

Backport PR astropy#12190: Cosmo: validating and setting Parameters

49e3fe1

meeseeksmachine mentioned this pull request Nov 3, 2021

Backport PR #12190 on branch v5.0.x (Cosmo: validating and setting Parameters) #12386

Merged

nstarman deleted the cosmo_param_fixed branch November 3, 2021 22:38

nstarman mentioned this pull request Nov 3, 2021

fix bug where redshift not recognized as unit #12390

Merged

9 tasks done

astrofrog added a commit that referenced this pull request Nov 4, 2021

Merge pull request #12386 from meeseeksmachine/auto-backport-of-pr-12…

60cc0c5

…190-on-v5.0.x Backport PR #12190 on branch v5.0.x (Cosmo: validating and setting Parameters)


		_registry_validators = {}

		def __init__(self, fget=None, fvalidate="default", doc=None, *,

Conversation

nstarman commented Sep 23, 2021 • edited by mhvk 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 23, 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:38:59 UTC

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!

mhvk commented Sep 24, 2021

Uh oh!

nstarman commented Oct 28, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

astrofrog commented Nov 3, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

nstarman commented Sep 23, 2021 •

edited by mhvk

Loading

pep8speaks commented Sep 23, 2021 •

edited

Loading