DM-38901: Clear the template mask plane in image differencing #257
Conversation
There was a problem hiding this comment.
I think that the few comments that I have might be more for tiny changes to avoid repetition of lines that were already executed. Otherwise this looks great to me. I approve, and the changes might be needed or not, they should not introduce significant alterations to code results or behavior.
| x1 = 75 | ||
| y0 = 150 | ||
| y1 = 175 | ||
| for maskPlane in mask.getMaskPlaneDict().keys(): |
There was a problem hiding this comment.
You repeat the for loop on the same dictionary. Could you avoid this repetition by defining your x0, x1, y0, y1 constants earlier?
python/lsst/ip/diffim/utils.py
Outdated
There was a problem hiding this comment.
This looks fine to me.
| @@ -281,7 +323,7 @@ def test_symmetry(self): | |||
| # Don't include a border for the template, in order to make the results | |||
| # comparable when we swap which image is treated as the "science" image. | |||
| science, sources = makeTestImage(psfSize=2.0, noiseLevel=noiseLevel, | |||
| noiseSeed=6, templateBorderSize=0) | |||
| noiseSeed=6, templateBorderSize=0, doApplyCalibration=True) | |||
There was a problem hiding this comment.
Is this related to the ticket or is it a bug that you found while working on this?
There was a problem hiding this comment.
The tests are Ok for me. The mask logic is clear, maybe for clarity rename the mask assignment to templateMask or something that makes it explicit that this is not the "main" mask (diffim mask in my mind) but the template one.
| @@ -153,7 +153,7 @@ class AlardLuptonSubtractBaseConfig(lsst.pex.config.Config): | |||
| detectionThreshold = lsst.pex.config.Field( | |||
| dtype=float, | |||
| default=10, | |||
| doc="Minimum signal to noise ration of detected sources " | |||
| doc="Minimum signal to noise ratio of detected sources " | |||
| # We have cleared the template mask plane, so copy the mask plane of | ||
| # the image difference so that we can calculate correct statistics | ||
| # during decorrelation | ||
| template[science.getBBox()].mask.array[...] = difference.mask.array[...] |
There was a problem hiding this comment.
I don't understand why this line is outside the if and also inside the if statement. I wouldn't place it in the if. Looking at the inline comments, I would think that the correct thing is to do just with line 519.
There was a problem hiding this comment.
Good catch! This line is indeed a duplicate of 519. I'll remove it.
The modifications in `test_order_equal_images` are needed because there is no border padding of the template. The changes in `test_symmetry` are needed to make the images actually symmetric. This should have been the case before, but after clearing the EDGE flag the `template_better.difference` image was entirely NANs due to the statistics calculation in decorrelation still needing a mask plane. This was fixed by copying the difference mask plane to the template before decorrelation.
isullivan
force-pushed
the
tickets/DM-38901
branch
from
c8b6824
to
ff935c2
Compare
A few minor bugs uncovered while working on this ticket are also fixed.