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

tiickets/OPSIM-1193: add SURVEY_START_MJD and DEFAULT_NSIDE to replace functions #108

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

yoachim
Copy link
Member

@yoachim yoachim commented Sep 18, 2024

Introduce SURVEY_START_MJD and DEFAULT_NSIDE to replace survey_start_mjd and set_default_nside.

Copy link
Collaborator

@ehneilsen ehneilsen left a comment

Choose a reason for hiding this comment

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

Looks good, but there's one gotcha you should be aware of. I see you removed set_default_nside, presumably because one can now just assign DEFAULT_NSIDE to whatever they want. I think this will work just as well as using set_default_nside, but there are a number of places one would expect either to work when neither will. In particulary, if the call signature of a function explicitly references DEFAULT_NSIDE, e.g.
def my_func(nside=DEFAULT_NSIDE):
This default value will not get updated; DEFAULT_NSIDE will get refenced at the time of import, and never again. If you really want it to get appropriately updated, you need to do something along the lines of:

def my_func(nside=None):
    if nside is None:
        nside = DEFAULT_NSIDE

(The same applies to the start date, but there never was a way to change it anyway.)
I don't think the changes in this PR actually make anything worse than it was before, but it is an issue we should keep in mind.

@yoachim
Copy link
Member Author

yoachim commented Sep 18, 2024

I see, you're saying there's no way for the user to programmatically change DEFAULT_NSIDE and SURVEY_START_MJD. I think that's fine, if you want to run a different nside it's just like it's always been and you have to make sure to pass that new kwarg around to everything that needs it.

@yoachim yoachim merged commit afdabda into main Sep 18, 2024
7 checks passed
@rhiannonlynne rhiannonlynne deleted the tickets/OPSIM-1193 branch September 18, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants