DM-23396: Function "overscanCorrection" in "isrFunctions.py" needs refactoring #127
Conversation
python/lsst/ip/isr/overscan.py
Outdated
| import lsst.afw.image as afwImage | ||
|
|
||
|
|
||
| class OverscanCorrector(): |
There was a problem hiding this comment.
I think this should be a Task, as that's the standard unit of code reuse. It would also give you access to a logger and configuration parameters. And otherwise, just about all the methods below should be @staticmethod: self is unused.
python/lsst/ip/isr/isrFunctions.py
Outdated
| if fitType in ('MEAN', 'MEANCLIP', 'MEDIAN'): | ||
| overscanValue, maskArray, isTransposed = corrector.overscanConstant(overscanImage) | ||
| elif fitType in ('MEDIAN_PER_ROW', 'POLY', 'CHEB', 'LEG', | ||
| 'NATURAL_SPLINE', 'CUBIC_SPLINE', 'AKIMA_SPLINE'): | ||
| overscanValue, maskArray, isTransposed = corrector.overscanVector(overscanImage, order) | ||
| else: | ||
| raise pexExcept.Exception('%s : %s an invalid overscan type' % ("overscanCorrection", fitType)) |
There was a problem hiding this comment.
Move this logic into the OverscanCorrectionTask. That's the thing that knows what it can do, so it should decide how to do it.
python/lsst/ip/isr/isrFunctions.py
Outdated
|
|
||
| if isinstance(overscanValue, float): |
There was a problem hiding this comment.
I'd like to see this logic in the OverscanCorrectionTask too. It knows best what the return types are, so it knows best how to apply them.
python/lsst/ip/isr/overscan.py
Outdated
| statControl : `lsst.afw.math.StatisticsControl`, optional | ||
| Statistics control object. | ||
| sigma : `float`, optional | ||
| Number of sigma to use for sigma clipping. |
There was a problem hiding this comment.
fitType and sigma should go in OverscanCorrectionConfig.
python/lsst/ip/isr/overscan.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| image : `numpy.ndarray`, `lsst.afw.image.Image` or `lsst.afw.image.MaskedImage` |
There was a problem hiding this comment.
I suspect this goes over 80 chars. Is that allowed? If not, how do we deal with it?
There was a problem hiding this comment.
I could not find any details, and have abbreviated types to fit.
czwa
force-pushed
the
tickets/DM-23396
branch
4 times, most recently
from
5015951
to
477b450
Compare
| config.overscanIsInt = True | ||
|
|
||
| overscanTask = OverscanCorrectionTask(config=config) | ||
| return overscanTask.run(ampImage, overscanImage) |
There was a problem hiding this comment.
You should deprecate this function, and modify IsrTask to call the OverscanCorrectionTask directly. Otherwise, there's not really any advantage to having the overscan correction in a Task.
There was a problem hiding this comment.
The newest commit does this, although it still calls it from an IsrTask.overscanCorrection call. The overscan chopping is still in IsrTask, allowing OverscanCorrectionTask to concentrate on the image-level math.
This has necessitated config changes on obs_ packages to point to the updated overscan config item.
| "CUBIC_SPLINE": "Fit cubic spline to the longest axis of the overscan region", | ||
| "AKIMA_SPLINE": "Fit Akima spline to the longest axis of the overscan region", | ||
| "MEAN": "Correct using the mean of the overscan region", | ||
| "MEANCLIP": "Correct using a clipped mean of the overscan region", |
There was a problem hiding this comment.
Where are the clipping parameters set?
There was a problem hiding this comment.
OverscanCorrectionTaskConfig.numSigmaClip controls the clipping.
czwa
force-pushed
the
tickets/DM-23396
branch
2 times, most recently
from
77bca0f
to
c5e6fc9
Compare
python/lsst/ip/isr/overscan.py
Outdated
| Value or fit subtracted from the amplifier image data. | ||
| - ``overscanFit`` : scalar or `lsst.afw.image.Image` | ||
| Value or fit subtracted from the overscan image data. | ||
| - ``overscanImage`` : `lsst.afw.image.Image` |
There was a problem hiding this comment.
Why return this? The input overscanImage should be modified, so the user has this already.
There was a problem hiding this comment.
This is used (optionally) in the empirical variance calculation. That happens after the trim and assembly, and I suspect saving that image segment as part of the overscan results struct was chosen instead of deciding whether to save it during the trim step.
python/lsst/ip/isr/overscan.py
Outdated
| @@ -131,6 +152,9 @@ def run(self, ampImage, overscanImage): | |||
| overscanArray[:, :] = overscanValue[:, np.newaxis] | |||
| if maskSuspect: | |||
| maskSuspect.getArray()[maskArray, :] |= ampImage.getMask().getPlaneBitMask("SUSPECT") | |||
| else: | |||
| raise pexExcept.Exception('%s : %s an invalid overscan type' % | |||
There was a problem hiding this comment.
Why this particular exception type, rather than something more specific?
There was a problem hiding this comment.
Also in python we really want to raise python exceptions not pex_exception exceptions. Those exist so that C++ code can raise python-style exceptions.
There was a problem hiding this comment.
Changed to RuntimeError.
tests/test_overscanCorrection.py
Outdated
| if 'fitType' in kwargs: | ||
| config.overscan.fitType = kwargs['fitType'] | ||
| if 'order' in kwargs: | ||
| config.overscan.order = kwargs['order'] |
There was a problem hiding this comment.
I think you should be more defensive about kwargs contents only matching the things you're looking for.
Here and in other places below (factor this into a function).
This split is necessary to reduce the confusion in overscanCorrection, reduce the duplication, and limit the number of nested if/else statements used. The functionality should be identical, just in better named and more reusable components.
Split isrFunctions.overscanCorrection to new class.
This split is necessary to reduce the confusion in overscanCorrection,
reduce the duplication, and limit the number of nested if/else
statements used. The functionality should be identical, just in
better named and more reusable components.