Repository navigation
refactor(cosmo): refactor separate baryon component - #17522
Conversation
|
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
Signed-off-by: nstarman <[email protected]>
4da417e to
90fd25a
Compare
| class _BaryonComponent: | ||
| """The cosmology has attributes and methods for the baryon density.""" | ||
|
|
||
| Ob0: float |
There was a problem hiding this comment.
Why is Ob0 not moved to the new mixin?
There was a problem hiding this comment.
in _BaryonComponent we're not mandating that Ob0 is a parameter of the cosmology, just an attribute.
There was a problem hiding this comment.
This will allow, e.g. models without Ob0 as a cosmological parameter.
There was a problem hiding this comment.
My thinking is that if Ob(z) is well defined then Ob0=Ob(0) must also be well defined, so why can't this mixin handle both?
There was a problem hiding this comment.
Ob0=Ob(0) is well defined as Ob0 (assuming inv_efunc(0) = 1). In (nearly?) all model formulations Ob0 is either
- Not a part of the cosmology, e.g. a Lambda-only universe or a radiation-only universe.
- A settable parameter of the cosmology
- A derived parameter of the cosmology.
So I don't want to make Ob0 "defined" in the sense of a "Parameter" as it is on the FLRW objects.
Any class that subclasses _BaryonComponent is in category 2 or 3, but if it's a derived parameter then it is vastly preferable to make Ob0 a @property, not a "astropy.cosmology.Parameter"
There was a problem hiding this comment.
I don't think I know enough about cosmology to judge how good this reasoning is, but at least the reasoning is now written down.
| Ob0: float | ||
| """Omega baryons: density of baryonic matter in units of the critical density at z=0.""" | ||
|
|
||
| inv_efunc: Callable[[NDArray[Any]], NDArray[Any]] |
There was a problem hiding this comment.
Should this be an abstract method?
There was a problem hiding this comment.
I don't think so. The best would probably be to have a HasInvEFunc protocol, but I think this suffices. It allows subclasses to implement inv_efunc however they want (method, callable attribute) so long as it's present for Ob() to call.
There was a problem hiding this comment.
An abstract method in the mixin would indeed force the subclasses to implement inv_efunc() as a method and prevent them from implementing it as a callable attribute. It is not obvious to me why both options should be supported, but apparently you've thought about this.
There was a problem hiding this comment.
It is not obvious to me why both options should be supported
Nor to me, but the plan is factor out all these building blocks of a cosmology, organize them into a user-facing sub-module, build some of the classic pedagogical models, and make it easy for users to put these building blocks together to easily make custom cosmologies. So I'm keeping these mixin classes very general.
There was a problem hiding this comment.
We should keep in mind that expanding the public API is much simpler than restricting it, so it might be better to avoid making the API too flexible right now.
There was a problem hiding this comment.
True, but this is not an unreasonable scenario. In fact we kind of already do it in Astropy
https://github.com/astropy/astropy/blob/abc83b4e661d3abf720dfe4c68f04ae48c220258/astropy/cosmology/flrw/lambdacdm.py#L124C1-L125C82
Part of #17150
With this separation (and others like it) we will be able to define a cosmology w/out baryons, not just setting it to 0.