DM-37819: Fix crosstalk measurement issues #200
Conversation
| @@ -29,9 +29,10 @@ | |||
| from lsstDebug import getDebugFrame | |||
| from lsst.afw.detection import FootprintSet, Threshold | |||
| from lsst.afw.display import getDisplay | |||
| from lsst.pex.config import Field, ListField | |||
| from lsst.pex.config import Field, ListField, ConfigurableField | |||
There was a problem hiding this comment.
I know this repo doesn't use isort, but alphabetical ordering would be great regardless.
| @@ -579,7 +609,9 @@ def measureCrosstalkCoefficients(self, ratios, ordering, rejIter, rejSigma): | |||
| if ii == jj: | |||
| values = [0.0] | |||
| else: | |||
| values = np.array(ratios[ordering[ii]][ordering[jj]]) | |||
| # ratios is ratios[Target][Source] | |||
| # use jj for Target, use ii for Source, to match ip_isr. | |||
There was a problem hiding this comment.
Slightly beyond the scope, but using indices like source_idx and target_idx, or even something like ss and tt instead of ii and jj could avoid a lot of confusion overall.
There was a problem hiding this comment.
This is such an obvious improvement that simply never occurred to me. I've made this change here and in ip_isr, so this won't confuse people in the future.
| @@ -109,6 +110,10 @@ class CrosstalkExtractConfig(pipeBase.PipelineTaskConfig, | |||
| default=True, | |||
| doc="Is the input exposure trimmed?" | |||
| ) | |||
| background = ConfigurableField( | |||
| target=SubtractBackgroundTask, | |||
| doc="Configuration for initial background estimation", | |||
There was a problem hiding this comment.
This isn't quite a configuration in the sense of a set of parameters. May be a better doc would be "Initial background estimation task"?
| self.log.debug(" Target amplifier: %s", targetAmpName) | ||
|
|
||
| targetAmpImage = CrosstalkCalib.extractAmp(targetIm.image, | ||
| targetAmpImage = CrosstalkCalib.extractAmp(targetIm, |
There was a problem hiding this comment.
I don't see any changes to the extractAmp signature, so I don't follow how it is now able to take targetIm instead of targetIm.image.
There was a problem hiding this comment.
Okay, I see that it can take either an Image instance or a MaskedImage instance and returns an object of the same type(?) I don't see why that change was necessary though.
There was a problem hiding this comment.
I think I wanted to look at the mask planes while I was debugging things.
|
The "Fix flake8 error" should be squashed with "Use better background modelling" commit and the last "Switch to less confusing iterator names" does more than what the commit message says. It should be broken down and the changes related to imports and docstrings should be squashed with appropriate previous commits as well. |
czwa
changed the title
No description provided.