Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-11905

Fix PhotoCalib defintion to use multiplication

    Details

    • Story Points:
      4
    • 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

            Hide
            price Paul Price added a comment -

            Thanks Lauren MacArthur: I really appreciate your checking meas_mosaic.

            Show
            price Paul Price added a comment - Thanks Lauren MacArthur : I really appreciate your checking meas_mosaic.
            Hide
            Parejkoj John Parejko added a comment -

            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.

            Show
            Parejkoj John Parejko added a comment - 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.
            Hide
            lauren Lauren MacArthur added a comment -

            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.

            Show
            lauren Lauren MacArthur added a comment - 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.
            Hide
            Parejkoj John Parejko added a comment -

            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.

            Show
            Parejkoj John Parejko added a comment - 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.
            Hide
            Parejkoj John Parejko added a comment -

            DM-9195 is finished, so I can merge this without breaking jointcal's master.

            Merged and done!

            Show
            Parejkoj John Parejko added a comment - DM-9195 is finished, so I can merge this without breaking jointcal's master. Merged and done!

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                John Swinbank, Lauren MacArthur
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel