DM-34909: Add preconvolution option for image differencing #238
Conversation
isullivan
force-pushed
the
tickets/DM-34909
branch
2 times, most recently
from
ef77dbb
to
ecc3c2c
Compare
There was a problem hiding this comment.
A lot of docstring-related comments, and several places where I think you can consolidate to reduce code duplication.
I am concerned about the clip-to-bbox change that is part of this: are we sure that that won't break anything downstream that is expecting the calexp and template/difference to all have the same size? Or am I not understanding the nature of the change.
|
|
||
| class DetectAndMeasureScoreConnections(DetectAndMeasureConnections): | ||
| scoreExposure = pipeBase.connectionTypes.Input( | ||
| doc="Output likelihood score image", |
There was a problem hiding this comment.
I feel like this docstring needs some more information: does it have a mask/variance? units? What does likelihood score mean exactly? I don't know how much of that should go into this docstring, but something more informative, please.
| @timeMethod | ||
| def run(self, science, matchedTemplate, difference, scoreExposure, | ||
| idFactory=None): | ||
| """Detect and measure sources on a difference image. |
There was a problem hiding this comment.
I'm torn on whether we should have a docstring for this run method at all. The only API change is adding scoreExposure, but also what run does here is different because of that score image. If you do leave this docstring, please expand the opening sentence/paragraph a bit to describe how it's different, otherwise expand the class docstring a bunch.
| # This will fail if we have mis-subtracted the background. | ||
| stdVal = _computeRobustStatistics(output.scoreExposure.image, output.scoreExposure.mask, | ||
| statsCtrl, statistic=afwMath.STDEV) | ||
| # get the img psf Noise Equivalent Area value |
There was a problem hiding this comment.
This comment doesn't add anything; the function name is descriptive enough.
tests/test_subtractTask.py
Outdated
| deltaStd = _computeRobustStatistics(delta.image, delta.mask, statsCtrl, | ||
| statistic=afwMath.STDEV) | ||
| self.assertFloatsAlmostEqual(deltaMean, 0, atol=5*meanError) | ||
| # get the img psf Noise Equivalent Area value |
There was a problem hiding this comment.
This comment doesn't add anything; the function name is descriptive enough.
tests/test_subtractTask.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| nea : `float` |
There was a problem hiding this comment.
Docstrings for Parameters and Returns.
tests/test_subtractTask.py
Outdated
|
|
||
|
|
||
| def _computePSFNoiseEquivalentArea(psf): | ||
| """Compute the noise equivalent area for an image psf |
There was a problem hiding this comment.
I'm not familiar with "noise equivalent area" - should this be something available for PSFs in general, or is it really only a thing for these tests? Is it worth a sentence describing what it means here?
|
|
||
|
|
||
| class DetectAndMeasureScoreConfig(DetectAndMeasureConfig, | ||
| pipelineConnections=DetectAndMeasureScoreConnections): |
There was a problem hiding this comment.
Do you even need this class, if it's just DetectAndMeasureConfig? Until you add/override things in it, might as well just use DetectAndMeasureConfig as the ConfigClass for the new Task.
There was a problem hiding this comment.
Yes, it's needed to set the connections.
isullivan
force-pushed
the
tickets/DM-34909
branch
2 times, most recently
from
1aa4258
to
1a9a284
Compare
isullivan
force-pushed
the
tickets/DM-34909
branch
2 times, most recently
from
9abc085
to
60ce4e1
Compare
isullivan
force-pushed
the
tickets/DM-34909
branch
2 times, most recently
from
53d5247
to
cc46b73
Compare
There was a problem hiding this comment.
I see several previous comments that were maybe missed? I tagged the ones I saw with eyes.
I think there's a lot of consolidation that can happen in these tests: as it stands, it's very unclear what's different between the different test methods.
I don't think you want to use the mask image in selectSources at all: the input catalog should already have all relevant flags set.
|
|
||
| class DetectAndMeasureScoreTask(DetectAndMeasureTask): | ||
| """Detect DIA sources using a Score image, | ||
| and measure the detections on the difference image. |
There was a problem hiding this comment.
Please expand the class docstring to explain why this is different from regular DetectandMeasure.
Also, what's up with the formatting?
There was a problem hiding this comment.
What's wrong with the formatting? It's too long for a single line, and I prefer to break lines at a comma if there is one.
There was a problem hiding this comment.
Fine to leave it, but we normally just wrap at the 79char limit.
| defaultTemplates=_defaultTemplates): | ||
| scoreExposure = connectionTypes.Output( | ||
| doc="The Score, or maximum likelihood, exposure," | ||
| " used for the detection of diaSources.", |
There was a problem hiding this comment.
Why is Score capitalized?
Why is this two lines?
| @@ -557,18 +559,28 @@ def _convolveExposure(exposure, kernel, convolutionControl, | |||
| else: | |||
| return convolvedExposure[bbox] | |||
|
|
|||
| def _sourceSelector(self, sources): | |||
| def sourceSelector(self, mask, sources): | |||
There was a problem hiding this comment.
Why are you making this non-private?
Also, generally when changing the args, I'd recommend adding the new arg after the old one (so sources, mask).
| ------ | ||
| RuntimeError | ||
| If there are too few sources to compute the PSF matching kernel | ||
| remaining after source selection. | ||
| """ | ||
| flags = [True, ]*len(sources) |
There was a problem hiding this comment.
I know it's not your commit, but I'm a bit bothered by this being a list and not np.ones(len(sources), dtype=bool).
There was a problem hiding this comment.
I'm changing it, but I think the original was more clear.
| selectSources = sources[flags] | ||
| self.log.info("%i sources selected for PSF matching out of %i from the input catalog", | ||
| len(selectSources), len(sources)) |
There was a problem hiding this comment.
I wonder if this would be easier to parse in the logs if it were (putting the numbers for comparison together):
nSelected = sum(flags)
"%i/%i=%.3f sources selected for PSF matching from the input catalog", len(selectSources), len(sources), nSelected, len(sources)/nSelected
Not sure whether it's worth including the fraction at the end, but doing the math for people can be handy.
tests/test_detectAndMeasure.py
Outdated
| for diaSource in output.diaSources: | ||
| self._check_diaSource(transientSources, diaSource, refIds=refIds, scale=polarity) | ||
| _detection_wrapper(polarity=1) | ||
| _detection_wrapper(polarity=-1) |
There was a problem hiding this comment.
Polarity is really a bool, no? False=>negative, True->positive? Or just make it positive=True/False instead?
| matchedTemplate.image.array[...] = np.roll(matchedTemplate.image.array[...], offset, axis=0) | ||
| matchedTemplate.variance.array[...] = np.roll(matchedTemplate.variance.array[...], offset, axis=0) | ||
| matchedTemplate.mask.array[...] = np.roll(matchedTemplate.mask.array[...], offset, axis=0) |
There was a problem hiding this comment.
Probably worth a comment to say that this shifts the maskedImage by one pixel, to cause dipoles in the subtraction; it wasn't obvious on first view.
| config.doForcedMeasurement = False | ||
| detectionTask = detectAndMeasure.DetectAndMeasureScoreTask(config=config) | ||
| output = detectionTask.run(science, matchedTemplate, difference, score) | ||
| self.assertIn(dipoleFlag, output.diaSources.schema.getNames()) |
There was a problem hiding this comment.
Shouldn't that flag always be in the output schema, whether there are detected dipoles in the image or not?
| output = detectionTask.run(science, matchedTemplate, difference, score) | ||
| self.assertIn(dipoleFlag, output.diaSources.schema.getNames()) | ||
| nSourcesDet = len(sources) | ||
| self.assertEqual(len(output.diaSources), 2*nSourcesDet) |
There was a problem hiding this comment.
Why should diaSources be twice the length of the input source list?
tests/test_detectAndMeasure.py
Outdated
| self._check_diaSource(transientSources, skySource, matchDistance=1000, scale=0., | ||
| atol=np.sqrt(transientFluxRange*transientFluxLevel)) | ||
|
|
||
| def _check_diaSource(self, refSources, diaSource, refIds=None, |
There was a problem hiding this comment.
Should this be in a parent class or a free function? This version looks identical to the one in DetectAndMeasureTest.
isullivan
force-pushed
the
tickets/DM-34909
branch
3 times, most recently
from
eaa9f7c
to
f742b0f
Compare
tests/test_detectAndMeasure.py
Outdated
| rtol=rtol, atol=atol) | ||
|
|
||
| def _check_values(self, values, minValue=None, maxValue=None): | ||
| """Summary |
| # Run detection and check the results | ||
| def _detection_wrapper(positive=True): | ||
| """ | ||
| """ |
|
Lets chat about it today, but I think what you want to replace the mask flags check with is something like |
isullivan
force-pushed
the
tickets/DM-34909
branch
from
f742b0f
to
eb6ceff
Compare
No description provided.