Add field default to Parameter

[nstarman]

Split off from #15168. This adds the field default to Parameter. Currently it doesn't do anything since the default value is specified on __init__(), but it is useful for introspecting a cosmology class e.g. LambdaCDM.H0.default and it will also replace __init__() when Cosmology is a dataclass for v6.

[github-actions]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• 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 instructions for rebase and squash.
• 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. Codestyle issues can be fixed by the bot.
• 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 this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; 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]

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

[nstarman]

@eerovaher Let's move the discussion about NotImplemented to here.

[nstarman]

And for comparison... I'm adding a 2nd commit with the sentinel value

[nstarman]

Just to say both passed.

[eerovaher]

Can you explain in more detail why HASNODEFAULT is needed and why wouldn't it be possible to use None as the sentinel value instead?

[nstarman]

If None is the sentinel value then it can't be used as a real value, which it is for Ob0.

[nstarman]

inspect, for example, uses _missing as their custom sentinel value for the same reason.

[nstarman]

Python needs Sentinels

[nstarman]

In commit 0aa86f9 I had NotImplemented as the sentinel because there's almost 0% chance someone needs that as a real value, unlike None.

[mhvk]

One could also consider np._NoValue for the sentinel.

[nstarman]

One could also consider np._NoValue for the sentinel.

I think we should avoid private api. Otherwise insect._empty, used by inspect.Signature for this purpose, would be best.

[mhvk]

The advantage of np._NoValue over inspect._empty is that users are not unlikely to have seen it in documentation, and that it overrides __repr__ to be "". But, yes, nominally private API. I think I'd still pick it over a homebrew one, but if you go with yours I would suggest to add a __repr__ (maybe ""?) so that the sphinx docs look OK.

p.s. PEP 661 was interesting to read, thanks for the link!

[nstarman]

The advantage of np._NoValue over inspect._empty is that users are not unlikely to have seen it in documentation, and that it overrides __repr__ to be "".
But, yes, nominally private API. I think I'd still pick it over a homebrew one, but if you go with yours I would suggest to add a __repr__ (maybe ""?) so that the sphinx docs look OK.

I like the the repr! But the values in Cosmology instances don't have to by NumPy objects, so I don't want to use a numpy object as the sentinel placeholder. But I will definitely do the repr!

I think I'll also name the object MISSING, which is in line with https://docs.python.org/3/library/dataclasses.html#dataclasses.MISSING and inspect._empty

Unfortunately we should not just use dataclasses.MISSING

As shown above, the [MISSING](https://docs.python.org/3/library/dataclasses.html#dataclasses.MISSING) value is a sentinel object used to detect if some parameters are provided by the user. This sentinel is used because None is a valid value for some parameters with a distinct meaning. No code should directly use the [MISSING](https://docs.python.org/3/library/dataclasses.html#dataclasses.MISSING) value.

from https://docs.python.org/3/library/dataclasses.html#dataclasses.field

p.s. PEP 661 was interesting to read, thanks for the link!

👍

[nstarman]

I changed the repr to "<MISSING>" which is better than what Enum was doing. Thanks @mhvk !

[eerovaher]

If None is the sentinel value then it can't be used as a real value, which it is for Ob0.

Ob0 is the ratio of baryonic matter density and critical density at z=0. I'd expect that value to be a float and I don't understand how None could be a valid ratio. If the default value of Ob0 would be a float, would MISSING still be needed?

[nstarman]

Ob0 is the ratio of baryonic matter density and critical density at z=0. I'd expect that value to be a float and I don't understand how None could be a valid ratio.

I've long wondered at the specific design choice myself, it predating my becoming the maintainer for cosmology.
The ideas seems to be that you can work with a general model, not specifying the mass constituents (Ob0=None turns off both baryons and dark matter Odm0, leaving only the general Om0).
So None isn't a valid ratio, and that's kind of the point.
Now if this is the best design decision... I'm not exactly sure. On one hand it allows for general mass models without having to differentiate flavours, OTOH it means that cosmo.Odm0 and cosmo.Ob0 and calculations that use these quantities can raise errors in expected but annoying ways. It's definitely a balance between a desire for clean theory and the hard practicalities of code. There might be a better option, I just haven't worked through the idea nor the refactor and deprecations.

If the default value of Ob0 would be a float, would MISSING still be needed?

Debatably. But I would lean to yes. Particularly because I don't think just doing Ob0 = None-> 0 is sufficient. There's a larger refactor required to allow for more general models that don't have baryons and DM, only mass.

So my preference would be to emulate Python's dataclasses and inspect libraries.

[eerovaher]

What if the default for Ob0 would be math.nan or np.nan?

[nstarman]

Interesting! I hadn't considered that. My hesitations are 3-fold:

1. I don't want users to have to differentiate between math and numpy's NaN.

2. This doesn't solve the underlying issue of multi-modality. I would much rather have a separate class that has matter only (without baryons & DM). So then None wouldn't be needed for Ob0, but for a different reason.

3. We let users define custom cosmologies. None is sufficiently common that I don't really want to disallow it as a default value for a Parameter. It seems unnecessary to force anyone implementing a similar pattern to our Ob0 to also have to change. Just because I don't like the multimodality doesn't mean it isn't a decent solution in another context. It's for similar reasons that Python made MISSING in dataclasses and _empty in inspect. It all comes back to the fact that Python forces us to roll our own Sentinels, as is done in this PR.

[nstarman]

@eerovaher, do you agree with this reasoning? This code pattern is suggested in https://peps.python.org/pep-0484/#support-for-singleton-types-in-unions for how to build statically recognizable sentinel values (until the PEP for Sentinels is in for py3.1X).

[eerovaher]

I don't use cosmology, so if there are any quirks because of historical reasons then I am blissfully unaware of them. As far as I can tell the appropriate data type for all the parameter values is float (or ndarray of floats or Quantity or something similar). If a sentinel value is needed then it should be possible to use nan, which is a valid float, or None, which is a Python builtin commonly used for this purpose. It is not true that math.nan is difficult to use with numpy or vice versa:

>>> import math
>>> import numpy as np
>>> math.isnan(np.nan)
True
>>> np.isnan(math.nan)
True

So in short I still don't understand why a brand new sentinel value is needed here. The reason dataclasses or inspect have their own sentinel value is that they don't have the luxury of knowing what data types they will be working with. I wouldn't be recommending nan as a sentinel for strings.

[nstarman]

It is not true that math.nan is difficult to use with numpy or vice versa:

They are statically different and also floats cannot be differentiated statically. I think that's important.

@overload
def function(x: Literal[inf]) -> NoReturn:  # Not a valid entry to `Literal`
     ...

@overload
def function(x: float) -> np.ndarray:
     ...

def function(x: float) -> np.ndarray:
     if x is inf: 
        raise ValueError
     return ...

So I actually like None as a sentinel better than nan. But None can't be used because of cosmology's historical quirks (a good turn of phrase). I think this actually ends up being a minor point since users will never see this unless they are deep-diving into cosmology's internals. If / when cosmology's quirks are cleaned up then MISSING can be deprecated in favor of None (a suitably long deprecation period in which both are valid)

[eerovaher]

I understand now why a brand new sentinel value is needed. It would be good to use None, but Ob0 already uses None as its default value and the default in Parameter needs to be different. And it is not good to use nan because we'd like to be able to recognize the Parameter default by type, not just by value.

Now that the big picture is cleared up I just have handful of small comments.

[mhvk]

This looks great. My comments are really all nitpicks, about comments and docstrings. Approving since it is OK as is too.

[mhvk]

I think I'd write this as

assert all_parameter.default is MISSING or all_parameter.default is None or isinstance(all_parameter.default, (float, u.Quantity))

This tests something closer to what is actually done (and Sentinel is an implementation detail).

[nstarman]

I like separating out the MISSING, but NoneType is py3.10+, so there's no need to separate that out. Ruff will auto-upgrade type(None) -> NoneType when we next bump versions.

[mhvk]

I meant that it is clearer to do all_parameter.default is None than isinstance(all_parameter.default, NoneType) -- but obviously not a big deal!

[nstarman]

Thanks @eerovaher and @mhvk for the detailed back and forth! Let's squash this in 🚀

[nstarman]

Ping @eerovaher if you approve as well.