DM-46141: Use ScienceSourceSelector to select kernel candidates #340
Conversation
| def _setup_subtraction(self, doSubtractBackground=False, **kwargs): | ||
| """Setup and configure the image subtraction PipelineTask. | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
Missing doSubtractBackground parameter in the docs.
Is this EVER set to True? Previously it was manually set to False in a handful of tests only, but now that it's the default, I'm not seeing any instances of True, which seems odd and implies we may be missing test coverage in that scenario.
There was a problem hiding this comment.
doSubtractBackground = True is set in test_background_subtraction, but that test has to set configs for subtasks which can't be handled by _setup_subtraction. I will remove the keyword since it isn't ever used.
tests/test_subtractTask.py
Outdated
| @@ -1057,8 +1033,7 @@ def test_clear_template_mask(self): | |||
| templateBorderSize=20, doApplyCalibration=True, | |||
| xSize=xSize, ySize=ySize) | |||
| diffimEmptyMaskPlanes = ["DETECTED", "DETECTED_NEGATIVE"] | |||
| config = subtractImages.AlardLuptonPreconvolveSubtractTask.ConfigClass() | |||
| config.doSubtractBackground = False # Ensure that each each mask plane is set for some pixels | |||
| task = self._setup_subtraction() # Ensure that each each mask plane is set for some pixels | |||
There was a problem hiding this comment.
This in-line comment doesn't really make sense with the task instantiation now that doSubtractBackground defaults to false across the board
| @@ -206,6 +210,7 @@ class AlardLuptonSubtractBaseConfig(lsst.pex.config.Config): | |||
| "base_PixelFlags_flag_saturated", | |||
| "base_PixelFlags_flag_bad", | |||
| ), | |||
| deprecated="This field is no longer used and will be removed after v28.", | |||
There was a problem hiding this comment.
We should use excludeMaskPlanes instead, right? Maybe mention this for clarity
There was a problem hiding this comment.
No, you should now set the config for the sourceSelector subtask instead. Except we should not typically do that, and should use the defaults.
There was a problem hiding this comment.
Ah OK, maybe say that here, then, in so many words, if feasible 😄
| nInitialSelected = np.count_nonzero(selected) | ||
| selected *= self._checkMask(mask, sources, self.config.excludeMaskPlanes) | ||
| nSelected = np.count_nonzero(selected) | ||
| self.log.info("%i candidate sources rejected from template mask", nInitialSelected - nSelected) |
There was a problem hiding this comment.
Instead of "rejected from template mask", perhaps "rejected due to regions in template with one or more excludeMaskPlanes" ?
| selected *= self._checkMask(mask, sources, self.config.excludeMaskPlanes) | ||
| nSelected = np.count_nonzero(selected) | ||
| self.log.info("%i candidate sources rejected from template mask", nInitialSelected - nSelected) | ||
| selectSources = sources[selected].copy(deep=True) | ||
| if (len(selectSources) > self.config.maxKernelSources) & (self.config.maxKernelSources > 0): |
There was a problem hiding this comment.
Consider adding a one-line comment along the lines of # Trim selectSources so it doesn't exceed maxKernalSources. I presume selectSources is not ordered in any meaningful way? So we won't, for example, throw out all the sources in one region of the image, or those with certain S/N or brightness properties?
Exclude sources from being used for PSF matching if any of the 3x3 central pixels are masked.
isullivan
force-pushed
the
tickets/DM-46141
branch
from
fcd1c07 to
0b9d46f
Compare
This PR also makes a few other changes:
subtractImagesunit tests to configure the Task used in the tests in a common method.