Conversation
…ated rect Previously with_nmm for OBB detections kept the winner's OBB geometry unchanged (only confidence was merged), making it inconsistent with AABB NMM which expands to the union envelope. Now computes cv2.minAreaRect over all N×4 corners from the merge group — the MARC degenerates to the axis-aligned union for zero-rotation OBBs, preserving full consistency with AABB NMM. - Replace winner-OBB xyxy patch with MARC of all merged corners - Update ORIENTED_BOX_COORDINATES in data to reflect merged geometry - Rename test to reflect new expected behaviour (union, not winner AABB) - Add consistency test asserting axis-aligned OBB NMM == AABB NMM xyxy --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2312 +/- ##
=======================================
Coverage 80% 80%
=======================================
Files 66 66
Lines 8893 8948 +55
=======================================
+ Hits 7142 7196 +54
- Misses 1751 1752 +1 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Detections.with_nmm for oriented bounding boxes (OBB) so that merged detections compute a new merged OBB geometry (via cv2.minAreaRect over all merged corners) instead of keeping the winner’s original OBB, aligning OBB NMM semantics more closely with AABB NMM “union envelope” behavior.
Changes:
- Update OBB NMM merge behavior to compute a merged min-area rotated rectangle and update
ORIENTED_BOX_COORDINATESaccordingly. - Update/expand tests to validate merged OBB geometry and assert axis-aligned OBB NMM matches AABB NMM xyxy results.
Quality checklist (n/5):
- Code quality: 3/5
- Testing: 3/5
- Docs: 4/5
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/supervision/detection/core.py |
Recomputes merged OBB geometry during NMM and updates xyxy/data[ORIENTED_BOX_COORDINATES]. |
tests/detection/test_core.py |
Renames and extends NMM tests to validate union-style merged geometry and AABB/OBB consistency for axis-aligned boxes. |
…OBB NMM - Add .reshape(4, 2) to OBB corner extraction loop so flat-adjacent shapes are normalised before cv2.minAreaRect - Add inline comment at xyxy override: OBB groups intentionally discard AABB-union xyxy from reduce() to stay consistent with MARC corners --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…ugh, class-agnostic, IOS, flat-format - Add test_rotated_obb_merge_produces_marc: two 45-degree OBBs, assert MARC encompasses all corners - Add test_three_detection_group_merge: three overlapping OBBs, assert merged len==1 and envelope spans all inputs - Add test_single_detection_passthrough_preserves_obb: non-overlapping OBB passes through unchanged - Add test_class_agnostic_obb_merge: class_agnostic=True merges cross-class OBBs - Add test_overlap_metric_ios_obb_merge: IOS metric merges contained OBBs - Add test_flat_n8_obb_format_raises_value_error: documents that (N,8) flat format is unsupported (canonical is (N,4,2)) - Import OverlapMetric for IOS test --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…log entry - Add Note section to with_nmm docstring explaining MARC behavior: union for zero-rotation OBBs, MARC for rotated OBBs, single-group passthrough - Add changelog UnReleased entry for #2312 behavioral change --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
| Note: | ||
| For detections carrying oriented bounding box data | ||
| (``data[ORIENTED_BOX_COORDINATES]``), each merge group's output OBB | ||
| is the minimum-area rotated rectangle (``cv2.minAreaRect``) enclosing |
Contributor
There was a problem hiding this comment.
I think that the zero-rotation consistency doesn't hold for diagonal merge groups - minAreaRect picks the angle by area, and the winning tilted rect can stick out past every input:
quads = np.stack([
box_corners(0, 0, 20, 20), # zero-rotation OBBs in a diagonal staircase,
box_corners(12, 12, 32, 32), # middle box has top score
box_corners(24, 24, 44, 44),
])
detections.with_nmm(threshold=0.05).xyxy
# develop: [[12, 12, 32, 32]]
# this branch: [[-10, -10, 54, 54]] <- negative coords, outside every input
# AABB NMM: [[0, 0, 44, 44]]The new three-detection test doesn't catch it because its boxes are laid out horizontally, where the min-area rect happens to be axis-aligned.
Maybe build the enclosing rect at the winner's angle (project all corners onto the winner's axes and take min/max) instead of letting minAreaRect pick the angle. For zero-rotation inputs that yields exactly the axis-aligned union, and for the rotated test case in this PR it produces an identical rect - so the existing tests pass unchanged.
Member
Author
There was a problem hiding this comment.
Maybe build the enclosing rect at the winner's angle (project all corners onto the winner's axes and take min/max) instead of letting minAreaRect pick the angle. For zero-rotation inputs that yields exactly the axis-aligned union, and for the rotated test case in this PR it produces an identical rect - so the existing tests pass unchanged.
Sounds good to me and really appreciate you are paying attention and helping us...
…void overshoot cv2.minAreaRect picks a 45-degree rect for diagonal staircase arrangements of axis-aligned boxes, producing an AABB like [-10,-10,54,54] that extends outside every input. Fix: lock merged OBB to winner's angle by projecting all corners onto the winner's principal axes (from first edge vector), computing AABB there, and back-rotating — for zero-rotation inputs this gives exactly the axis-aligned union; for same-angle groups the result equals the prior MARC. - Remove cv2 dependency from the OBB merge block (pure numpy now) - Add test_diagonal_staircase_obb_merge_stays_within_union regression test - Rename test to reflect winner-angle semantics --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…in NMM
- docs/changelog.md: replace stale MARC/cv2.minAreaRect wording with
winner's-angle description matching the actual implementation
- core.py: validate ORIENTED_BOX_COORDINATES shape is (N, 4, 2) at the
start of the OBB merge block; raises ValueError("corners must have
shape (N, 4, 2)") for flat (N, 8) input instead of silently mis-reshaping
---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…ate 7 individual OBB NMM test methods into a single parametrized test_obb_nmm_merge with explicit expected_confidence and expected_corners assertions. Add cases for mixed-angle merges, multiple merge groups, and degenerate collinear OBBs. Add standalone test_obb_nmm_empty_detections for empty inputs.
…usage with nested lists - Update test parameters to use plain Python lists instead of `numpy` arrays for corner definitions. - Adjust the `_make_obb_detections` setup to preprocess corners into `numpy` arrays. - Add explicit conversion of `expected_corners` to `numpy` arrays in the assertions.
…zed conversion of oriented bounding box corners (N, 4, 2) to axis-aligned bounding boxes (N, 4). Used internally in with_nmm and exposed via top-level import.
…Function is unused dead code with no external callers. Decorator emits FutureWarning while preserving existing behavior.
…with_nmm Replace inline OBB post-processing and reduce-based merging with two private helpers using single-pass area-weighted confidence. Deprecate merge_inner_detection_object_pair and merge_inner_detections_objects_without_iou (0.29.0 -> 0.34.0). Rename TestDetectionsWithNmm -> TestDetectionsWithNMM and expand TestMergeDetectionGroup to assert all output fields via expected_detections.
Collaborator
|
Borda
commented
Jun 15, 2026
Comment on lines
+1228
to
+1234
| _make_obb_detections( | ||
| [ | ||
| np.array( | ||
| [[0, 0], [10, 0], [10, 5], [0, 5]], | ||
| dtype=np.float32, | ||
| ) | ||
| ], |
Member
Author
There was a problem hiding this comment.
feel would be easier to read if the function accepts lists and does the conversion to np array inside
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.
Previously with_nmm for OBB detections kept the winner's OBB geometry unchanged (only confidence was merged), making it inconsistent with AABB NMM which expands to the union envelope. Now computes cv2.minAreaRect over all N×4 corners from the merge group — the MARC degenerates to the axis-aligned union for zero-rotation OBBs, preserving full consistency with AABB NMM.