refactor: centralize geometry-aware dispatch for detection_area and detection_iou#2374
Open
mvanhorn wants to merge 2 commits into
Open
refactor: centralize geometry-aware dispatch for detection_area and detection_iou#2374mvanhorn wants to merge 2 commits into
mvanhorn wants to merge 2 commits into
Conversation
2 tasks
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes geometry-precedence dispatch (mask → OBB → AABB) for per-detection area and pairwise IoU into a single internal helper module, then routes Detections.area through that shared implementation to reduce recurring “silent AABB fallback” regressions.
Changes:
- Added internal
detection_area(...)anddetection_iou(...)helpers undersrc/supervision/detection/utils/geometry.py. - Refactored
Detections.areato delegate todetection_area(removing duplicated dispatch logic incore.py). - Added contract-style tests ensuring area/IoU respect richer geometry and catch envelope-vs-OBB regressions.
Review scores (1–5):
- Code quality: 4/5
- Testing: 5/5
- Documentation: 4/5
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/supervision/detection/utils/geometry.py | Introduces centralized geometry-aware dispatch helpers for area and IoU. |
| src/supervision/detection/core.py | Refactors Detections.area to call the shared helper. |
| tests/detection/utils/test_geometry.py | Adds parity + metamorphic/contract tests for dispatch correctness across mask/OBB/AABB. |
Comment on lines
+39
to
+42
| if detections.mask is not None: | ||
| if isinstance(detections.mask, CompactMask): | ||
| return detections.mask.area | ||
| return np.array([np.sum(mask) for mask in detections.mask]) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before submitting
Description
Centralizes the "which geometry is the real one?" dispatch for detection area and IoU into a single internal module so the mask -> OBB -> box precedence lives in one place instead of being re-implemented (and silently forgotten) at each call site.
Type of Change
Motivation and Context
A recurring "silent AABB" bug class keeps reappearing: code reaches for
detections.xyxyand forgets the real geometry lives indata["xyxyxyxy"](OBB) ormask. Becausexyxyis always present, the axis-aligned answer is the silent default. Recent fixes (#2306 area returning AABB area, #2303with_nms/with_nmmdropping crossed OBBs via AABB IoU, #2289as_yolodropping OBB rotation) share this root cause: every consumer re-derives geometry precedence and forgetting it fails silently.@kounelisagis endorsed (2026-06-17) keeping the policy internal ("exact when richer geometry is present") behind a single geometry-resolution helper rather than a public per-op mode argument, and suggested a metamorphic test.
This PR is a deliberate first slice of the multi-part issue: the read-only dispatch helpers plus the contract test land first.
move_detectionsconsolidation and thewith_nms/with_nmm/as_yolocall-site migration are left as independent follow-ups per the issue's incremental rollout.Refs #2318
Changes Made
src/supervision/detection/utils/geometry.pywith two batched, read-only helpers:detection_area(mask sum /CompactMask.area-> shoelace OBB area ->box_area) anddetection_iou(mask_iou_batch->oriented_box_iou_batch->box_iou_batch, by the richest geometry both operands carry).Detections.areaincore.pythroughdetection_areaso the property is a thin call into the shared helper, with no behavior change and the vectorized design preserved.tests/detection/utils/test_geometry.py: area-parity across mask/OBB/AABB plus the metamorphic contract (identical envelopes with different OBB corners must differ; rotating an OBB off-axis leavesdetection_areainvariant while the envelope area changes).Testing
Added
tests/detection/utils/test_geometry.pycovering area parity (mask/OBB/AABB), the metamorphic rotation-invariance contract, emptyDetections,CompactMaskrouting, and the AABB IoU fallback. The helpers are read-only and preserveDetections.areabehavior exactly.Note: I was unable to run the full suite in my local sandbox (the test stack pulls
cv2/matplotlib, which aren't installed in my environment); the new tests are written to run under the repo's CI. Happy to adjust if CI surfaces anything.Google Colab (optional)
N/A
Screenshots/Videos (optional)
N/A (internal refactor; no user-visible output change)
Additional Notes
The new helpers are intentionally module-private (no public API surface, no per-op mode argument) to match the maintainer's stated preference. Follow-ups can migrate
with_nms/with_nmm/as_yoloand consolidatemove_detectionsonto the same dispatch.