Fix CircularRegionOfInterest.pixels_in_region omitting boundary pixels - #1895
Fix CircularRegionOfInterest.pixels_in_region omitting boundary pixels#1895adityasingh2400 wants to merge 1 commit into
Conversation
pixels_in_region enumerated range(floor(c - r), ceil(c + r)). range excludes its stop value, so pixels at the +x and +y extremes of the disc were never passed to is_inside at all. is_inside tests a closed disc with <=, so it accepts those pixels, leaving the region asymmetric about its own centre. Add 1 to both upper bounds so every pixel is_inside accepts is enumerated. The existing radius=1 expectation in test_regionofinterest.py contradicted the radius=1.01 expectation directly below it. Both enclose the same pixel centres under a closed disc, so the radius=1 case is corrected here. RectangularRegionOfInterest and PolygonRegionOfInterest were swept for the same off-by-one and are unaffected: the rectangle's is_inside uses a half-open interval that matches its range bounds, and the polygon's ray caster excludes its top and right edges to match. Fixes NeuralEnsemble#1889
|
The readthedocs check here is a builder-side network failure, not something this branch does. The build dies in This PR only touches A rebuild should clear it. I have left it alone rather than pushing an empty commit to force one, but say the word if you would rather I did. |
|
The Read the Docs check is red here and it is a network failure on the builder rather than anything in this change. The build dies while sphinx is reading This PR touches I also checked the version angle before assuming it. The two neo builds earlier the same day, at 12:07 and 12:40, resolved to the same A rebuild should clear it. I have not pushed an empty commit to force one, since that seemed like the wrong way to retrigger, but I am happy to if that is easier than clicking rebuild. |
Fixes #1889.
CircularRegionOfInterest.is_inside()tests a closed disc with<=, so a pixel centre exactlyradiusaway is inside. Butpixels_in_region()enumeratedrange(int(floor(c - r)), int(ceil(c + r))), andrangeexcludes its stop value, so pixels at the+xand+yextremes were never passed tois_inside()at all. They were not rejected, they were never asked about.The region therefore came out asymmetric about its own centre. For
CircularRegionOfInterest(seq, 10, 10, 5)it returned 79 pixels spanning x 5 to 14, whileis_inside()accepts 81 spanning 5 to 15.The fix adds 1 to both upper bounds, so every pixel
is_inside()accepts is offered to it.The repo's own test file contradicted itself on this. Line 11 expected 3 pixels at
radius=1while line 13 expected 5 atradius=1.01. Under a closed disc both radii enclose exactly the same pixel centres, so those two lines could not both be right. Updating theradius=1expectation to the same 5 pixels is an intentional part of this fix, not an accident. That assertion is what encoded the bug.I swept the other two ROI classes for the same off-by-one and they are unaffected, so this leaves them alone.
RectangularRegionOfInterest.is_inside()uses a half-open interval that exactly matches what itsrange()bounds enumerate, with 0 disagreements across 11664 centre, width and height combinations. The polygon ray caster excludes its top and right edges to match its bounding box, with 0 disagreements across 400 random polygons.Tests: the corrected
test_result, plustest_pixels_in_region_matches_is_inside, which locks in the real invariant that the enumeration must equal the predicate's accept set, andtest_pixels_in_region_is_symmetric_about_centre, which is the issue's exact reproduction.Restoring
neo/core/regionofinterest.pyfrom master makes all three fail, with[[6, 5], [5, 6], [6, 6]] != [[6, 5], [5, 6], [6, 6], [7, 6], [6, 7]]. All five pass after. The core suite is green at 617 passed and 16 skipped, and the circle sweep goes from 486 of 4536 configurations disagreeing to 0 of 4536.Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.