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

            Parejkoj John Parejko created issue -
            Parejkoj John Parejko made changes -
            Field Original Value New Value
            Link This issue relates to RFC-289 [ RFC-289 ]
            Hide
            Parejkoj John Parejko added a comment -

            Have to do this before I can finish jointcal PhotoCalib persistence.

            Show
            Parejkoj John Parejko added a comment - Have to do this before I can finish jointcal PhotoCalib persistence.
            Parejkoj John Parejko made changes -
            Assignee John Parejko [ parejkoj ]
            Status To Do [ 10001 ] In Progress [ 3 ]
            Parejkoj John Parejko made changes -
            Link This issue blocks DM-9195 [ DM-9195 ]
            Hide
            Parejkoj John Parejko added a comment -

            John Swinbank Can you please review this change to the definition of afw::image::photoCalib? I think I updated all the necessary docstrings as well, but I'm not sure, so a review of any method docstring with math in it may be worthwhile.

            This change also requires changing jointcal, but I'm going to do that as part of DM-9195 and merge simultaneously with this.

            Jim Bosch This ticket only implements the change in afw, but meas_mosaic's BoundedField will need the change as well. I don't know where that lives: should I change it, or would you like to?

            Show
            Parejkoj John Parejko added a comment - John Swinbank Can you please review this change to the definition of afw::image::photoCalib? I think I updated all the necessary docstrings as well, but I'm not sure, so a review of any method docstring with math in it may be worthwhile. This change also requires changing jointcal, but I'm going to do that as part of DM-9195 and merge simultaneously with this. Jim Bosch This ticket only implements the change in afw, but meas_mosaic's BoundedField will need the change as well. I don't know where that lives: should I change it, or would you like to?
            Parejkoj John Parejko made changes -
            Reviewers Jim Bosch, John Swinbank [ jbosch, swinbank ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            jbosch Jim Bosch added a comment -

            Please do make the change in meas_mosaic, as I'm busy or away most of next week and don't think I'll be abl to get to it (I think you'll only need changes in FluxFitBoundedField.cc and test_FluxFitBoundedField.py).

            Show
            jbosch Jim Bosch added a comment - Please do make the change in meas_mosaic, as I'm busy or away most of next week and don't think I'll be abl to get to it (I think you'll only need changes in FluxFitBoundedField.cc and test_FluxFitBoundedField.py).
            Hide
            Parejkoj John Parejko added a comment -

            John Swinbank meas_mosaic is now updated: here's the PR if Jira doesn't pick it up: https://github.com/lsst/meas_mosaic/pull/22

            Both packages should be ready for review now. One open question I have is whether I got the error conversion correct (I think so, but please check): calibrationErr = instFluxMag0Err / instFluxMag0^2

            afw/meas_mosaic jenkins run ongoing:
            https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/26722/pipeline

            Show
            Parejkoj John Parejko added a comment - John Swinbank meas_mosaic is now updated: here's the PR if Jira doesn't pick it up: https://github.com/lsst/meas_mosaic/pull/22 Both packages should be ready for review now. One open question I have is whether I got the error conversion correct (I think so, but please check): calibrationErr = instFluxMag0Err / instFluxMag0^2 afw/meas_mosaic jenkins run ongoing: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/26722/pipeline
            Hide
            price Paul Price added a comment -

            Are any changes required in meas_mosaic's updateExposure.py?

            Show
            price Paul Price added a comment - Are any changes required in meas_mosaic's updateExposure.py?
            Hide
            Parejkoj John Parejko added a comment -

            It looks like updateExposure still uses the old `Calib` object, so it doesn't need any changes.

            Show
            Parejkoj John Parejko added a comment - It looks like updateExposure still uses the old `Calib` object, so it doesn't need any changes.
            Parejkoj John Parejko made changes -
            Reviewers Jim Bosch, John Swinbank [ jbosch, swinbank ] John Swinbank, Lauren MacArthur [ swinbank, lauren ]
            Hide
            Parejkoj John Parejko added a comment -

            Lauren MacArthur could you please try this with the updated afw (same ticket) and see if it affects anything in HSC land? I don't know that the photoCalib code branch is actually used anywhere post meas_mosaic yet.

            Show
            Parejkoj John Parejko added a comment - Lauren MacArthur could you please try this with the updated afw (same ticket) and see if it affects anything in HSC land? I don't know that the photoCalib code branch is actually used anywhere post meas_mosaic yet.
            Hide
            swinbank John Swinbank added a comment - - edited

            I took a quick look through the changes here, and they seem fine. However, I don't have time — and likely won't have time in the next several weeks — to take more than a superficial once-over. It'd probably be smart to select another reviewer for this — perhaps Paul Price or Lauren MacArthur could help?

            Show
            swinbank John Swinbank added a comment - - edited I took a quick look through the changes here, and they seem fine. However, I don't have time — and likely won't have time in the next several weeks — to take more than a superficial once-over. It'd probably be smart to select another reviewer for this — perhaps Paul Price or Lauren MacArthur could help?
            Hide
            jbosch Jim Bosch added a comment -

            The PhotoCalib code branch in mosaic is not used post meas_mosaic yet. I thought I had at least created an issue to make the coaddition code use these outputs instead of the old ones (so the conversion to jointcal would be seamless), but it looks like I didn't.

            The best way to test right now would be to run meas_mosaic (on RC wide data) with the changes, then use those outputs to replace the files in the meas_mosaic tests directory and see if the tests pass (John Parejko, I know you managed to get the tests running without doing this, but I think following this approach would actually give us more confidence that it's working).

            Show
            jbosch Jim Bosch added a comment - The PhotoCalib code branch in mosaic is not used post meas_mosaic yet. I thought I had at least created an issue to make the coaddition code use these outputs instead of the old ones (so the conversion to jointcal would be seamless), but it looks like I didn't. The best way to test right now would be to run meas_mosaic (on RC wide data) with the changes, then use those outputs to replace the files in the meas_mosaic tests directory and see if the tests pass ( John Parejko , I know you managed to get the tests running without doing this, but I think following this approach would actually give us more confidence that it's working).
            Parejkoj John Parejko made changes -
            Summary Fix PhotoCalib defintion to use multiplciation Fix PhotoCalib defintion to use multiplication
            Hide
            lauren Lauren MacArthur added a comment -

            I have put some comments on the PR.

            I can confirm nothing is affected in the current meas_mosaic land when running with the two new branches on this ticket. I checked both the applyMosaicResultsCatalog & applyMosaicResults functions in updateExposure.py and they produce identical before & after corrected catalog and image results.

            As for Jim Bosch's suggestion above, we agree this should be done, but can happen on its own ticket (i.e. with my confirmation above, it need not block this ticket). Can you please create that ticket?

            Finally, the only reason I'm not yet marking this as reviewed is based on your comment in the commit message:

            Fix bug in test, where it was using self.instFlux0Err instead of the passed
            instFlux0Err (now calibrationErr). How this passed the tests before when the
            photoCalib error was 0, I don't know, and it worries me. But its fixed now!

            I'm not comfortable with this...if the tests shouldn't have passed before then they are faulty and no confidence can be placed on them passing (regardless of them now using the proper variables). Can you please try to get to the bottom of this?

            Show
            lauren Lauren MacArthur added a comment - I have put some comments on the PR. I can confirm nothing is affected in the current meas_mosaic land when running with the two new branches on this ticket. I checked both the applyMosaicResultsCatalog & applyMosaicResults functions in updateExposure.py and they produce identical before & after corrected catalog and image results. As for Jim Bosch 's suggestion above, we agree this should be done, but can happen on its own ticket (i.e. with my confirmation above, it need not block this ticket). Can you please create that ticket? Finally, the only reason I'm not yet marking this as reviewed is based on your comment in the commit message: Fix bug in test, where it was using self.instFlux0Err instead of the passed instFlux0Err (now calibrationErr). How this passed the tests before when the photoCalib error was 0, I don't know, and it worries me. But its fixed now! I'm not comfortable with this...if the tests shouldn't have passed before then they are faulty and no confidence can be placed on them passing (regardless of them now using the proper variables). Can you please try to get to the bottom of this?
            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.
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-12017 [ DM-12017 ]
            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.
            lauren Lauren MacArthur made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production F17 - 9 [ 639 ] Alert Production F17 - 9, Alert Production F17 - 10 [ 639, 643 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            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!
            Parejkoj John Parejko made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            swinbank John Swinbank made changes -
            Epic Link DM-11798 [ 34281 ]

              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