Added mask planes from template and science fake plane into difference #280

BrunoSanchez · 2023-09-13T11:39:45Z

Added the needed mask plane into differences. These are two new planes named 'INJECTED' and 'INJECTED_TEMPLATE'

isullivan

In addition to the code comments, please add a simple unit test of the new updateMasks.

isullivan · 2023-09-20T00:03:40Z

python/lsst/ip/diffim/subtractImages.py

+            templateFakeBitMask = template[bbox].mask.getPlaneBitMask('FAKE')
+
+            injScienceMaskArray = ((science.mask.array & scienceFakeBitMask) > 0)
+            injScienceMaskArray *= diffInjectedBitMask


In testing, I get the following error from this line (and below):
numpy.core._exceptions._UFuncOutputCastingError: Cannot cast ufunc 'multiply' output from dtype('int64') to dtype('bool') with casting rule 'same_kind'
I suggest removing the line injScienceMaskArray *= diffInjectedBitMask and using mask.array[injScienceMaskArray] |= diffInjectedBitMask below. You might want to change the variable name of injScienceMaskArray to reflect the new usage.

I found an alternative solution to this. I have simplified to a single line, and multiplied by the bit mask values in the same variable assignation. It also seems to suit the same name, at least to me.

isullivan · 2023-09-20T00:04:02Z

python/lsst/ip/diffim/subtractImages.py

+            injTemplateMaskArray = ((template[bbox].mask.array & templateFakeBitMask) > 0)
+            injTemplateMaskArray *= diffInjectedTemplateBitMask
+
+            mask.array |= injScienceMaskArray


~~See my requested modification in the comment above.~~ Looks resolved with the above changes.

isullivan · 2023-09-20T00:06:49Z

python/lsst/ip/diffim/subtractImages.py

@@ -581,6 +576,42 @@ def finalize(self, template, science, difference, kernel,
            correctedExposure = difference
        return correctedExposure

+    def updateMasks(self, difference, science, template):


Could you keep the same order of images as in finalize: template, science, difference

isullivan · 2023-09-20T00:17:08Z

python/lsst/ip/diffim/subtractImages.py

+            maskPlane for maskPlane in badMaskPlanes if maskPlane in mask.getMaskPlaneDict().keys()
+        ]
+
+        badPixelMask = mask.getPlaneBitMask(setBadMaskPlanes)


Thanks for changing this. We need to switch from lsst.afw.image.Mask.getPlaneBitMask to mask.getPlaneBitMask in general (see lsst/meas_algorithms#329 (comment))

isullivan · 2023-09-20T00:20:42Z

python/lsst/ip/diffim/subtractImages.py

@@ -712,7 +743,12 @@ def _checkMask(mask, sources, badMaskPlanes):
            kept (True) or rejected (False) based on the value of the
            mask plane at its location.
        """
-        badPixelMask = lsst.afw.image.Mask.getPlaneBitMask(badMaskPlanes)
+        setBadMaskPlanes = [
+            maskPlane for maskPlane in badMaskPlanes if maskPlane in mask.getMaskPlaneDict().keys()


Minor point, but I don't think the .keys() is necessary here.

BrunoSanchez · 2023-10-12T12:55:27Z

This is ready for second review!

isullivan

Looks good! I had a few suggested changes, though the only necessary one is to remove the dangling code comment.

isullivan · 2023-10-12T17:39:46Z

python/lsst/ip/diffim/subtractImages.py

        if self.config.doDecorrelation:
            self.log.info("Decorrelating image difference.")
            # 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[...]


Also remove the associated code comment above.

isullivan · 2023-10-12T17:50:51Z

tests/test_subtractTask.py

+        science.image.array += science_fake_img.image.array
+        template.image.array += tmplt_fake_img.image.array
+
+        # TODO: update to INJECTED names when source injection gets refactored


Try to always reference a ticket number with any TODO. I think that is DM-40796 here.

isullivan · 2023-10-12T18:18:21Z

tests/test_subtractTask.py

+        inj_masked = (diff_mask.array & diff_mask.getPlaneBitMask("INJECTED")) > 0
+
+        # template mask should be now in INJECTED_TEMPLATE
+        injTmplt_masked = (diff_mask.array & diff_mask.getPlaneBitMask("INJECTED_TEMPLATE")) > 0
+
+        self.assertEqual(np.sum(inj_masked.astype(int)-science_fake_masked.astype(int)), 0)
+        self.assertEqual(np.sum(injTmplt_masked.astype(int)-template_fake_masked.astype(int)), 0)


Since the mask plane is changing, this looks like a good way to do the test. For future reference, see https://pipelines.lsst.io/py-api/lsst.utils.tests.TestCase.html for useful built-in asserts, though I don't think any would quite work here.

isullivan · 2023-10-12T18:33:01Z

tests/test_subtractTask.py

+        science.mask.addMaskPlane("FAKE")
+        template.mask.addMaskPlane("FAKE")
+
+        science_mask_planes = science.mask.getMaskPlaneDict()


You could use science_fake_mask = science.mask.getMaskPlane('FAKE') here, and then just use science_fake_mask instead of science_mask_planes['FAKE'] later. Same for the template below.

Or use science_fake_mask = science.mask.getPlaneBitMask("FAKE") if you use my suggestion below.

isullivan · 2023-10-12T18:47:42Z

tests/test_subtractTask.py

+            for dy in (-1, 0, 1):
+                science.mask.setMaskPlaneValues(
+                    science_mask_planes['FAKE'],
+                    int(a_science_source.getX() - x0)-1,
+                    int(a_science_source.getX() - x0)+1,
+                    int(a_science_source.getY() - y0)+dy
+                )


This is fine, but I have an alternate suggestion:

bbox = lsst.geom.Box2I(lsst.geom.Point2I(a_science_source.getX(), a_science_source.getY()), lsst.geom.Extent2I(3, 3)) science[bbox].mask.array |= science_fake_mask

using science_fake_mask from my above suggestion.

BrunoSanchez · 2023-10-12T20:55:56Z

Thanks for the comments.

All of them are implemented!

BrunoSanchez · 2023-10-18T17:04:57Z

This is the final Jenkins run that validates this merge: https://rubin-ci.slac.stanford.edu/job/stack-os-matrix/480/

BrunoSanchez requested a review from isullivan September 13, 2023 11:39

BrunoSanchez force-pushed the tickets/DM-32889 branch from d790d07 to 143cdf1 Compare September 13, 2023 11:43

isullivan requested changes Sep 20, 2023

View reviewed changes

BrunoSanchez requested a review from isullivan October 12, 2023 12:55

isullivan approved these changes Oct 12, 2023

View reviewed changes

Added mask planes from template and science fake plane into difference

7d888a3

BrunoSanchez force-pushed the tickets/DM-32889 branch from fc2f8aa to 7d888a3 Compare October 12, 2023 21:34

BrunoSanchez added 2 commits October 17, 2023 06:46

Fixing mask issues

e4e8c90

Added mask planes from template and science fake plane into difference

41f9f53

BrunoSanchez closed this Oct 18, 2023

BrunoSanchez reopened this Oct 18, 2023

BrunoSanchez merged commit 21e1ef4 into main Oct 18, 2023
4 checks passed

BrunoSanchez deleted the tickets/DM-32889 branch October 18, 2023 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added mask planes from template and science fake plane into difference #280

Added mask planes from template and science fake plane into difference #280

BrunoSanchez commented Sep 13, 2023

isullivan left a comment

isullivan Sep 20, 2023

BrunoSanchez Oct 12, 2023

isullivan Sep 20, 2023 •

edited

Loading

isullivan Sep 20, 2023

BrunoSanchez Oct 12, 2023

isullivan Sep 20, 2023

isullivan Sep 20, 2023

BrunoSanchez Oct 12, 2023

BrunoSanchez commented Oct 12, 2023

isullivan left a comment

isullivan Oct 12, 2023

isullivan Oct 12, 2023

isullivan Oct 12, 2023

isullivan Oct 12, 2023

isullivan Oct 12, 2023

BrunoSanchez commented Oct 12, 2023

BrunoSanchez commented Oct 18, 2023

Added mask planes from template and science fake plane into difference #280

Added mask planes from template and science fake plane into difference #280

Conversation

BrunoSanchez commented Sep 13, 2023

isullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isullivan Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoSanchez commented Oct 12, 2023

isullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoSanchez commented Oct 12, 2023

BrunoSanchez commented Oct 18, 2023

isullivan Sep 20, 2023 •

edited

Loading