DM-38246: Exclude bad pixels from detection #329
Conversation
isullivan
force-pushed
the
tickets/DM-38246
branch
from
40e069e
to
6b03d97
Compare
| """ | ||
| badPixelMask = lsst.afw.image.Mask.getPlaneBitMask(self.config.badMaskPlanes) | ||
| badPixels = middle.mask.array & badPixelMask > 0 | ||
| middle.image.array[badPixels] = 0 |
There was a problem hiding this comment.
At first I was a little unnerved that you were just setting this to zero, but upon further reflection I think a whole lot more would break here if that wasn't a valid thing to do. It might be slightly better to set it to self.config.adjustBackground, since that claims it's a way to add some sort of fixed background to the image for debugging purposes, and then you'd want these pixels to have that background too. But it'd probably be ever better to deprecate that config option and fix whatever test code (if any) is abusing the production task to make testing easier. And that's of course out of scope for this ticket, so I think this is fine.
There was a problem hiding this comment.
Should the variance plan also get zeroed?
| Score or maximum likelihood difference image. | ||
| The image plane will be modified in place. | ||
| """ | ||
| badPixelMask = lsst.afw.image.Mask.getPlaneBitMask(self.config.badMaskPlanes) |
There was a problem hiding this comment.
This should be middle.mask.getPlaneBitMask(), to ensure you're using the plane definitions associated with the image itself.
That distinction may not actually matter right now in most circumstances, but it could in the future.
There was a problem hiding this comment.
There should be no distinction, since the mask planes are defined globally. I try to use the lsst.afw.image.Mask.getPlaneBitMask() invocation in general to make that clear.
There was a problem hiding this comment.
That "global" definition is one of the things that will be changing on DM-32438, and I'm trying to catch uses of it like this ahead of time. It's held up by a house of cards, and isn't as guaranteed to be global as one might hope. Too late for this ticket now, but please use the mask instance call in the future.
| badMaskPlanes = lsst.pex.config.ListField( | ||
| dtype=str, | ||
| default=("NO_DATA", "BAD", "EDGE"), | ||
| doc="Mask planes to exclude when detecting sources." |
There was a problem hiding this comment.
Given the docstring, I think I would rename this to skipDetectionMaskPlanes, doNotDetectMaskPlanes, zeroPixelsBeforeDetectMaskPlanes (ok, that's verbose, but still) or something like that, to make it more explicit what exactly the config is used for. They're not just "bad", they're "never detect on pixels with these masks".
There was a problem hiding this comment.
John correctly pointed out in conversation that individual hot pixels that are successfully interpolated could still have the NO_DATA bit set, and after convolution the masked region would grow to the width of the convolution kernel. That behavior should be fine for the EDGE mask plane, but we risk losing real detections with NO_DATA and BAD. I'll update the defaults here, and we should pursue this further on DM-33798.
There was a problem hiding this comment.
Ah, I just commented on something like this on Jira after your last post made me think about it. I think NO_DATA is probably safe, too, but I am indeed worried about BAD, and I'm not at all opposed to putting off including NO_DATA until we've done more testing.
| """ | ||
| badPixelMask = lsst.afw.image.Mask.getPlaneBitMask(self.config.badMaskPlanes) | ||
| badPixels = middle.mask.array & badPixelMask > 0 | ||
| middle.image.array[badPixels] = 0 |
There was a problem hiding this comment.
Should the variance plan also get zeroed?
isullivan
force-pushed
the
tickets/DM-38246
branch
from
6b03d97
to
9561c9d
Compare
isullivan
force-pushed
the
tickets/DM-38246
branch
from
9561c9d
to
c2bbf9e
Compare
No description provided.