DM-42313: PixelFlags offimage check should flag non-finite positions #261
Conversation
| if (!mimage.getBBox().contains(geom::Point2I(center))) { | ||
| // Flag centroids off the image. | ||
| geom::Box2D bbox(mimage.getBBox()); | ||
| if (!bbox.contains(center)) { |
There was a problem hiding this comment.
Add a comment explaining why you're specifically converting int to float instead of the other way around?
There was a problem hiding this comment.
Do we need to worry about the differing coordinate systems for Boxes (which does not come up when converting Points)? Put another way, if center has coordinates of (0,0), is it in the middle of the corner pixel, or at the image corner?
There was a problem hiding this comment.
Can you add a test case that covers these off-image or borderline centroids?
There was a problem hiding this comment.
Good point. I've added some tests of this.
As far as I can tell, the behavior I've implemented here doesn't change how points on the boundary are handled. For example, Point2I(Point2D(-20.55, 20.55)) gives Point2I(-21, 21) (the Point2I constructor rounds from Point2D), which in effect already "expands" the image bbox by 0.5 pixels, as the Box2D(Box2I) constructor describes.
The code had set flag_edge if the source was off the chip, but the docstring did not specify that.
There is no Box2I.contains(Point2D), so we have to cast the box instead of the centroid, if we want it to work for non-finite values.
Catching NaN centroids early means that the offimage flag is not set for them, but it should be. Reformat exception message to be more useful (although the measurement task squashes all these to debug level logs, so noone ever sees them anyway). Add tests of non-finite centroids and sources right on the image boundary.
parejkoj
force-pushed
the
tickets/DM-42313
branch
from
f87b0ee
to
f4c3f67
Compare
No description provided.