Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-23083: Update large masks for BF convolution issues #123

Merged
merged 1 commit into from Jan 28, 2020

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Jan 17, 2020

Grow large masks after BF correction.
Large masks should be treated like edges in the mask plane. To do so,
convolve the important problem masks (those that are interpolated) by
a top-hat the size of the BF kernel, and add pixels to the mask if
they are calculated using a significant fraction of masked pixels.

@czwa czwa requested a review from yalsayyad January 17, 2020 18:13
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
fpSet = afwDetection.FootprintSet(fpSet, rGrow=growSaturatedFootprints, isotropic=False)
fpSet.setMask(mask, "SAT")
growMasks(mask, radius=growSaturatedFootprints, maskNameList=['SAT'], maskValue="SAT")
# thresh = afwDetection.Threshold(mask.getPlaneBitMask("SAT"), afwDetection.Threshold.BITMASK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave the original inline comment, but remove the commented out code.

@@ -2127,6 +2141,43 @@ def maskEdges(self, exposure, numEdgePixels=0, maskPlane="SUSPECT"):
maskBitMask
)

def maskGrowTophat(self, exposure, boxRadius=0, boxThreshold=0.25, maskPlane="BAD", maskLabel="BAD"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the definition of maskGrowTophat. It'll bitrot without a unit test or use anywhere

Mask image to process.
radius : scalar
Amount to grow the mask.
maskNameList : `list` [`str`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct anymore since you no longer pass in a list here:

                    isrFunctions.growMasks(ccdExposure.getMask(),
                                           radius=self.config.brighterFatterMaskGrowSize,
                                           maskNameList=maskPlane,
                                           maskValue=maskPlane)

You're passing in a single string. The current interface isn't bad, but worth thinking about whether growing a maskNameList with a single maskValue is the most reusable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

growMasks uses the mask.getPlaneBitMask() method, which accepts vectors of strings or single strings, and returns the appropriate value. I've updated the docstring to note both str and list [str] are valid types.

@@ -134,6 +134,27 @@ def makeThresholdMask(maskedImage, threshold, growFootprints=1, maskName='SAT'):
return measAlg.Defects.fromFootprintList(fpList)


def growMasks(mask, radius=0, maskNameList=['BAD'], maskValue="BAD"):
"""Grow a mask by an amount and add to the BAD plane.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring isn't correct after the behavior change. BAD is just your default value.

The brighter-fatter correction convolves pixel values in a way that
isn't safe around masked regions (similar to the edge issue).  To
resolve this, allow masks to be grown by a fixed pixel radius to cover
the pixels that are likely not corrected fully.
@czwa czwa merged commit a51890c into master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants