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. 

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.

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?

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?

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

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 )

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?

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.

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.

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

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.

Note my comment about the structure of your tests.

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

Merged