# Fix PhotoCalib defintion to use multiplication

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
4
• Sprint:
• Team:

## 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).

## Activity

Hide
John Parejko added a comment -

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

Show
John Parejko added a comment - Have to do this before I can finish jointcal PhotoCalib persistence.
Hide
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
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?
Hide
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
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
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
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
Paul Price added a comment -

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

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

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

Show
John Parejko added a comment - It looks like updateExposure still uses the old Calib object, so it doesn't need any changes.
Hide
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
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
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
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
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
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).
Hide
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 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
Paul Price added a comment -

Thanks Lauren MacArthur: I really appreciate your checking meas_mosaic.

Show
Paul Price added a comment - Thanks Lauren MacArthur : I really appreciate your checking meas_mosaic.
Hide
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
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 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 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
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
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
John Parejko added a comment -

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

Merged and done!

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

## People

• Assignee:
John Parejko
Reporter:
John Parejko
Reviewers:
John Swinbank, Lauren MacArthur
Watchers:
Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff