Skip to content

split cosmology classes from realizations - #11345

Merged
mhvk merged 1 commit into
astropy:mainfrom
nstarman:cosmology_realizations
Apr 10, 2021
Merged

split cosmology classes from realizations#11345
mhvk merged 1 commit into
astropy:mainfrom
nstarman:cosmology_realizations

Conversation

@nstarman

@nstarman nstarman commented Feb 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Part of #10673

All this PR does is separate the class definitions of a cosmology with its realization(s).
As both the classes and realizations are intended to be accessed from the top-level namespace of the cosmology package, this PR should have no/minimal/very-easily-fixed impact on users.
Looking forward, this separation will make it easier to add separate entry points for both the classes and realizations so that 3rd party packages can augmentcosmology's offerings.

@github-actions

github-actions Bot commented Feb 23, 2021

Copy link
Copy Markdown
Contributor

👋 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
nstarman marked this pull request as ready for review February 28, 2021 03:38
@nstarman nstarman mentioned this pull request Feb 28, 2021
Closed
2 of 7 tasks
@nstarman

nstarman commented Mar 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@pllim, can this be milestoned to 4.3? I just want to get all the tests passing. I've found it encourages followup. Thanks!

@pllim pllim added this to the v4.3 milestone Mar 3, 2021
@nstarman
nstarman force-pushed the cosmology_realizations branch from 4860350 to 1b5ed97 Compare March 8, 2021 21:20
@nstarman

nstarman commented Mar 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@embray @mhvk @pllim. As the most recent reviewers of a cosmology PR, I thought you'd be the most appropriate to "@" for this one.

@pllim

pllim commented Mar 9, 2021

Copy link
Copy Markdown
Member

I can do general review but unfortunately I am uncomfortable with doing a detail review of the science content.

@nstarman

nstarman commented Mar 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I can do general review

Much appreciated! I think this PR only requires a general review, anyway. I'm just shifting code from the file core to realizations.

@nstarman

nstarman commented Apr 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@embray @mhvk @pllim. All tests passing and codecov above the 90% mark (it would be 100% but I replace some never-used code with a warning.)

Edit: rebased and test failures are unrelated.

@nstarman
nstarman force-pushed the cosmology_realizations branch 2 times, most recently from 9baed78 to a3ed89b Compare April 3, 2021 05:04
@nstarman

nstarman commented Apr 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@embray @mhvk @adrn. Switched change-log to Towncrier news fragment. Test failures unrelated.

Edit: rebased to get rid of unrelated mpl error.

@pllim

pllim commented Apr 3, 2021

Copy link
Copy Markdown
Member

Thanks for the ping. Unfortunately, I am not qualified to review this, so I'll have to recuse myself.

@nstarman
nstarman force-pushed the cosmology_realizations branch from a3ed89b to 8d4b871 Compare April 5, 2021 21:32
@nstarman nstarman mentioned this pull request Apr 6, 2021
Closed
mhvk
mhvk approved these changes Apr 10, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nstarman - as you actually promised in the PR, this is really just a technical one that does what I agree is a very logical split -- I'm afraid I had missed that sufficiently to worry this would take me a lot of time, while it turned out to be just 10 minutes.

Anyway, I like this very much - we really should be starting to use entrypoints more -- and I have only one request for a comment about the commented-out code (which I think, contrary to almost anytime else, is worth having here). Approving in the meantime.

Comment thread astropy/cosmology/realizations.py
cosmo.__doc__ = docstr.format(key, par["reference"])
else:
warnings.warn("Please open a PR for your added cosmology realization.")
# cosmo = LambdaCDM(

@mhvk mhvk Apr 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment here explaining that for a non-flat LCDM model, it would look like this?

@nstarman nstarman Apr 10, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Good suggestion. Done!

# For a non-flat LCDM realization, the following is the code necessary
# to create the realization, given the parameters. We comment this out
# as there are not currently any such built-in realizations.

@nstarman
split cosmology classes from realizations
7a43459 
* there are no non-flat realizations, so raise warning if 3rd party adds one.
* test realizations
* changelog

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the cosmology_realizations branch from 8d4b871 to 7a43459 Compare April 10, 2021 04:11
@mhvk

mhvk commented Apr 10, 2021

Copy link
Copy Markdown
Contributor

Great, thanks. Merging!

@mhvk
mhvk merged commit ec940f5 into astropy:main Apr 10, 2021
@nstarman
nstarman deleted the cosmology_realizations branch April 10, 2021 19:59
@nstarman nstarman mentioned this pull request May 23, 2021
Closed
2 of 7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

cosmology Refactoring

Projects

Cosmology, the Expansion
Status: Done

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

3 participants

@nstarman @pllim @mhvk