DM-14182: subtract fit from overscan; add option to set variance empirically #53
Conversation
PaulPrice
force-pushed
the
tickets/DM-14182
branch
from
e017d00
to
2b7fdf1
Compare
There was a problem hiding this comment.
This new functionality makes sense and it's good to return the overscanFit as well as the offImage for downstream use. Thanks for doing python 2 support cleanup as well as some numpydoc along the way. The changes are mostly fine, but please see my individual comments.
|
|
||
| The ``ampMaskedImage`` and ``overscanImage`` are modified, with the fit | ||
| subtracted. Note that the ``overscanImage`` should not be a subimage of | ||
| the ``ampMaskedImage``, to avoid being subtracted twice. |
There was a problem hiding this comment.
This is a good point. Is it possible to add a test to check that overscanImage is not a subimage of ampMaskedImage? You say in your commit message that it's not clear how to detect this case, but I think it's rather important if it's doable.
There was a problem hiding this comment.
I don't believe it's possible to test, since we have no way of determining whether two images share the same physical pixels. I therefore consider that the only way to deal with this is to document the possibility.
| doEmpiricalReadNoise = pexConfig.Field( | ||
| dtype=bool, | ||
| default=False, | ||
| doc="Calculate empirical read noise instead of value from AmpInfo data?" |
There was a problem hiding this comment.
Clarify: "instead of using value from amp.getReadNoise?" This also makes me wonder what sets the read noise value in the amplifier info in the first place. Where does it usually come from?
There was a problem hiding this comment.
The read noise is hard-wired into the camera configuration, often in the form of entries in FITS files.
| if self.config.doEmpiricalReadNoise and overscanImage is not None: | ||
| stats = afwMath.StatisticsControl() | ||
| stats.setAndMask(overscanImage.mask.getPlaneBitMask(maskPlanes)) | ||
| readNoise = afwMath.makeStatistics(overscanImage, afwMath.STDEVCLIP, stats).getValue() |
There was a problem hiding this comment.
I'm assuming a plain old .getValue() is adequate here even though earlier uses of makeStatistics to get, e.g., the mean specifies .getValue(afwMath.MEAN).
There was a problem hiding this comment.
getValue() works if you requested only a single statistic: https://github.com/lsst/afw/blob/master/include/lsst/afw/math/Statistics.h#L277-L282
| @@ -887,17 +930,17 @@ def overscanCorrection(self, exposure, amp): | |||
|
|
|||
| maskedImage = exposure.getMaskedImage() | |||
| dataView = maskedImage.Factory(maskedImage, amp.getRawDataBBox()) | |||
| overscanImage = maskedImage.Factory(maskedImage, amp.getRawHorizontalOverscanBBox()) | |||
There was a problem hiding this comment.
Does this assume overscan is always and forever horizontal? I'm thinking of DECam, which has both horizontal and vertical overscan regions (and its own isr.py, of course, so this code doesn't break anything, but I still think it should be as generic as possible).
There was a problem hiding this comment.
My understanding is that the point of the overscanCorrection function is to correct for bias drift as the device is read out. It therefore uses the serial overclock ("horizontal overscan"), because that tracks the bias level as a function of time as the device is read out. I don't believe the parallel overclock ("vertical overscan") is useful for this because it's read out after all the science pixels; to my knowledge we've never used it (for anything). I'm not even sure what use it has unless you have zero bias drift and simply want more pixels to throw into your calculation.
|
|
||
| class EmpiricalVarianceTestCast(lsst.utils.tests.TestCase): | ||
| def setUp(self): | ||
| """Constructs a CCD with two amplifiers and prepares for ISR""" |
There was a problem hiding this comment.
Could you use a real image from some testdata rather than building a fake one here? I'd be more inclined to believe the test result for a real-life exposure than for this minimalistic fake one.
There was a problem hiding this comment.
obs_decam tests include running processCcd.py on some real DECam data, and ci_hsc does similarly with real HSC data. Both ran fine under Jenkins.
I think leaving the tests on real data to other packages is the right balance: it keeps the size of this code repo small, and using simulated data means we can isolate the features being tested, and we know the exact right answer.
tests/test_empiricalVariance.py
Outdated
|
|
||
| def testEmpiricalVariance(self): | ||
| results = self.task.run(self.exposure) | ||
| exposure = results.exposure |
There was a problem hiding this comment.
Mildly confusing variable name (since there is a self.exposure floating around already, but this is an exposure created from the result of processing self.exposure). Perhaps call one raw and one post-ISR.
| @@ -44,6 +44,9 @@ def testOverscanCorrectionY(self, **kwargs): | |||
| maskedImage = afwImage.MaskedImageF(bbox) | |||
| maskedImage.set(10, 0x0, 1) | |||
|
|
|||
| dataBox = afwGeom.Box2I(afwGeom.Point2I(0, 0), afwGeom.Extent2I(10, 10)) | |||
| dataImage = afwImage.MaskedImageF(maskedImage, dataBox) | |||
There was a problem hiding this comment.
Is the point of dataImage just to be a small chunk of maskedImage? Does it save a lot of time to run the overscan correction test on this rather than the entire maskedImage throughout?
There was a problem hiding this comment.
maskedImage includes the overscan, so if I were to operate on that I would be subtracting the overscan twice, which breaks the checks.
PaulPrice
force-pushed
the
tickets/DM-14182
branch
from
2b7fdf1
to
dec64c7
Compare
This allows downstream processing to inspect the fit results. Because the overscan is now subtracted explicitly, this requires that the overscan passed to this function not be contained within the image being corrected (or the correction is applied twice). Unfortunately, it's not clear how to detect this case; however, the primary user is IsrTask, which pulls out the components explicitly and so shouldn't mix them. Since this involved changes to the docstring, reformatted the docstring for this function into numpydoc.
The read noise is measured from the overscan. This might be useful when working on detectors whose read noise hasn't been measured, or drifts due to hardware tests.
PaulPrice
force-pushed
the
tickets/DM-14182
branch
from
dec64c7
to
9379bf4
Compare
No description provided.