Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw, jointcal, meas_mosaic
-
Labels:
-
Story Points:4
-
Epic Link:
-
Sprint:Alert Production F17 - 9, Alert Production F17 - 10
-
Team:Alert Production
Description
After further work on jointcal, I've realized that the basic definition of PhotoCalib should be multiplicative-instFlux * zeroPoint(x,y, instead of the current division-instFlux * zeroPoint(x,y. Fortunately, PhotoCalib isn't really used in the stack yet, so now is the time to fix it. Minor changes will also be required in jointcal and meas_mosaic creation of a PhotoCalib for persistence.
This shouldn't have any impact on the existing API, just on the internal calculations and persistence.
Not RFCing this, as it was defined this way in the original RFC and we changed it during implementation, and all of the current stakeholders have agreed to the change (slack#dm).
Attachments
Issue Links
Activity
Thanks for the review. I've responded to all your comments and pushed an update.
I'm not sure what you want me to get to the bottom of in that test. The relevant change is the added comment on this docstring:
https://github.com/lsst/afw/pull/276/files#diff-544338bf5b8a926b2cd5264959a908e1R107
and the change from self.instFlux0Err to calibrationErr at this call to computeMaggiesErr.
https://github.com/lsst/afw/pull/276/files#diff-544338bf5b8a926b2cd5264959a908e1L139
I discovered the problem when trying to run the updated test and that line was just barely failing when calibrationErr was large compared to calibration (for example 0.001 +/- 0.1), because I hadn't yet sorted out what the correct value for the error term should be in the new system. Changing to use the local variable is the obvious and correct fix. I checked that I didn't make the same mistake anywhere else. It only manifests when the error is unrealistically large.
A few more minor comments on the PR.
As for the test. I totally agree that you are now passing in the correct variable (i.e. the one the test was designed to use). What worries me is that in your commit message you, yourself say you are worried that it passed previously with the WRONG variable being passed in. That implies a flawed test in the first place, so the fact that you are now passing in the correct variable is besides the point. If you can satisfy yourself that the test was not flawed, i.e. can revert your opinion stated in the commit message to "I DO understand why it passed before, this is why, so I'm NOT worried as to the fidelity of the test itself", then all is good.
Thanks for the follow up comments: the doxygen docstrings produce much cleaner docs now.
I've amended the commit message to better reflect what the testing bug was.
I can't merge this until DM-9195 is done (otherwise, jointcal won't work), so this will be in a holding pattern until then.
DM-9195 is finished, so I can merge this without breaking jointcal's master.
Merged and done!
Thanks Lauren MacArthur: I really appreciate your checking meas_mosaic.