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-16650: Update to nanojansky photocalib definition #5
Conversation
@@ -737,7 +738,7 @@ def _getChebyshevPhotoCalib(self, coefficients, err, xyMax, offset, scaling): | |||
# Take the zeropoint, apply the absolute relative calibration offset, | |||
# and whatever flat-field scaling was applied | |||
pars[:, :] = (coefficients.reshape(orderPlus1, orderPlus1) * | |||
10.**(offset / (-2.5)) * scaling) | |||
(offset*units.ABmag).to_value(units.nJy) * scaling) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The offset here is delta-mag, not ABmag. So an offset of say delta_mag = 0.1 should be applying a factor of 0.91 (original code), and now will apply an offset of 3311311214825.901.
Basically, the key is that the code will match whatever the units of the reference catalog are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I finally figured out for myself what's going on here, and it's correct, but it works because the units of the original returned coefficients from fgcm
. I'll have to document that better on the fgcm
side, or change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods like this are perfect use cases for simple unittests. I can help you write one, if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is DM-16525
@@ -771,7 +772,7 @@ def _getConstantPhotoCalib(self, zeropoint, err, offset, scaling): | |||
# Take the zeropoint, apply the absolute relative calibration offset, | |||
# and whatever flat-field scaling was applied | |||
|
|||
calibMean = 10.**(zeropoint / (-2.5)) * 10.**(offset / (-2.5)) * scaling | |||
calibMean = (offset*units.ABmag).to_value(units.nJy) * scaling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this it turns out is untested, because I only run through the "variable calibration" part of the code (because that's going to be the standard way of running it). I will have to think of a way of testing this branch on its own without taking too long. In any event, the line should be:
calibMean = ((zeropoint + offset)*units.ABmag).to_value(units.nJy) * scaling
c15572c
to
b326e26
Compare
b326e26
to
e78e4f3
Compare
No description provided.