Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Cosmology name when parameters change in clone #11536

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

nstarman
Copy link
Member

Description

Cloning a cosmology with changed parameters now appends "(modified)" to the
new instance's name, unless a name is explicitly passed to clone.

The problem can be seen when cloning a built-in cosmology and changing a parameter — eg Planck18.clone(H0=10). This should NOT have the same name because it is not the same thing.

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@nstarman nstarman changed the title Change name when parameters change Change Cosmology name when parameters change Apr 12, 2021
@nstarman nstarman changed the title Change Cosmology name when parameters change Change Cosmology name when parameters change in clone Apr 12, 2021
@nstarman nstarman changed the title Change Cosmology name when parameters change in clone Change Cosmology name when parameters change in clone Apr 12, 2021
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@pllim pllim added this to the v4.3 milestone Apr 12, 2021
@@ -157,6 +157,11 @@ def clone(self, **kwargs):
if len(kwargs) == 0:
return self

# There are changed parameter values.
# The name needs to be changed accordingly, if it wasn't already.
kwargs.setdefault("name", (self.name + " (modified)"
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why you use "setdefault" here since the return value if the function is not used.

Suggested change
kwargs.setdefault("name", (self.name + " (modified)"
kwargs['name'] = None if self.name is None else self.name + " (modified)"

Copy link
Member

Choose a reason for hiding this comment

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

Note that this suggestion can't be applied directly, since the line below will also have to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used setdefault because it only sets the value if one is not already present. The suggested code doesn't quite work because it doesn't check if kwargs already has "name", in which case it should not be changed. An alternative one-liner is
kwargs["name"] = kwargs.get("name", self.name + " (modified)" if self.name is not None else None).

Copy link
Member Author

@nstarman nstarman Apr 13, 2021

Choose a reason for hiding this comment

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

It was mostly just a lazy way of skipping an if-block to make getting that 100% codecov easier. This doesn't need multiple test cases, unlike the equivalent code.

if "name" not in kwargs:
    kwargs['name'] = None if self.name is None else self.name + " (modified)"

Copy link
Member

Choose a reason for hiding this comment

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

Great, thank you for the explanation. I did not know about "setdefault" before, I will definitely use that more often in the future myself!

@hamogu hamogu merged commit 92e4bcf into astropy:main Apr 13, 2021
@nstarman nstarman deleted the cosmo_clone_name branch April 13, 2021 13:26
@nstarman
Copy link
Member Author

Thanks @hamogu for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants