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

Add PhotoCalib.calibrateImage() option to compute variance without calib err term

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      Description

      Currently PhotoCalib.calibrateImage() computes the variance plane as:

      fluxVar = instFlux.variance * scaleVar +  instFlux.variance * scale**2 + instFlux.image**2 * scaleVar
      

      where scale and scaleVar are my own names for the scale factor that the photocalib applies and the measured uncertainty on that photocalib.

      This equation assumes that the scale factor variance and the image variance are independent, and this isn't obviously true.

      Whether this is the correct variance equation is irrelevant, however. It is a change to how the variance plane in the zeropoint-normalized warps is currently computed, and I suspect it is the primary source of the photometry differences we're seeing in coadds produced with jointcal. While we are testing the effects of using jointcal vs. meas_mosaic's calibration on coadd (DM-16596), it is important that we are able to test these two changes (jointcal's calibrations and the variance plane equation change) separately.

      Implementation: This could be done with an extra argument like photoCalib.calibrateImage(Image, addCalibVar=False or with a state switch like Calib's Calib.setThrowOnNegativeFlux(False).

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment - - edited

            I wanted to run some more tests because it didn't make any sense that this ticket could CAUSE an offset by itself. It doesn't change how it handles the image plane. And why didn't we see it before. After some digging, I think this mystery was all my fault.

            (1) I generated https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_44_jointcal with the version of DM-15751 that I reviewed. The problem could either be: (a) This was not the same version that landed on master (I think John made some changes after I tested it) and (b) If you look at it, it looks more like meas_mosaic photo_calibs applied with John's new computeImage method. (The repo has 2 parents which supports this hypothesis).
            (2) The plots I posted in the above comment (with magnitude-dependent stellar size issue resolve + offset) is actually master. (Yep, I had screwed up the versions/configs in that run. I'm sorry). Which means that issue was resolved on DM-15751 before it hit master. For proof, Hsin-Fang's w_2018_48-jointcal run () looks the same as my first attempt to run this branch
            (3) Multiband for my second attempt to run this branch will be done tomorrow morning. But looking at the warps, its a MUCH smaller effect that what we were seeing before.

            Lauren MacArthur, I recommend you pick up the QA on that branch when it lands and Hsin-Fang's w_2018_48-jointcal run

            John Parejko Please forgive me for the mess. I still think the config option on this ticket is important. Your afw branch is good to go. Will you review my branch on pipe_tasks? I go with False I'll need to update the unit test.

            Show
            yusra Yusra AlSayyad added a comment - - edited I wanted to run some more tests because it didn't make any sense that this ticket could CAUSE an offset by itself. It doesn't change how it handles the image plane. And why didn't we see it before. After some digging, I think this mystery was all my fault. (1) I generated https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_44_jointcal with the version of DM-15751 that I reviewed. The problem could either be: (a) This was not the same version that landed on master (I think John made some changes after I tested it) and (b) If you look at it, it looks more like meas_mosaic photo_calibs applied with John's new computeImage method. (The repo has 2 parents which supports this hypothesis). (2) The plots I posted in the above comment (with magnitude-dependent stellar size issue resolve + offset) is actually master. (Yep, I had screwed up the versions/configs in that run. I'm sorry). Which means that issue was resolved on DM-15751 before it hit master. For proof, Hsin-Fang's w_2018_48-jointcal run () looks the same as my first attempt to run this branch (3) Multiband for my second attempt to run this branch will be done tomorrow morning. But looking at the warps, its a MUCH smaller effect that what we were seeing before. Lauren MacArthur , I recommend you pick up the QA on that branch when it lands and Hsin-Fang's w_2018_48-jointcal run John Parejko Please forgive me for the mess. I still think the config option on this ticket is important. Your afw branch is good to go. Will you review my branch on pipe_tasks? I go with False I'll need to update the unit test.
            Hide
            Parejkoj John Parejko added a comment -

            Yusra: your pipe_tasks change looks ok, though you need to rebase it and there wasn't a PR.

            Show
            Parejkoj John Parejko added a comment - Yusra: your pipe_tasks change looks ok, though you need to rebase it and there wasn't a PR.
            Hide
            yusra Yusra AlSayyad added a comment -

            John Parejko: Unit test updated, master rebased and PR opened. I can force push my master rebased afw too and kick off a Jenkins, unless you'd rather do it.

            Show
            yusra Yusra AlSayyad added a comment - John Parejko : Unit test updated, master rebased and PR opened. I can force push my master rebased afw too and kick off a Jenkins, unless you'd rather do it.
            Hide
            Parejkoj John Parejko added a comment -

            Note my comment about the structure of your tests.

            If you would like to shepherd this ticket through the final steps, please do!

            Show
            Parejkoj John Parejko added a comment - Note my comment about the structure of your tests. If you would like to shepherd this ticket through the final steps, please do!
            Hide
            yusra Yusra AlSayyad added a comment -

            Merged

            Show
            yusra Yusra AlSayyad added a comment - Merged

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                yusra Yusra AlSayyad
                Reviewers:
                Yusra AlSayyad
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel