Skip to content

Simplify Cosmology cloning logic - #11515

Merged
adrn merged 1 commit into
astropy:mainfrom
nstarman:cosmo_store_init
Apr 12, 2021
Merged

Simplify Cosmology cloning logic#11515
adrn merged 1 commit into
astropy:mainfrom
nstarman:cosmo_store_init

Conversation

@nstarman

@nstarman nstarman commented Apr 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Massively simplify the cloning logic of Cosmology by storing the initialization signature and arguments on the class and instance, respectively. This is fine since Cosmo classes are immutable (expressed in old .clone docs).
The refactor generalizes the method to work for any subclass, user-defined or not, which was a notable limitation of the former implementation.
This PR will enable the next PR, which will allow for Cosmology to by useful for instantiating cosmologies that are "perturbations" around the default cosmology (see #10673)

Part of #10673

@mhvk @embray : Pretty much purely technical PR.

p.s. Are any new tests needed? this seems a pretty atomic change. The only reason I labelled it API is that it changes the errors that clone raises.

p.p.s. Can I add a warning that we'll be changing the error from an Attribute to TypeError at some later date? It really shouldn't be an AttributeError.

@github-actions

github-actions Bot commented Apr 10, 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 April 10, 2021 22:19
@nstarman

nstarman commented Apr 10, 2021

Copy link
Copy Markdown
Member Author

Aha! @adrn, I see you looked at this PR. You're now caught in my nefarious scheme to improve the cosmo package. Can I has review plz?

adrn
adrn reviewed Apr 11, 2021
View reviewed changes

@adrn adrn left a comment

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.

Nice! I think this is a nice refactor and only have some minor nitpicks (mostly around fixing some of the docstring language, while you're touching the code...).

On TypeError / AttributeError: What causes that TypeError to get thrown?

Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
Comment thread astropy/cosmology/core.py Outdated
@pep8speaks

pep8speaks commented Apr 11, 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-04-11 18:36:24 UTC

@nstarman

nstarman commented Apr 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

On TypeError / AttributeError: What causes that TypeError to get thrown?

@adrn, the old code throws AttributeError for any problem while cloning since it requires a property / attribute for each argument. In my opinion this is the wrong error to raise since it is specific to the implementation, not the intent of the code. For unrecognized arguments, the "right" error to throw for arguments is a TypeError . inspect.Signature.bind_partial correctly throws a TypeError... but for backward compatibility I map it back to an AttributeError.

Edit: I would like to deprecate the AttributeError and, in future, raise a TypeError.

@adrn

adrn commented Apr 11, 2021

Copy link
Copy Markdown
Member

Understood and agree AttributeError is wrong - the thing I'm not clear on is whether it should be a TypeError or a ValueError.

Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError.

@nstarman

nstarman commented Apr 11, 2021

Copy link
Copy Markdown
Member Author

The specific cosmo classes will/should handle TypeError and ValueError for arguments that they can accept. But for ones they cannot...

Screen Shot 2021-04-11 at 12 45 39

@nstarman

nstarman commented Apr 11, 2021

Copy link
Copy Markdown
Member Author

Thanks @adrn for the review! I think I resolved the issues and since you agree about AttributeError, I added a deprecation warning.

adrn
adrn approved these changes Apr 11, 2021
View reviewed changes

@adrn adrn left a comment

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.

Great - I'm happy with this (aside from the optional very minor nitpick :))!

Comment thread astropy/cosmology/core.py Outdated
@adrn

adrn commented Apr 11, 2021

Copy link
Copy Markdown
Member

@embray would you like to take a look at this, as you were involved in the discussion in #10673?

@nstarman nstarman mentioned this pull request Apr 11, 2021
Closed
2 of 7 tasks
@nstarman

nstarman commented Apr 11, 2021

Copy link
Copy Markdown
Member Author

Uh oh! it fails the test because it raises it's own DeprecationWarning? I'm not familiar with Astropy's standard fix!

@adrn

adrn commented Apr 11, 2021

Copy link
Copy Markdown
Member

Ah, right haha. Inside the pytest.raises(), does it work to add a pytest.warns block? For an example:

astropy/astropy/utils/tests/test_decorators.py

Line 72 in aef38c7

with pytest.warns(AstropyDeprecationWarning, match="The foo attribute is "

@nstarman @adrn
Simplify cloning logic
e806f6a 
Store initialization signature and arguments
changelog fragment
deprecation warning for AttributeError over TypeError

Co-authored-by: Adrian Price-Whelan <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@mhvk

mhvk commented Apr 11, 2021

Copy link
Copy Markdown
Contributor

Only had a quick look, but just a general 👍 to the idea!

@adrn

adrn commented Apr 11, 2021

Copy link
Copy Markdown
Member

(Note: test failures are unrelated)

@nstarman nstarman mentioned this pull request Apr 11, 2021
Closed
@adrn
adrn merged commit ea04267 into astropy:main Apr 12, 2021
@adrn

adrn commented Apr 12, 2021

Copy link
Copy Markdown
Member

Thanks @nstarman for this!

@nstarman
nstarman deleted the cosmo_store_init branch April 12, 2021 12:37
@pllim pllim added this to the v5.0 milestone Apr 12, 2021
@pllim

pllim commented Apr 12, 2021

Copy link
Copy Markdown
Member

@adrn , for future reference, please don't forget to milestone. The milestone check is flaky nowadays.

Also, are you sure user doc update isn't needed?

I added a "whatsnew-needed" label for now but feel free to remove if you disagree.

@adrn

adrn commented Apr 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Oh yes, good to know RE: the milestone check - I scanned the checks and hit merge, but will explicitly look for milestone in the future!

No docs needed: this is not a user-facing change.

@saimn

saimn commented Apr 12, 2021

Copy link
Copy Markdown
Contributor

@pllim - why the v5.0 milestone ? If it's merged in main it will be in 4.3.

pllim
pllim reviewed Apr 12, 2021
View reviewed changes
Comment thread astropy/cosmology/core.py
@saimn saimn modified the milestones: v5.0, v4.3 Apr 12, 2021
@nstarman nstarman mentioned this pull request May 23, 2021
Closed
2 of 7 tasks
@mhvk mhvk mentioned this pull request Jun 9, 2021
Closed
5 of 6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn left review comments
@adrn adrn adrn approved these changes
@pllim pllim pllim left review comments

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.

6 participants

@nstarman @pep8speaks @adrn @mhvk @pllim @saimn