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

Zeropoint and zeropoint_airmass references #63

Merged
merged 5 commits into from
Mar 2, 2022

Conversation

mpaillassa
Copy link
Collaborator

This PR aims to update the zeropoint references (especially the speclite ones) and to add the zeropoint_airmass references that were missing.

(I did not update Euclid as it is done in its corresponding PR #49)

@aboucaud
Copy link
Collaborator

aboucaud commented Mar 2, 2022

Currently the zeropoint_airmass is an optional parameter.
I think we should ignore it instead of writing zeropoint_airmass=0.0 for space surveys so we can test for instance :

for surveys in available_surveys:
    if survey.zeropoint_airmass is not None:
        ...

What do you think ?

@ismael-mendoza
Copy link
Contributor

ismael-mendoza commented Mar 2, 2022

I think we should ignore it instead of writing zeropoint_airmass=0.0 for space surveys so we can test for instance :

I'm usually in favor of avoiding if statements and branching in code if possible. Could you tell us a little more of why testing this with an if statement might be useful even if setting zeropoint_airmass = 0.0 for space telescopes makes sense?

@aboucaud
Copy link
Collaborator

aboucaud commented Mar 2, 2022

Thanks for your comment @ismael-mendoza.
After some thoughts I agree with you.
In that specific case, putting 0.0 makes sense and is not just a "fill value".

So since we have values for all surveys I'll make this parameter not optional in an upcoming PR and merge this one.

@aboucaud aboucaud merged commit 0cebb17 into LSSTDESC:main Mar 2, 2022
@mpaillassa mpaillassa deleted the zeropoint_ref branch March 2, 2022 14:45
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.

3 participants