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-23526: Fix fgcmcal issues exposed during PDR2 run #28
Conversation
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.
LGTM, but please consider the following guidelines.
À la: https://developer.lsst.io/work/flow.html#review-preparation
The pull request’s name should be formatted as
DM-NNNN: {{JIRA Ticket Title}}
This helps you and other developers find the right pull request when browsing repositories on GitHub.
And https://developer.lsst.io/work/flow.html#git-commit-message-best-practices:
Writing commit summary lines
Messages start with a single-line summary of 50 characters or less. Consider 50 characters as a hard limit; your summary will be truncated in the GitHub UI otherwise.
@@ -825,7 +825,7 @@ def _outputZeropoints(self, butler, zptCat, visitCat, offsets, tract=None): | |||
|
|||
# The "mean" calibration will be set to the center of the ccd for reference | |||
calibCenter = fgcmField.evaluate(fgcmField.getBBox().getCenter()) | |||
calibErr = (np.log(10.0) / 2.5) * calibCenter * rec['fgcmZptErr'] | |||
calibErr = (np.log(10.0) / 2.5) * calibCenter * np.sqrt(rec['fgcmZptVar']) |
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.
Another gentle reminder of https://developer.lsst.io/python/style.html#binary-operators-should-be-surrounded-by-a-single-space-except-for (although I think there’s much inconsistency even within this file, so do as you wish... 😉)
tests/fgcmcalTestBase.py
Outdated
# And the photoCal error is just the zeropoint gray error | ||
self.assertFloatsAlmostEqual(testCal.getCalibrationErr(), | ||
(np.log(10.0) / 2.5) * testCal.getCalibrationMean() * | ||
fgcmZptGrayErr) |
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.
Ditto, and (fyi, but I don’t insist you do it now!):
https://community.lsst.org/t/the-rules-for-breaking-long-python-lines-is-changing/4005
https://developer.lsst.io/python/style.html#wrap-lines-before-binary-operators
Previously, the errors included an estimate of the global calibration error. The code that uses the photoCalib error expects this to be simply the local error in the "gray" correction.
c83b80a
to
c09e0eb
Compare
This PR includes a few changes required to get the PDR2 reprocessing through.