DM-46720: Add uncalibrateImage method to afw.image.PhotoCalib #748
Conversation
parejkoj
force-pushed
the
tickets/DM-46720
branch
3 times, most recently
from
729c6ff to
0aeacf3
Compare
src/image/PhotoCalib.cc
Outdated
| auto eigenFluxVar = ndarray::asEigen<Eigen::ArrayXpr>(fluxVar); | ||
| auto eigenInstFlux = ndarray::asEigen<Eigen::ArrayXpr>(instFlux); | ||
| auto eigenOut = ndarray::asEigen<Eigen::ArrayXpr>(out); | ||
| eigenOut = (eigenFluxVar / eigenFlux.square() - (scaleErr / eigenFlux * eigenInstFlux).square()) * |
There was a problem hiding this comment.
| eigenOut = (eigenFluxVar / eigenFlux.square() - (scaleErr / eigenFlux * eigenInstFlux).square()) * | |
| eigenOut = (eigenFluxVar / eigenFlux.square() - (scaleErr / (eigenFlux * eigenInstFlux).square())) * |
I don't remember the C++ precedence rules well enough to know if what you had was already correct, but this ensures it.
There was a problem hiding this comment.
Uh oh: the above is the exact expression used in the ADU->nJy version:
eigenOut = eigenFlux.square() *
(eigenInstFluxVar / eigenInstFlux.square() + (scaleErr / eigenFlux * eigenInstFlux).square());
Have we been doing that incorrectly the whole time? Because your change here does switch the precedence. I've never been sure whether that calibration error is a variance or not. In jointcal just copied it over from PhotoCalTask: https://github.com/lsst/pipe_tasks/blob/main/python/lsst/pipe/tasks/photoCal.py#L418
There was a problem hiding this comment.
Oh, wait, I read that incorrectly. Your original version was correct (but not obviously so), and I was trying to write it as
eigenOut = (eigenFluxVar / eigenFlux.square() - (scaleErr / (eigenFlux / eigenInstFlux)).square()) *
(note the divide rather than multiply inside the new parentheses).
So we've got a strange test tolerance to explain.
There was a problem hiding this comment.
Oh, and I apparently put the closing parentheses in the wrong place in my original suggestion, too (now fixed in the post above). Here's a fully corrected suggestion:
| eigenOut = (eigenFluxVar / eigenFlux.square() - (scaleErr / eigenFlux * eigenInstFlux).square()) * | |
| eigenOut = (eigenFluxVar / eigenFlux.square() - (scaleErr / (eigenFlux / eigenInstFlux)).square()) * |
There was a problem hiding this comment.
Hmmm... I believe the original expression evaluates to (scaleErr / eigenFlux) * eigenInstFlux, because / and * have the same precedence, and are evaluated left-to-right, not the version that you wrote above.
EDIT: Ugh, sorry, yes, I think what you wrote is the same, dang it.
There was a problem hiding this comment.
But unfortunately, changing to make that flux/instFlux division explicit didn't fix the numerical problem.
There was a problem hiding this comment.
The way I think about it - which may be worth writing out more explicitly via another temporary variable for scale - is
scale = flux / instFlux
instFluxVar = instFlux**2 * ((fluxVar / flux**2) - (scaleVar / scale**2))
There was a problem hiding this comment.
Would it be possible to modify the numbers going into the test make sure we're not adding/subtracting a big number to/from a small number here?
There was a problem hiding this comment.
As we discussed on slack, I think we both now agree that the PhotoCalib.calibrationErr is supposed to be sigma (that's what comes out of PhotoCalTask), although my docstring is not really specific enough, sadly.
tests/test_photoCalib.py
Outdated
| uncalibrated = photoCalib.uncalibrateImage(result) | ||
| # High rtol because our specific choice of calib values results in a | ||
| # very-close-to-but-not-1 value, which results in poor round-tripping. | ||
| self.assertMaskedImagesAlmostEqual(maskedImage, uncalibrated, rtol=4e-2) |
There was a problem hiding this comment.
I'm hoping those parentheses in the calculation were important and you can shrink this after you add them, because that is indeed an extremely high tolerance.
This test should have always used the subimage, not the full image.
This doesn't change the math, just makes it more obvious what the calculation is.
parejkoj
force-pushed
the
tickets/DM-46720
branch
from
0aeacf3 to
26b2174
Compare
The old values caused high numerical error in some image-related calculations; these reduce that error, which I think is inherent to the math (small - big number and float uncertainty squared problems).
parejkoj
force-pushed
the
tickets/DM-46720
branch
from
26b2174 to
f753d4d
Compare
|
@TallJimbo : please have another look. I changed the test values and that lets the various calculations match within typical uncertainty. There's a "small number minus big number" problem that will always be there for some input values. |
|
Looks good! |
No description provided.