EngineOutput to normalize engine outputs#16
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
dtronmans
changed the title
…tput to the appropriate engine files
… engine abstractoin
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
luxonis_eval/config/resolver.py (1)
312-321: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude missing keypoint and segmentation parameters during YOLO parser config construction.
YOLOExtendedParsersupportsmask_conf,keypoint_label_names, andkeypoint_edges, but these keys are missing from this metadata extraction loop. If they are provided in an NNArchive for a pose or instance segmentation model, they will be ignored, causing the parser to fall back to defaults and potentially breaking the evaluation pipeline.🐛 Proposed fix
for key in ( "subtype", "n_classes", "anchors", "conf_threshold", "iou_threshold", "max_det", + "mask_conf", + "keypoint_label_names", + "keypoint_edges", ): if key in metadata: params[key] = metadata[key]🤖 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 `@luxonis_eval/config/resolver.py` around lines 312 - 321, Extend the metadata extraction loop in the YOLO parser configuration construction to include mask_conf, keypoint_label_names, and keypoint_edges alongside the existing parameters. Ensure provided values from NNArchive metadata are copied into params so YOLOExtendedParser receives them instead of using defaults.luxonis_eval/engines/depthai_engine.py (1)
79-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire
nn_archive_cfginto NNArchive loading. The constructor stores it, but_load_nn_archive()still buildsdai.NNArchivefrommodel_pathalone, so the option has no effect. Pass the config through the supported archive-loading path or remove the argument.🤖 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 `@luxonis_eval/engines/depthai_engine.py` around lines 79 - 99, Update _load_nn_archive() to use the stored self.nn_archive_cfg when constructing or loading dai.NNArchive, passing it through the supported archive-loading API alongside model_path. If that API cannot accept the configuration, remove nn_archive_cfg from __init__ and stop storing it rather than leaving an unused option.
🧹 Nitpick comments (2)
luxonis_eval/parsers/segmentation.py (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
class_maptoparsed_maskto prevent confusion.The output of
DepthAINodesSegmentationParser.computeis assigned to a variable namedclass_map. Sinceclass_mapconventionally refers to a dictionary mapping class indices to names (as used throughout the rest of the codebase), using it here for the parsed segmentation mask is misleading.♻️ Proposed refactor
- class_map = DepthAINodesSegmentationParser.compute( + parsed_mask = DepthAINodesSegmentationParser.compute( np.asarray(segmentation_mask), classes_in_one_layer=classes_in_one_layer, ) - return create_segmentation_message(class_map) + return create_segmentation_message(parsed_mask)🤖 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 `@luxonis_eval/parsers/segmentation.py` around lines 34 - 39, Rename the local variable assigned from DepthAINodesSegmentationParser.compute in the segmentation parsing flow from class_map to parsed_mask, and pass parsed_mask to create_segmentation_message. Do not change the computation or message behavior.luxonis_eval/metrics/dice_coef.py (1)
112-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove redundant
astype(np.int64)conversions.In both metrics,
pred_maskandtarget_maskare already explicitly cast and returned asint64arrays bynormalize_prediction_segmentation_maskandtarget_segmentation_to_index_mask. Since NumPy's.astype()copies the array by default, calling.astype(np.int64)during thetorch.from_numpyconversion creates unnecessary full copies of the segmentation masks on every batch iteration, needlessly increasing memory allocation and degrading evaluation performance.
luxonis_eval/metrics/dice_coef.py#L112-L115: Remove.astype(np.int64)from both tensor conversions (usetorch.from_numpy(pred_mask)).luxonis_eval/metrics/mIoU.py#L113-L116: Remove.astype(np.int64)from both tensor conversions.🤖 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 `@luxonis_eval/metrics/dice_coef.py` around lines 112 - 115, Remove the redundant astype(np.int64) calls from both torch.from_numpy conversions in luxonis_eval/metrics/dice_coef.py lines 112-115 and luxonis_eval/metrics/mIoU.py lines 113-116, passing pred_mask and target_mask directly. The existing normalization functions already return int64 arrays, so preserve that dtype without creating per-batch copies.
🤖 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 `@luxonis_eval/config/resolver.py`:
- Around line 285-286: Update the parser-name set in the resolver branch that
calls construct_yolo_parser_config to include the legacy names
YOLODetectionParser, YOLOInstanceSegmentationParser, and
YOLOKeypointDetectionParser, ensuring archives using these names follow the
existing YOLO migration path instead of reaching _map_archive_parser_name and
raising NotImplementedError.
In `@luxonis_eval/engines/onnx_engine.py`:
- Around line 23-29: The ONNXEngineOutput.get method currently discards the
layout hint, allowing YOLO tensors to be interpreted with the wrong channel
axis. Update get to either normalize outputs to the requested layout or
explicitly reject any layout other than NCHW, while preserving existing behavior
when no layout is provided.
In `@luxonis_eval/metrics/dice_coef.py`:
- Around line 103-110: The conditional masking around the Dice and MIoU
calculations incorrectly treats the background as ignored when
include_background is false, removing foreground false positives. In
luxonis_eval/metrics/dice_coef.py lines 103-110 and luxonis_eval/metrics/mIoU.py
lines 104-111, remove this masking when target_bg is the true background class;
if target_bg denotes a void class, decouple masking from include_background and
pass it through the underlying metric’s ignore_index handling where supported.
In `@luxonis_eval/metrics/metrics_utils.py`:
- Around line 126-127: The 2D mask branch currently always returns binary_target
as false, so binary index masks bypass required normalization. Update the mask
handling logic around the 2D mask conversion to determine binary_target using
the metric’s num_classes configuration or the mask’s value range, ensuring 2D
masks containing only 0 and 1 are identified as binary while preserving
multiclass behavior.
In `@luxonis_eval/utils/depthai_nodes.py`:
- Around line 167-180: The class-count calculation in the
final_anchors/inferred_n_classes logic uses the number of heads instead of
anchors per output. Preserve anchors as a nested tensor, derive the per-head
anchor count from that structure, and use it when computing inferred_n_classes;
leave the no-anchors path unchanged.
---
Outside diff comments:
In `@luxonis_eval/config/resolver.py`:
- Around line 312-321: Extend the metadata extraction loop in the YOLO parser
configuration construction to include mask_conf, keypoint_label_names, and
keypoint_edges alongside the existing parameters. Ensure provided values from
NNArchive metadata are copied into params so YOLOExtendedParser receives them
instead of using defaults.
In `@luxonis_eval/engines/depthai_engine.py`:
- Around line 79-99: Update _load_nn_archive() to use the stored
self.nn_archive_cfg when constructing or loading dai.NNArchive, passing it
through the supported archive-loading API alongside model_path. If that API
cannot accept the configuration, remove nn_archive_cfg from __init__ and stop
storing it rather than leaving an unused option.
---
Nitpick comments:
In `@luxonis_eval/metrics/dice_coef.py`:
- Around line 112-115: Remove the redundant astype(np.int64) calls from both
torch.from_numpy conversions in luxonis_eval/metrics/dice_coef.py lines 112-115
and luxonis_eval/metrics/mIoU.py lines 113-116, passing pred_mask and
target_mask directly. The existing normalization functions already return int64
arrays, so preserve that dtype without creating per-batch copies.
In `@luxonis_eval/parsers/segmentation.py`:
- Around line 34-39: Rename the local variable assigned from
DepthAINodesSegmentationParser.compute in the segmentation parsing flow from
class_map to parsed_mask, and pass parsed_mask to create_segmentation_message.
Do not change the computation or message behavior.
🪄 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: b701c498-f70b-42a5-84bc-884a6c21cc8b
📒 Files selected for processing (28)
README.mdconfigs/classification_config.yamlconfigs/detection_config.yamlconfigs/pose_config.yamlconfigs/semantic_segmentation_config.yamlconfigs/yolov8n_inst_seg_config.yamlluxonis_eval/config/resolver.pyluxonis_eval/core/core.pyluxonis_eval/core/factories.pyluxonis_eval/core/runtime.pyluxonis_eval/engines/__init__.pyluxonis_eval/engines/base_engine.pyluxonis_eval/engines/depthai_engine.pyluxonis_eval/engines/io.pyluxonis_eval/engines/onnx_engine.pyluxonis_eval/metrics/dice_coef.pyluxonis_eval/metrics/mIoU.pyluxonis_eval/metrics/metrics_utils.pyluxonis_eval/parsers/__init__.pyluxonis_eval/parsers/base_parser.pyluxonis_eval/parsers/classification.pyluxonis_eval/parsers/detection.pyluxonis_eval/parsers/instance_seg.pyluxonis_eval/parsers/keypoint_detection.pyluxonis_eval/parsers/segmentation.pyluxonis_eval/parsers/semantic_seg.pyluxonis_eval/parsers/yolo.pyluxonis_eval/utils/depthai_nodes.py
💤 Files with no reviewable changes (4)
- luxonis_eval/parsers/keypoint_detection.py
- luxonis_eval/parsers/detection.py
- luxonis_eval/parsers/instance_seg.py
- luxonis_eval/parsers/semantic_seg.py
2 participants
Purpose
EngineOutput(parser-facing, now the parsers have no information regarding which backend is supplying them) andTensorSpec/ModelSpecfor resolved model I/O metadata.EngineOutputinstead of their raw data they were previously returning (raw tensors ordai.NNData) and new implemented engines have to adapt this.EngineOutput+ModelSpec: no longer dependent on engine-specific raw output typesluxonis-evalparsers call thecompute()function of parsers indepthai-nodesinstead of hand-written computation logicSet to draft: 1) depends on depthai-nodes PR and 2) I need to generate different models by training them with
LuxonisTrain, converting them to ONNX NNArchive and RVC4 NNArchive (FP16) and making sure the results are coherent betweenluxonis_train test ...andluxonis_eval eval .... Although this PR is supposed to only be architectural shouldn't change anything functionally, this is just to make sure since the computation now relies on the depthai-nodes parsers'compute. Then we can use these .ckpt/.onnx.tar.xz/.rvc4.tar.xz files later on for automatic E2E/regression testing.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
New Features
Bug Fixes
Documentation