Tickets/dm 15203 #63
Tickets/dm 15203 #63
Conversation
| @@ -172,7 +173,7 @@ class IsrTaskConfig(pexConfig.Config): | |||
| "or number of spline knots if overscan fit type is a spline."), | |||
| default=1, | |||
| ) | |||
| overscanRej = pexConfig.Field( | |||
| overscanNumSigmaClip = pexConfig.Field( | |||
There was a problem hiding this comment.
I think this change and the addition of MEANCLIP should be in their own commits, not hidden in a "modernisation" commit.
There was a problem hiding this comment.
I'll restructure the commits.
| @@ -937,7 +939,7 @@ def overscanCorrection(self, exposure, amp): | |||
| overscanImage=overscanImage, | |||
| fitType=self.config.overscanFitType, | |||
| order=self.config.overscanOrder, | |||
| collapseRej=self.config.overscanRej, | |||
| statControl=sctrl, | |||
There was a problem hiding this comment.
I think this change belongs in the next commit.
| @@ -934,6 +934,9 @@ def overscanCorrection(self, exposure, amp): | |||
| dataView = maskedImage.Factory(maskedImage, amp.getRawDataBBox()) | |||
| overscanImage = maskedImage.Factory(maskedImage, amp.getRawHorizontalOverscanBBox()) | |||
|
|
|||
| sctrl = afwMath.StatisticsControl() | |||
| sctrl.setNumSigmaClip(self.config.overscanNumSigmaClip) | |||
There was a problem hiding this comment.
It's not clear to me why passing a StatisticsControl object into overscanCorrection is better than passing in a rejection threshold: it doesn't add any information (or are you thinking to expand its use?), so the rejection threshold is more fundamental. It's like passing a Struct consisting of a single value.
Convince me?
There was a problem hiding this comment.
I think that the API in isrFunctions should support the StatisticsControl as the user can set other options (e.g. masks to ignore); indeed, we could then add them to IsrTask without modifying the API
python/lsst/ip/isr/isrFunctions.py
Outdated
| if fitType in ('MEDIAN',): | ||
| # we need an image with integer pixels to handle ties properly | ||
| if hasattr(overscanImage, "image"): | ||
| imageI = overscanImage.image.convertI() |
There was a problem hiding this comment.
Is it worth including a check that the data is indeed integer that has been converted to floating-point? Might someone want to run an overscan correction on an image that has floating-point values (maybe some other operation has been performed before the overscan correction)?
There was a problem hiding this comment.
I'll add an argument (default True) to allow the conversion to int. But I don't think anyone would/should ever want to run overscan estimation on processed data.
RobertLuptonTheGood
force-pushed
the
tickets/DM-15203
branch
2 times, most recently
from
16ff381
to
b19eb0b
Compare
RobertLuptonTheGood
force-pushed
the
tickets/DM-15203
branch
from
b19eb0b
to
15f97b2
Compare
N.b. renamed overscanRej config parameter to overscanNumSigmaClip; a better name and used for MEANCLIP too
Even if the overscan image is floating, it's really ints with lots of ties which must be handled carefully to get sub-integer precision. You should never estimate overscan on "real" floating images, but there's an option to prevent the conversion to int in case you do
RobertLuptonTheGood
force-pushed
the
tickets/DM-15203
branch
from
15f97b2
to
cd36fdf
Compare
No description provided.