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

DM-47187: Update from synthLSST to real ComCam color terms. #23

Merged
merged 34 commits into from
Jan 7, 2025

Conversation

erykoff
Copy link
Collaborator

@erykoff erykoff commented Dec 18, 2024

No description provided.

These bits are apparently untested which is ... not great.
Yes, the Monster is self-referential.  No, I don't know how to avoid that.
This uses a special loader to get embargoed data and correct the
absolute calibration.
Copy link
Collaborator

@psferguson psferguson left a comment

Choose a reason for hiding this comment

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

This looks good, I left a few minor comments (and confirmed that the unit tests pass)

return 7
elif band in ["u"]:
return 5

Copy link
Collaborator

Choose a reason for hiding this comment

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

should this throw an error if given an unknown band instead of returning 10?

print("C2602 (ComCam)")
print("Band CalSpec Original Corrected")
for i, band in enumerate(bands):
print(f"{band} "
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this ever end up double printing this information with

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the set of lines when reading in a catalog, and then there is a second pass when doing the spline terms to make sure everything is still normalized (the second pass doesn't change things by more than a couple mmag at most fwiw).

@@ -303,6 +333,46 @@ def measure_spline_fit(
mag_spline_values=mag_spline_values,
)

if self.do_check_c26202_absolute_calibration:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would consider moving the code in this if statement to a method since it is comcam observations of c26202 specific and makes the measure_spline_fit method a little less readable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I didn't want to move it inside a method since it requires so many dang variables that are part of the main routine. However, I did refactor things a bit to make it more generic and in the future if we have a different calspec absolute calibrator I think it'll be easy to plug it in.

x=f_nu*throughputs[band]["throughput"]/tput_lambda,
y=tput_lambda
y=f_nu*throughputs[band]["throughput"]/tput_lambda,
x=tput_lambda,
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for fixing this!

@erykoff erykoff merged commit b1b1a02 into main Jan 7, 2025
4 checks passed
@erykoff erykoff deleted the tickets/DM-47187 branch January 7, 2025 23: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.

2 participants