refactor: vectorize _mask_from_mask loop - #2495
Draft
shaoming11 wants to merge 10 commits into
Draft
Conversation
Hoist loop-invariant branch outside the loop and replace per-detection Python loop with single-pass list comprehensions over the backing arrays. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover the class_name, class_id, index-fallback, custom labels, and empty detections paths to close the coverage gap from roboflow#2465. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Each branch already returns, so the elif/else chain was structurally unneeded; flat guard clauses read slightly cleaner. [resolve group] PR roboflow#2465 — items 7 --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…match get_labels_text() previously always produced exactly len(detections) labels. The class_name/class_id branches now raise ValueError instead of silently returning a shorter/longer list if detections.data['class_name'] or class_id is mutated out of alignment with detections directly (bypassing Detections' own validation). Flagged by Copilot code review; discussed independently in /review as low-risk/unreachable for validly-constructed Detections, but worth the defensive check since it costs nothing on the happy path. [resolve group] PR roboflow#2465 — items 1 --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- N=1 single-detection case for class_id-fallback and index-fallback - custom_labels precedence over class_name specifically (not just class_id) - non-str-dtype class_name to make the str() coercion load-bearing - pin which branch Detections.empty() exercises (class_id) - cover the ValueError guard added for class_name/class_id length mismatch [resolve group] PR roboflow#2465 — items 3, 4, 5, 6 (+1 additional test for the length-guard added alongside item 1, not separately selected) --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2495 +/- ##
========================================
Coverage 87% 88%
========================================
Files 85 85
Lines 12023 12152 +129
========================================
+ Hits 10514 10683 +169
+ Misses 1509 1469 -40 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Borda
changed the title
_mask_from_mask loop
Contributor
There was a problem hiding this comment.
Pull request overview
Vectorizes mask unioning in ComparisonAnnotator, but introduces a CompactMask memory regression and unrelated label-validation changes.
Changes:
- Replaces iterative dense-mask union with
np.any. - Adds label-alignment validation and tests.
- Expands scope beyond the PR description.
Review scores: Code quality 2/5 · Testing 2/5 · Documentation 2/5
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/supervision/annotators/core.py |
Vectorizes mask unioning. |
src/supervision/annotators/utils.py |
Adds label-alignment errors. |
tests/annotators/test_utils.py |
Tests label generation and validation. |
Suppressed comments (1)
src/supervision/annotators/core.py:3464
- The existing
ComparisonAnnotatortests cover only empty and box-based detections, so none exercises the mask-union path changed here; the newly added tests target a different helper. Add regression coverage for multiple overlapping dense masks and forCompactMaskwithout whole-stack materialization (for example, patchto_denseto fail), so both output parity and the memory-preserving branch are protected.
result: npt.NDArray[np.bool_] = np.any(detections.mask, axis=0)
return result
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Changes: - Keep dense comparison-mask union vectorized while reducing CompactMask crops incrementally without materializing the full mask stack. - Add dense exact-pixel and sparse CompactMask regression coverage that fails if to_dense() is called. - Revert unrelated get_labels_text validation and its tests so this PR remains scoped to mask reduction. Impact: - CompactMask annotation avoids the prior O(N*H*W) memory regression while dense-mask behavior remains unchanged. - The regression tests protect both rendered overlap semantics and the compact-memory contract. - No new label-helper exception behavior or changelog entry is introduced. Verification: - .venv/bin/python -m pytest -q tests/annotators/test_core.py tests/annotators/test_utils.py — 236 passed. - ruff check, ruff format --check, and git diff --check — passed. - Raw mypy remains failed with 90 repository/environment-wide errors; changed modules retain pre-existing missing-stub errors only. - Canonical pytest gate skipped because external pytest-rerunfailures attempts a sandbox-forbidden localhost bind. Residual limits: - Production CompactMask RSS, fresh remote Codecov, supported-environment mypy/pytest, and runtime provenance evidence remain unresolved. --- Co-authored-by: Codex <codex@openai.com>
Borda
approved these changes
Aug 21, 2026
Member
|
@shaoming11, could you pls add some benchmark comparisons before and after? |
Borda
self-requested a review
August 21, 2026 18:41
Borda
marked this pull request as draft
August 21, 2026 22:33
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.
Summary
forloop inComparisonAnnotator._mask_from_maskwith a singlenp.any(detections.mask, axis=0)call, eliminating per-mask iteration and in-place|=accumulation.Test plan
uv run pytest tests/test_annotators.py)ComparisonAnnotatoroutput unchanged🤖 Generated with Claude Code