CodeRabbitAI suggested fixes + miscellaneous fixes#305
Conversation
… truncation warning
…-standard strides in nnarchive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFastSAM now propagates mask counts explicitly. SCRFD refreshes cached anchors after configuration changes. Segmentation logging uses configured or inferred class counts, and parser utilities add input normalization and validation. ChangesParser updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
==========================================
- Coverage 48.28% 48.16% -0.13%
==========================================
Files 110 110
Lines 6753 6783 +30
==========================================
+ Hits 3261 3267 +6
- Misses 3492 3516 +24 ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
depthai_nodes/node/parsers/utils/fastsam.py (1)
410-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new mask-count contract.
Please cover empty results (
0), normal detections, and bbox/point prompts. These tests should assert both the returned tuple and the count forwarded to the parser, especially given the reported low patch coverage.Also applies to: 465-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@depthai_nodes/node/parsers/utils/fastsam.py` around lines 410 - 411, Add regression tests around the FastSAM output-decoding function returning the mask/count tuple, covering empty results, normal detections, and both bbox and point prompts. Assert the complete returned tuple and verify the corresponding mask count forwarded to the parser, using mocks or fixtures for the parser interaction.depthai_nodes/node/parsers/segmentation.py (1)
212-212: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrevent expensive
np.uniqueevaluation when debug logging is disabled.Because f-strings are evaluated before the method is called,
_get_logged_class_countwill execute on every frame. Ifclass_namesandn_classesare not configured, this falls back tonp.uniqueon the entire segmentation mask, which can introduce noticeable CPU overhead (e.g., filtering and sorting a large flattened array) even when debug logs are turned off.Consider wrapping this statement to check the log level first.
⚡ Proposed performance optimization
- self._logger.debug( - f"Created segmentation message with {self._get_logged_class_count(class_map)} classes" - ) + if self._logger.isEnabledFor(logging.DEBUG): + self._logger.debug( + f"Created segmentation message with {self._get_logged_class_count(class_map)} classes" + )(Note: Ensure
import loggingis present at the top of the file if applying this change.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@depthai_nodes/node/parsers/segmentation.py` at line 212, Guard the “Created segmentation message” debug log with a DEBUG-level check before interpolating `_get_logged_class_count`, preventing its potentially expensive `np.unique` fallback when debug logging is disabled. Add the `logging` import if needed and preserve the existing message and count behavior when debug logging is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@depthai_nodes/node/parsers/utils/yolo.py`:
- Around line 707-708: Update the YOLO segmentation error message to report the
number of encodable instance masks as max_instance_id + 1, matching the
validation check’s zero-based ID range; keep the surrounding SegmentationMask
handling unchanged.
---
Nitpick comments:
In `@depthai_nodes/node/parsers/segmentation.py`:
- Line 212: Guard the “Created segmentation message” debug log with a
DEBUG-level check before interpolating `_get_logged_class_count`, preventing its
potentially expensive `np.unique` fallback when debug logging is disabled. Add
the `logging` import if needed and preserve the existing message and count
behavior when debug logging is enabled.
In `@depthai_nodes/node/parsers/utils/fastsam.py`:
- Around line 410-411: Add regression tests around the FastSAM output-decoding
function returning the mask/count tuple, covering empty results, normal
detections, and both bbox and point prompts. Assert the complete returned tuple
and verify the corresponding mask count forwarded to the parser, using mocks or
fixtures for the parser interaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be2a9a69-7fa4-4fc1-8c2b-a4782ed8398c
📒 Files selected for processing (8)
depthai_nodes/node/parsers/fastsam.pydepthai_nodes/node/parsers/scrfd.pydepthai_nodes/node/parsers/segmentation.pydepthai_nodes/node/parsers/utils/detection.pydepthai_nodes/node/parsers/utils/fastsam.pydepthai_nodes/node/parsers/utils/image_output.pydepthai_nodes/node/parsers/utils/map_output.pydepthai_nodes/node/parsers/utils/yolo.py
klemen1999
left a comment
klemen1999
left a comment
There was a problem hiding this comment.
One potential improvement, otherwise LGTM
3 participants
Purpose
In a previous PR #302 CodeRabbit suggested some fixes that were unrelated to the refactor of this PR. In this PR, these fixes are addressed one-by-one.
Summary of suggested changes covered by this PR:
Non-CodeRabbit suggestion: this PR also implements this suggestion from LuxonisML
Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Submitted code was reviewed by a human: YES/NO
The author is taking the responsibility for the contribution: YES/NO
Summary by CodeRabbit
Summary by CodeRabbit