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-36358: "broken" amplifiers trigger a failure in setting the threshold for defects #155
Conversation
Can we add a test for this? |
python/lsst/cp/pipe/defects.py
Outdated
except InvalidParameterError: | ||
# This occurs if the image sigma value is 0.0. | ||
# Let's mask the whole area. | ||
minValue = np.min(ampImg.image.array) - 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking if this needs to be nanmin.
605069c
to
d96a0d6
Compare
try: | ||
footprintSet = afwDetection.FootprintSet(ampImg, threshold) | ||
except InvalidParameterError: | ||
# This occurs if the image sigma value is 0.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other way this could happen? Testing exceptions should be the last resort. Can't the threshold
value be checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The threshold
itself is fine. It's the image sigma value coming out as zero that causes the InvalidParameterError
. Manually calculating the image statistics seemed to be less efficient than doing the try/except, as most amplifiers should work fine.
# Let's mask the whole area. | ||
minValue = np.nanmin(ampImg.image.array) - 1.0 | ||
threshold = afwDetection.createThreshold(minValue, 'value', polarity=True) | ||
footprintSet = afwDetection.FootprintSet(ampImg, threshold) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a pretty complicated (and expensive) way to create a FootprintSet
covering the whole area. Since a FootprintSet
can be created from a Box2I
, wouldn't that be simpler and faster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got lost in the FootprintSet
definition before I saw that it could have a Box
passed to it. I'll refactor for that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After coding this and sending to Jenkins, I'm now discovering that the box definition only sets the region, and does not create a footprint set containing footprints that can be used to mask the image.
Handle exception caused by a single constant pixel value.