Skip to content

Add Metadata Attribute to Cosmology - #11542

Merged
mhvk merged 11 commits into
astropy:mainfrom
nstarman:cosmo_add_meta
May 25, 2021
Merged

Add Metadata Attribute to Cosmology#11542
mhvk merged 11 commits into
astropy:mainfrom
nstarman:cosmo_add_meta

Conversation

@nstarman

@nstarman nstarman commented Apr 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Adds metadata to cosmologies. The original motivation was that all the parameter dictionaries for built-in realizations have all this extra info that isn't being used. Let's just stick the extra info in metadata! Seems generically useful for e.g. adding references to user-defined cosmologies.

Should the metadata be mutable? yes, I think this should be the one mutable thing on a cosmology. It is never used by the cosmology.

Ready for review! But plz merge #11540 first. Then I can rebase this since there's some overlap. 🎉
request review: @adrn @mhvk @embray @dhomeier

@pep8speaks

pep8speaks commented Apr 13, 2021 •
edited
Loading

Copy link
Copy Markdown

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-05-25 00:38:48 UTC

@github-actions

github-actions Bot commented Apr 13, 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?

This was referenced Apr 14, 2021
Merged
Merged
Closed
@nstarman
nstarman force-pushed the cosmo_add_meta branch 4 times, most recently from 97841ef to 237e4c0 Compare April 16, 2021 06:00
nstarman
nstarman commented Apr 16, 2021
View reviewed changes
Comment thread astropy/cosmology/core.py Resolved
nstarman
nstarman commented Apr 17, 2021
View reviewed changes
Comment thread astropy/cosmology/core.py Resolved
Comment thread astropy/cosmology/core.py Resolved
@nstarman
nstarman marked this pull request as ready for review April 17, 2021 03:32
@nstarman nstarman mentioned this pull request Apr 17, 2021
Closed
@nstarman nstarman changed the title WIP: Add Metadata Attribute to Cosmology Add Metadata Attribute to Cosmology Apr 17, 2021
@nstarman

nstarman commented Apr 19, 2021

Copy link
Copy Markdown
Member Author

@pllim, not v4.3 critical, but IMO this is ready for merge. Hopefully if someone reviews, they'll agree and then this would be nice in v4.3

@pllim pllim added this to the v4.3 milestone Apr 19, 2021
@pllim

pllim commented Apr 19, 2021

Copy link
Copy Markdown
Member

Not sure who should review here...

@nstarman

nstarman commented Apr 19, 2021

Copy link
Copy Markdown
Member Author

The cosmology maintainer 😢

@adrn

adrn commented Apr 30, 2021

Copy link
Copy Markdown
Member

On first glance, looks good, but won't have time to review carefully today. OK to move milestone to 5.0? (unless someone else has time to review today!)

@pllim pllim modified the milestones: v4.3, v5.0 Apr 30, 2021
@pllim

pllim commented Apr 30, 2021

Copy link
Copy Markdown
Member

I tentatively moved the milestone as requested. I'd rather this gets a proper review than to rush things in last minute. If the situation changes, please move the milestone back. Thanks!

nstarman
nstarman commented May 3, 2021
View reviewed changes
Comment thread astropy/cosmology/realizations.py Resolved
@nstarman

nstarman commented May 16, 2021

Copy link
Copy Markdown
Member Author

ping @adrn. I'm not sure if the 4.3 feature freeze is still in effect, but if it isn't... Thanks!

@pllim

pllim commented May 17, 2021

Copy link
Copy Markdown
Member

Branching happened, so you can merge 5.0 stuff now. FYI.

@nstarman

nstarman commented May 23, 2021

Copy link
Copy Markdown
Member Author

Request review: @adrn @mhvk @embray @dhomeier

mhvk
mhvk reviewed May 24, 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.

This looks very nice! I really only have one rather nitpicky comment, about __all_subclasses__, which seems a bit out of scope here (and possibly not the ideal way to go about it). If we can keep this as a short function for now where it is needed, then I'm happy to get it in.

Comment thread astropy/cosmology/core.py
ba = self._init_signature.bind_partial(**new_init)
except TypeError as e:
# for backward compatibility, map TypeError to AttributeError
warnings.warn("Starting in Astropy v5.0, passing an unrecognized "

@mhvk mhvk May 24, 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.

For follow-up, we can now get rid of this...

@nstarman nstarman May 24, 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.

#11785

Comment thread astropy/cosmology/realizations.py Resolved
adrn
adrn reviewed May 24, 2021
View reviewed changes

@adrn adrn left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM aside from a very minor nitpick! But I agree with @mhvk's points about __all_subclasses__

Comment thread astropy/cosmology/tests/test_cosmology.py Resolved
Comment thread astropy/cosmology/core.py Resolved
@nstarman
nstarman force-pushed the cosmo_add_meta branch 3 times, most recently from 000017e to 4990186 Compare May 24, 2021 18:31
nstarman and others added 11 commits May 24, 2021 20:37
@nstarman
add metadata attr to Cosmo
71bd448 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
metadata kwargs in all cosmos
bad356d 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
changelog
66b5c3b 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
also store init signature on baseclass
2d97085 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
add metadata mutability test
ab5e073 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
improve docstrings and cloning logic
c6b0542 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
test cloning, with metadata
2126dc1 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
correct docstring class name fstring
591385a 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
move subclass recursion function
3d6bba7 
also simplify recursion

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
add mapping to docs/conf.py
9c68c74 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman @adrn
Stylistic improvements
b462a3a 
Co-authored-by: Adrian Price-Whelan <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
mhvk
mhvk approved these changes May 25, 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.

OK, thanks, looks all good now! Merging...

@mhvk
mhvk merged commit b669023 into astropy:main May 25, 2021
@nstarman
nstarman deleted the cosmo_add_meta branch May 25, 2021 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrn adrn adrn left review comments
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

cosmology Enhancement Ready-for-final-review

Projects

Cosmology, the Expansion
Status: Done

Milestone

v5.0

Development

Successfully merging this pull request may close these issues.

5 participants

@nstarman @pep8speaks @pllim @adrn @mhvk