DM-28961: DiaSource centroids outside of image causing crash in cuttout creation in PackageAlerts. #108
Conversation
Debug unittests.
There was a problem hiding this comment.
Looks good. My main question is whether we should catch all InvalidParameterErrors, or only the ones that were caused by this particular bug. I assume we would want to catch them all, but we might want to hit them for now so that we can try to understand where they are coming from.
| skyCenter : `lsst.geom.SpherePoint` | ||
| Center point of DiaSource on the sky. | ||
| bbox : `lsst.geom.Extent2I` |
There was a problem hiding this comment.
Isn't it a BBox and not and Extent2I?
There was a problem hiding this comment.
It is an Extent2I. Do you want me to change the variable names?
tests/test_packageAlerts.py
Outdated
| self.exposure.getPhotoCalib()) | ||
| self.exposure.getBBox().getDimensions(), | ||
| self.exposure.getPhotoCalib(), | ||
| 1234) |
There was a problem hiding this comment.
Please define 1234 as a variable (something like `diaSourceId = 1234) and pass that in, instead of a number. Also for the rest of this file where you are passing in a source ID.
| self.log.warn( | ||
| "Failed to retrieve cutout from image for DiaSource with " | ||
| "id=%i. InvalidParameterError thrown during cutout creation. " | ||
| "Returning `None` for cuttout." |
| % srcId) | ||
| if not geom.Box2D(image.getBBox()).contains(point): | ||
| self.log.warn( | ||
| "DiaSource centroid lies at pixel (%.2f, %.2f) " |
There was a problem hiding this comment.
Please include the source ID in this warning as well.
| "which is outside the Exposure with bounding box " | ||
| "((%i, %i), (%i, %i))." % | ||
| (point.x, point.y, | ||
| imBBox.minX, imBBox.maxX, imBBox.minY, imBBox.maxY)) |
There was a problem hiding this comment.
I'm a little worried about this swallowing up other as-yet-unknown errors. Would it make sense to only return None for this case, and re-raise the error if it was a different problem?
There was a problem hiding this comment.
So I looked over the code for cutouts in C++. The two errors this block can throw are both InvalidParameterError one for centroid outside of exposure bbox and the extent of the cutout bbox is <=0. There may be others further up stream but I haven't investigated that completely. I guess the best I can do here is restructure it to catch the error for outside centroid (by testing it) and then do a raise on anything else. Sound good?
Split raise and warn conditions. Change from BBox to correct Extent. Add diaSrcId variable to unittest. Fix linting.
morriscb
force-pushed
the
tickets/DM-28961
branch
from
5953b8a
to
e05e6aa
Compare
No description provided.