feat: uni-modal cosmology wrt Ob0 by nstarman · Pull Request #16847 · astropy/astropy

nstarman · 2024-08-19T02:07:17Z

Deprecates Ob0=None, changing it to Ob0=0.
Currently FLRW is multi-modal in that it supports both a cosmology with no baryon component and also one with a baryon component. Except that the no-baryon cosmology is implemented as an attribute that always raises an exception. This is not an ideal code pattern. From a code perspective the best is to have two different classes, each representing one of these modalities. Pragmatically, at least for now, just having Ob0 = 0 fulfills the same requirements without adding a new class nor all the child classes for the dark energy EoS. Longer term, one of the
goals of the cosmology API project and its application to Astropy is to make defining new cosmology classes significantly easier through more modular components. Then we can have true no-baryon cosmologies, not just baryons=0.

This is a breaking change since Ob0 = None is now Ob0 = 0 and no longer ever errors.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-08-19T02:07:42Z

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

I don't feel completely competent to judge the impact, but it certainly seems to make more sense to keep the class consistent like this, rather than sometimes getting errors. My comments are mostly about making clearer what is changed.

mhvk · 2024-08-19T18:45:24Z

        if value is None:
-            return value
+            warnings.warn(
+                "Ob0=None is deprecated, use Ob0=0 instead.",


Also mention that there will no failures!

mhvk · 2024-08-19T18:46:24Z

-        tba.arguments.pop("Ob0", None)
-        cosmo = cosmo_cls(*tba.args, **tba.kwargs)
-        with pytest.raises(ValueError):
-            cosmo.Ob(1)


Proabbly makes sense to check that cosmo.Ob(1) == 0

mhvk · 2024-08-19T18:49:29Z

 In this case the dark matter-only density at redshift 0 is available as class
 attribute ``Odm0`` and the redshift evolution of dark and baryonic matter
 densities can be computed using the methods ``Odm`` and ``Ob``, respectively.
-If ``Ob0`` is not specified at class instantiation, it defaults to ``None`` and


Should state that if Ob0 is not specified, it will be set to 0 and thus ignored in further calculations. Only then does the next example make sense (though I'd add cosmo.Ob(1) as well, to show it stays zero).

mhvk · 2024-08-19T18:51:48Z

+Setting ``Ob0 = None`` in FLRW cosmologies has been deprecated in favor of ``Ob0 =
+0.0``. Conceptually this is a change in that baryons are now always a component of the
+cosmology. Practically, the only change (besides that ``Ob0`` is never ``None``) is that
+methods relying on ``Ob0`` always work, rather than sometimes raising an exception.


Add ", instead by default taking the contribution of the baryons to be negligible."

mhvk

The other PRs brought this up again. Still seems reasonable, and I now have only a most minimal comment left. Approving, since either I'm right and this is a trivial change, or I'm not, and then what you have is OK.

mhvk · 2024-10-10T14:14:38Z

        # validation
-        assert Ob0.validate(cosmo, None) is None
+        with pytest.warns(
+            DeprecationWarning, match=re.escape("Ob0=None is deprecated")


Why the re.escape - this string should work fine directly, no?

Good Q. Not sure if I added this because of a test failure or as a matter of habit.

Hopefully habits will be auto-fixed astral-sh/ruff#13705

Indeed, this seems auto-detectable (it already detects f-strings without actual {...}).

mhvk · 2024-10-10T14:16:38Z

-            cosmo.Ob(1)
+        tba.arguments["Ob0"] = None
+        with pytest.warns(
+            DeprecationWarning, match=re.escape("Ob0=None is deprecated")


I now quickly checked, and it does just work without re.escape.

Deprecate Ob0=None, changing to Ob0=0 Signed-off-by: nstarman <[email protected]>

nstarman added this to the v7.0.0 milestone Aug 19, 2024

github-actions Bot added the cosmology label Aug 19, 2024

nstarman force-pushed the cosmo-Ob0-default branch 2 times, most recently from 6c8c678 to 9b48ed7 Compare August 19, 2024 14:47

nstarman marked this pull request as ready for review August 19, 2024 14:48

nstarman requested a review from a team as a code owner August 19, 2024 14:48

nstarman requested review from mhvk and removed request for a team August 19, 2024 14:48

nstarman force-pushed the cosmo-Ob0-default branch 2 times, most recently from b61406f to 8111cfb Compare August 19, 2024 15:11

astropy deleted a comment from github-actions Bot Aug 19, 2024

mhvk reviewed Aug 19, 2024

View reviewed changes

This was referenced Sep 8, 2024

test: move Parameter test mixin classes #16968

Merged

test: move FLRW-specific test #16969

Merged

nstarman force-pushed the cosmo-Ob0-default branch 2 times, most recently from e09f90f to 2e1320e Compare September 10, 2024 17:19

nstarman requested a review from mhvk October 9, 2024 20:10

nstarman modified the milestone: v7.0.0 Oct 9, 2024

nstarman mentioned this pull request Oct 10, 2024

Towards Pedagogical Cosmological Models #17150

Open

12 of 22 tasks

mhvk approved these changes Oct 10, 2024

View reviewed changes

feat: always have Ob0

2e41761

Deprecate Ob0=None, changing to Ob0=0 Signed-off-by: nstarman <[email protected]>

nstarman force-pushed the cosmo-Ob0-default branch from 2e1320e to 2e41761 Compare October 10, 2024 15:01

nstarman enabled auto-merge October 10, 2024 15:02

nstarman merged commit ecb98df into astropy:main Oct 10, 2024

nstarman mentioned this pull request Dec 4, 2024

fix: Ob0 is never None #17501

Merged

1 task

Conversation

nstarman commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 19, 2024

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!

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!

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!

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nstarman commented Aug 19, 2024 •

edited

Loading