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

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

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • 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

            Parejkoj John Parejko added a comment -

            Definitely no state switches (PhotoCalib is immutable anyway). An argument to calibrateImage() yes; maybe includeScaleUncertainty. I would advocate it default to True, but I'm not positive.

            Parejkoj John Parejko added a comment - Definitely no state switches (PhotoCalib is immutable anyway). An argument to calibrateImage() yes; maybe includeScaleUncertainty . I would advocate it default to True, but I'm not positive.

            Defaulting to True would be fine for now. 

            yusra Yusra AlSayyad added a comment - Defaulting to True would be fine for now. 
            Parejkoj John Parejko added a comment -

            I'm going to see if I can do this quickly today.

            I think we will need to have a conversation about what the correct way to handle the calibration uncertainty is.

            Parejkoj John Parejko added a comment - I'm going to see if I can do this quickly today. I think we will need to have a conversation about what the correct way to handle the calibration uncertainty is.
            Parejkoj John Parejko added a comment -

            I believe I have implemented what you wanted here, but please check both the math and the output to see if they line up with what you expected.

            I'm still quite ambivalent about it: although the calibration uncertainty and image plane variance are correlated, I don't think the situation is any worse than it is when we incorporate the calibration uncertainty into the flux error on a particular source (given that that sources may have been used to calculate the calibration uncertainty in the first place).

            Parejkoj John Parejko added a comment - I believe I have implemented what you wanted here, but please check both the math and the output to see if they line up with what you expected. I'm still quite ambivalent about it: although the calibration uncertainty and image plane variance are correlated, I don't think the situation is any worse than it is when we incorporate the calibration uncertainty into the flux error on a particular source (given that that sources may have been used to calculate the calibration uncertainty in the first place).

            Did you want to put the makeCoaddTempExp config parameter for this switch on this ticket, or should I put it on another ticket?

            yusra Yusra AlSayyad added a comment - Did you want to put the makeCoaddTempExp config parameter for this switch on this ticket, or should I put it on another ticket?
            Parejkoj John Parejko added a comment -

            I wasn't sure how you wanted to handle it: the ticket only referred to modifying photoCalib. I guess it could go in this ticket though? Can you make the modification to pipeTasks?

            Parejkoj John Parejko added a comment - I wasn't sure how you wanted to handle it: the ticket only referred to modifying photoCalib. I guess it could go in this ticket though? Can you make the modification to pipeTasks?
            yusra Yusra AlSayyad added a comment - I tried out your ticket with includeScaleUncertainty=False. Good news is it resolves the magnitude-dependent stellar size issue: New jointcal with option on this ticket: https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_48_jointcal_noScaleVar/HSC-Y/tract-9697/plot-t9697-HSC-Y-matches_mag_calib_psf_used-psfMagHist.png and https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_48_jointcal_noScaleVar/HSC-Y/tract-9697/plot-t9697-HSC-Y-mag_modelfit_CModel_unforced-psfMagHist.png Old jointcal with problem: https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_44_jointcal/HSC-Y/tract-9697/plot-t9697-HSC-Y-matches_mag_calib_psf_used-psfMagHist.png https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_44_jointcal/HSC-Y/tract-9697/plot-t9697-HSC-Y-mag_modelfit_CModel_unforced-psfMagHist.png meas_mosaic: https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_48/HSC-Y/tract-9697/plot-t9697-HSC-Y-matches_mag_calib_psf_used-psfMagHist.png Bad news is that there's now an offset (more noticeable in other bands): https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_48_jointcal_noScaleVar/HSC-Z/tract-9697/plot-t9697-HSC-Z-matches_mag_calib_psf_used-psfMagHist.png Colors are offset now: https://lsst-web.ncsa.illinois.edu/~yusra/RC_QA/w_2018_48_jointcal_noScaleVar/color/tract-9697/plot-t9697-griPSF-wFit-fit.png
            lauren Lauren MacArthur added a comment - - edited

            I’m not sure how this would’ve worked with/been affected by the old variance scaling and not the new, but it looks like jointcal is not yet applying colorterms to the reference catalog fluxes: https://github.com/lsst/jointcal/blob/master/python/lsst/jointcal/jointcal.py#L564
            (in case it’s helpful, meas_mosaic does it here: https://github.com/lsst/meas_mosaic/blob/master/python/lsst/meas/mosaic/mosaicTask.py#L380-L409 )

            lauren Lauren MacArthur added a comment - - edited I’m not sure how this would’ve worked with/been affected by the old variance scaling and not the new, but it looks like jointcal is not yet applying colorterms to the reference catalog fluxes: https://github.com/lsst/jointcal/blob/master/python/lsst/jointcal/jointcal.py#L564 (in case it’s helpful, meas_mosaic does it here: https://github.com/lsst/meas_mosaic/blob/master/python/lsst/meas/mosaic/mosaicTask.py#L380-L409 )
            Parejkoj John Parejko added a comment -

            It's not clear to me from the above discussion whether the above changes are ok to merge or not. There were no comments on github, and this is still "In Review". The discussion in DM-16596 suggests that we want this now?

            Parejkoj John Parejko added a comment - It's not clear to me from the above discussion whether the above changes are ok to merge or not. There were no comments on github, and this is still "In Review". The discussion in DM-16596 suggests that we want this now?
            jbosch Jim Bosch added a comment -

            Yes, I think this is a clear improvement even if it's not obviously everything we want to do.  Should make sure there's a follow-up ticket.

            jbosch Jim Bosch added a comment - Yes, I think this is a clear improvement even if it's not obviously everything we want to do.  Should make sure there's a follow-up ticket.
            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, I recommend you pick up the QA on that branch when it lands and Hsin-Fang's w_2018_48-jointcal run

            Parejkoj 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.

            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 , I recommend you pick up the QA on that branch when it lands and Hsin-Fang's w_2018_48-jointcal run Parejkoj 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.

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

            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.

            Parejkoj: 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.

            yusra Yusra AlSayyad added a comment - Parejkoj : 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.

            Note my comment about the structure of your tests.

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

            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!

            Merged

            yusra Yusra AlSayyad added a comment - Merged

            People

              Parejkoj John Parejko
              yusra Yusra AlSayyad
              Yusra AlSayyad
              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:

                Jenkins

                  No builds found.