New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cosmo: validating and setting Parameters #12190
Conversation
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 |
134c62c
to
9052da9
Compare
124da15
to
cb70288
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, orFalse
otherwise (with the default__set__
raising onFalse
); could then do things likeverifier=lambda x: x>=0.
That said, in-line I'm suggestingfvalidate=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.
p.s. I'm wondering a bit whether it would make sense to in fact inherit from |
cb70288
to
a658c00
Compare
6554129
to
3005a30
Compare
3005a30
to
95d3cf3
Compare
Rebased to fix merge conflicts. |
95d3cf3
to
3e61d72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
a9d9b30
to
84185e9
Compare
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
23bf916
to
8caa417
Compare
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
astropy/cosmology/parameter.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of pre-defined validators is incomplete... My tendency would be to just write str
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made it a classproperty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
astropy/cosmology/parameter.py
Outdated
|
||
_registry_validators = {} | ||
|
||
def __init__(self, fget=None, fvalidate="default", doc=None, *, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think fget
is used at all anymore (and if it is, would not do anything), so remove!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a big oversight. Thanks for catching!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
astropy/cosmology/flrw.py
Outdated
@@ -262,6 +246,12 @@ def __init__(self, H0, Om0, Ode0, Tcmb0=0.0*u.K, Neff=3.04, m_nu=0.0*u.eV, | |||
self._Tnu0 = 0.0 * u.K | |||
self._Onu0 = 0.0 | |||
|
|||
# now set m_nu Parameter (Note the value is calculated and not | |||
# influenced by this value) | |||
self.m_nu = m_nu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part still feels slightly odd. Somehow it seems cleaner to just do the calculation and set the private property. But no big deal!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could just do self.m_nu = ...
With the Ellipsis it feels less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
baa43c6
to
bc1e7dc
Compare
bc1e7dc
to
0f6f8e9
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
astropy/cosmology/flrw.py
Outdated
# Parameter details | ||
|
||
@m_nu.validator | ||
def m_nu(self, param, _): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above. The validator for Ob0
, in contrast, feels just right.
astropy/cosmology/parameter.py
Outdated
|
||
return register | ||
|
||
@classproperty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd not add this - it is useful for developers only and those can be pointed to the private attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
docs/cosmology/dev.rst
Outdated
@lazyproperty # so only does expensive computation once | ||
def m_nu(self): | ||
""" Mass of neutrino species""" | ||
@m_nu.setter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct any more, I think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Co-authored-by: Marten van Kerkwijk <mhvk@astro.utoronto.ca> Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
0f6f8e9
to
95549de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks all great! Just one remaining suggestion for the docs (plus a very small typo). I'll approve in the meantime.
... | ||
return u.Quantity(numass, self.__class__.m_nu.unit) | ||
return value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to copy the whole validator, as right now it misses the point that you can access the cosmology instance easily.
docs/cosmology/dev.rst
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a values -> a value's
@@ -237,7 +237,6 @@ class to use. Details about metadata treatment are in | |||
``Cosmology.from_format.help("mapping")``. | |||
|
|||
.. code-block:: python | |||
:emphasize-lines: 12,13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit unrelated but fine to get it in too!
95549de
to
93a4d29
Compare
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! |
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) <nstarkman@protonmail.com>
93a4d29
to
2fa1897
Compare
…190-on-v5.0.x Backport PR #12190 on branch v5.0.x (Cosmo: validating and setting Parameters)
Parameter value validation and allowing parameters to be "fixed"/derived, e.g. setting
Ode0
inFlatFLRWMixin
.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.
Extra CI
label.no-changelog-entry-needed
label. If this is a manual backport, use theskip-changelog-checks
label unless special changelog handling is necessary.astropy-bot
check might be missing; do not let the green checkmark fool you.backport-X.Y.x
label(s) before merge.