DM-44363: clear the NOT_DEBLENDED mask plane for the difference image #315
Conversation
isullivan
force-pushed
the
tickets/DM-44363
branch
from
68bf150
to
5e29d2b
Compare
There was a problem hiding this comment.
I'm still not sure I understand what the tests are supposed to be doing (e.g. self.assertTrue(diffimSum >= scienceSum) is checking that there are more of that mask bit set in one image than the other? What is that informative of?).
Otherwise, I think this is ok, and there's not obviously other bits we should clear from the list you posted.
| # Erase existing detection mask planes. | ||
| # We don't want the detection mask from the science image |
There was a problem hiding this comment.
Thanks for removing this: it was always going to be misleading compared with the docstring of updateMasks.
| # Clear existing deblender mask planes, if present | ||
| if "NOT_DEBLENDED" in difference.mask.getMaskPlaneDict().keys(): | ||
| mask = difference.mask | ||
| mask &= ~(mask.getPlaneBitMask("NOT_DEBLENDED")) |
There was a problem hiding this comment.
I think this should happen at the same place the clearing of the detected planes happens, at line 337. You can just add it to the ORed bits being cleared there.
There was a problem hiding this comment.
We aren't guaranteed to have NOT_DEBLENDED present, so we still need to have this check. I can consolidate this with clearing the DETECTED mask.
tests/test_subtractTask.py
Outdated
| @@ -143,10 +140,7 @@ def test_clear_template_mask(self): | |||
| diffimMask = output.difference.mask | |||
| for maskPlane, scienceSum in scienceMaskCheck.items(): | |||
| diffimSum = np.sum(diffimMask.array & mask.getPlaneBitMask(maskPlane) > 0) | |||
There was a problem hiding this comment.
Highlighting here, but the line above about "Except the "DETECTED" planes should have been cleared" should be deleted. Please check for other places in the tests that make that claim about the mask, since it's now false.
| The DETECTED and DETECTED_NEGATIVE mask planes of the science image | ||
| will be erased. | ||
| """ | ||
| self._clearMask(science.mask, clearMaskPlanes=["DETECTED", "DETECTED_NEGATIVE"]) |
There was a problem hiding this comment.
Thinking about this more: I think we do want to clear these mask planes on the difference image before we write it, even though it is the temporary one that gets superseded by the one in detectAndMeasure. Having any detection planes on a diffim before we've measured on it is just misleading. I think it doesn't matter whether or not we clear them on the science image, because we don't write it.
There was a problem hiding this comment.
OK, so would you prefer I remove the second commit that modified subtractImages?
There was a problem hiding this comment.
Yes, I think so. I think it is more misleading to have a detection mask set before we've done detection than whatever benefit we might gain in debugging, but I'm not 100% sure.
isullivan
force-pushed
the
tickets/DM-44363
branch
2 times, most recently
from
eae755d
to
ee26088
Compare
| for mp in clearMaskPlanes: | ||
| if mp not in mask.getMaskPlaneDict(): | ||
| mask.addMaskPlane(mp) |
There was a problem hiding this comment.
I understand why you're doing this, but it makes me a bit uncomfortable. We should be getting a coherent mask plane dict from earlier in the pipeline, if we're not that's a problem.
At the very least a comment above these lines saying why you're adding them would be good.
isullivan
force-pushed
the
tickets/DM-44363
branch
from
ee26088
to
fe4690d
Compare
isullivan
force-pushed
the
tickets/DM-44363
branch
from
fe4690d
to
bbd8616
Compare
No description provided.