Skip to content

Binary classifier#23

Open
zehdari wants to merge 22 commits into
devfrom
BinaryClassifier
Open

Binary classifier#23
zehdari wants to merge 22 commits into
devfrom
BinaryClassifier

Conversation

@zehdari

@zehdari zehdari commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • New Features
    • Added binary-classifier-driven mapping with two-instance clustering/locking and mapping updates based on the selected instance.
    • Added services to start, freeze, start the second instance, and stop the classifier.
    • Expanded 3D vision post-processing (including slalom, gate pairing, and torpedo separation) with generated markers and point clouds.
  • Configuration Updates
    • Added a dedicated binary-classifier parameter set and expanded mapping targets (including bin, bin_target1, bin_target2).
    • Updated initial frame/object definitions and improved dummy-detection coverage (including new blood objects and orientation-invalid handling).
    • Updated YOLO orientation model/label mappings and added use_sim_time support.

zehdari and others added 5 commits June 7, 2026 13:37
Added binary classifier to split between 2 instances of a class. Updated dummy dets to support this behavior, and moved list to the config. Mapping now supports binary classifier. Normal behavior when binary classifier is not active. Mapping config yaml now has optional lock_orientation_to_config for object (ie slalom). Added reset mapping service so we don't have to restart mapping to clear previous dets/locations before starting a new run. Tested on multiple objects in sim, but not with autonomy
@zehdari

zehdari commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a9c7d52d-3ddd-4078-abeb-06ebb8499b78

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces a complete binary classifier feature for two-instance detection tracking alongside a significant refactoring of the tensor detector into modular utilities and processors. The mapping node gains ROS services to control classifier lifecycle, with detections routed through either the classifier or standard closest-object logic. Configuration is updated to support dynamic dummy detection objects and restructured target frames. The tensor detector transitions from a monolithic node to a layered architecture with separate geometry, output formatting, point cloud accumulation, and detection processing modules.

Changes

Binary Classifier Feature and Mapping Integration

Layer / File(s) Summary
Binary Classifier Configuration and Data Contracts
riptide_mapping/config/binary_classifier.yaml, riptide_mapping/riptide_mapping2/binary_classifier.py (lines 1–65)
New YAML schema defines tuning parameters for buffer size/TTL, clustering radii, assignment/exclusion gates, centroid EMA smoothing, and 2D/3D distance toggle. Public dataclasses encode BinaryClassifierParams with all tuning fields, DetectionSample with 3D point conversion, AssignmentResult with acceptance/reason/distance, and BinaryClassifierState lifecycle constants.
Binary Classifier State Machine and Core Operations
riptide_mapping/riptide_mapping2/binary_classifier.py (lines 66–445)
Stateful classifier implementing initialization, lifecycle methods (start(), freeze_buffer(), start_second(), stop()), main observe() dispatcher routing samples across acquisition/tracking states, greedy centroid clustering with neighbor density and quantile-based tail trimming, EMA centroid updates, distance gating, spatial separation enforcement, TTL-based buffer pruning respecting FROZEN mode, and 2D/3D distance metrics.
Target Frame Hierarchy and Configuration Updates
riptide_mapping/config/config.yaml
Restructures bin targets: bin becomes parent with bin_target1 and bin_target2 as children. Renames gate frames (gate_sharkgate_rescue, gate_sawgate_repair) and torpedo holes (torpedo_shark_holefire_hole_large, torpedo_sawfish_holefire_hole_small). Adds lock_orientation_to_config: true to all slalom frames. Replaces table_* entries with named frames (pill, bandage, nut_and_bolt, plug, helmet, warning, compass, hammer_and_wrench, buoy, sos).
Dummy Detection Object Configuration and Parameterization
riptide_mapping/config/dummy_detections.yaml, riptide_mapping/riptide_mapping2/dummydetections.py
Declares enumerated objects parameter list in YAML; updates slalom_front invalid orientation to true; adds blood_1 and blood_2 detections. Removes hardcoded objects list from Python module; now declares objects as STRING_ARRAY ROS parameter and dynamically generates per-object parameter declarations (class_id, pose, noise, visibility, orientation flags, distance bounds). Timer callback reads per-object config and uses configured class_id instead of object name.
Launch Script and Mapping Node Integration
riptide_mapping/launch/mapping.launch.py, riptide_mapping/riptide_mapping2/mapping.py
Launch script adds binary_classifier_yaml argument and wires both config and binary classifier YAML files to mapping node parameters. Mapping node imports classifier/DetectionSample/service types, extends objects set with bin targets, expands downwards_objects with new classes, declares per-object lock_orientation_to_config parameter. Constructor initializes BinaryClassifier, creates ROS service callbacks (start/freeze/start_second/stop), implements runtime reset_runtime_state() to clear classifier and state, and centralizes LED pulsing via pulse_detection_led(). vision_callback routes detections to classifier handler when active; standard mapping otherwise. Pose-transform logic refactored into transform_detection_to_object_parent() helper; try_update_pose() now delegates to helper; new try_update_binary_classifier_pose() updates classifier buffer and assigned target poses.

Tensor Detector Refactoring and Modularization

Layer / File(s) Summary
Model Configuration and Build Updates
tensor_detector/config/yolo_orientation.yaml, tensor_detector/launch/tensorrt.launch.py, tensor_detector/CMakeLists.txt
Updates FFC model to ffc_rs_26.pt with new class ID map (8 classes); updates DFC model to dfc_rs_26.pt with new class ID map (8 classes). Adds use_sim_time launch parameter forwarded to node. Replaces hardcoded yolo_orientation.py installation with glob-based install of all src/*.py files.
Geometry Utilities Module
tensor_detector/src/geometry.py
Pure-geometry utilities providing pixel-to-3D back-projection, ray-plane intersection, SVD-based least-squares plane fitting, rotation/quaternion conversions for plane normals, in-plane basis construction, 2D bbox containment testing, and statistical/radius-based outlier filtering.
Output and Visualization Utilities
tensor_detector/src/colors.py, tensor_detector/src/outputs.py
COLOR_MAP constant for class-to-RGB visualization. ROS message constructors: new_detection() creates Detection3D with header; make_hypothesis() builds ObjectHypothesisWithPose with position/orientation/confidence. MarkerBuilder generates visualization markers with configurable ID counter, lifetime, optional cube markers (with flat depth scaling), and arrow markers with quaternion-based orientation.
Point Cloud Accumulation and Rendering
tensor_detector/src/pointcloud.py
PointCloudBuilder for back-projecting masked pixels to 3D using depth and camera intrinsics, filtering via bounds/depth/mask/outliers, accumulating points (capped at MAX_POINTS), rendering ROS PointCloud2 messages with float32 serialization, and providing optional image overlay visualization with error handling.
YOLO Model Wrapper
tensor_detector/src/yolo_model.py
Minimal wrapper around Ultralytics YOLO for segmentation with model loading, TensorRT export preference (checks for .engine files), and unified infer() interface with confidence/IoU parameters.
Detection 3D Processing and Instance Routing
tensor_detector/src/detection.py
Complete post-processing pipeline converting YOLO results and camera data into Detection3DArray messages. Builds ROI mask from instance segmentation, routes detections by class with special handling: slalom uses depth sampling at bbox center plus temporal history and optional TF-based orientation; gate pairs unions bboxes and fits shared plane when both members present; torpedo fits board/symbol planes, projects circle centers, computes center/confidence, and separates large/small holes by 3D proximity. Includes shared geometry workflow (bbox shrinking, grid sampling, plane fitting via SVD, camera-facing quaternions).
YOLO Node Refactoring and Lifecycle Management
tensor_detector/src/yolo_orientation.py
Replaces monolithic implementation with delegation to DetectionProcessor and YoloModel. Refactored initialization parameterizes camera selection, model paths, thresholds, IOU, visualization/processing tuning; validates cloud color mode; initializes publishers, CvBridge, TF buffer/listener. Simplified camera/depth lifecycle. Per-image callback validates prerequisites (depth + intrinsics), runs inference, constructs Frame bundle, calls processor, conditionally publishes markers/point cloud/annotated image/Detection3DArray based on subscribers; preserves optional processing-time logging.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~80 minutes

Poem

🐰 Two instances now dance in clustering light,
With gates and gating to get each one right,
The tensor detector shed its old ways—
Geometry, colors, point clouds blaze,
Modular vision that hops with delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.83% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Binary classifier' directly summarizes the main feature added in this PR—a complete binary classifier implementation that distinguishes between two instances of a class.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch BinaryClassifier

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@riptide_mapping/riptide_mapping2/binary_classifier.py`:
- Around line 104-109: The declare_params method dereferences self.node even
though BinaryClassifier.__init__ permits node=None; update_params already guards
for None, so make declare_params mirror that by returning early if self.node is
None. Modify declare_params to check "if self.node is None: return" before
calling self.node.declare_parameters, keeping use of self.param_name and
DEFAULTS unchanged so declare_params safely no-ops when constructed without a
node.
- Around line 435-436: buffer_size() currently accesses a non-existent attribute
self.samples.c on the deque which raises AttributeError; change the
implementation in buffer_size() to return the deque length via len(self.samples)
so it reports the buffer length correctly (locate the buffer_size method and
replace the self.samples.c reference with len(self.samples)).

In `@riptide_mapping/riptide_mapping2/mapping.py`:
- Around line 318-323: The buffered centroid in
binary_classifier.instance2_centroid is expressed in the classifier's parent
frame (the same parent used for instance1_name via
try_update_binary_classifier_pose), but seed_object_estimate() expects map-frame
coordinates; when binary_classifier.second_locked is true you must transform
instance2_centroid from the classifier parent frame into the map frame before
calling seed_object_estimate() (or alternatively extend seed_object_estimate()
to accept a source frame and perform the transform internally). Update the block
that checks binary_classifier.second_locked to obtain the classifier parent
frame (the parent used by try_update_binary_classifier_pose/instance1_name), use
the existing TF lookup/transform utility to convert instance2_centroid to map
coordinates, then call seed_object_estimate(binary_classifier.instance2_name,
transformed_centroid) and publish_pose().
🪄 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 Plus

Run ID: f62fe6c6-1209-4f1f-9661-9123a43390cb

📥 Commits

Reviewing files that changed from the base of the PR and between 9c1b2de and d7faaa2.

📒 Files selected for processing (7)
  • riptide_mapping/config/binary_classifier.yaml
  • riptide_mapping/config/config.yaml
  • riptide_mapping/config/dummy_detections.yaml
  • riptide_mapping/launch/mapping.launch.py
  • riptide_mapping/riptide_mapping2/binary_classifier.py
  • riptide_mapping/riptide_mapping2/dummydetections.py
  • riptide_mapping/riptide_mapping2/mapping.py

Comment thread riptide_mapping/riptide_mapping2/binary_classifier.py
Comment thread riptide_mapping/riptide_mapping2/binary_classifier.py Outdated
Comment thread riptide_mapping/riptide_mapping2/mapping.py

@zehdari zehdari left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested Tirpak test, big W

@zehdari

zehdari commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
riptide_mapping/riptide_mapping2/binary_classifier.py (1)

117-128: ⚡ Quick win

Validate runtime parameter bounds in update_params().

Line 117-128 currently accepts any override value. Out-of-range values (for example EMA outside [0,1], non-positive radii/gates, or negative TTL) can destabilize tracking or cause systematic rejection. Please validate/clamp before assigning self.params.

Proposed patch
     def update_params(self):
         if self.node is None:
             return

-        self.params = BinaryClassifierParams(
+        new_params = BinaryClassifierParams(
             buffer_size=int(self.node.get_parameter(self.param_name("buffer_size")).value),
             buffer_ttl_sec=float(self.node.get_parameter(self.param_name("buffer_ttl_sec")).value),
             min_cluster_detections=int(self.node.get_parameter(self.param_name("min_cluster_detections")).value),
             cluster_radius_m=float(self.node.get_parameter(self.param_name("cluster_radius_m")).value),
             assignment_gate_m=float(self.node.get_parameter(self.param_name("assignment_gate_m")).value),
             exclusion_radius_m=float(self.node.get_parameter(self.param_name("exclusion_radius_m")).value),
             min_instance_separation_m=float(self.node.get_parameter(self.param_name("min_instance_separation_m")).value),
             max_cluster_variance_m2=float(self.node.get_parameter(self.param_name("max_cluster_variance_m2")).value),
             centroid_ema_alpha=float(self.node.get_parameter(self.param_name("centroid_ema_alpha")).value),
             use_3d_distance=bool(self.node.get_parameter(self.param_name("use_3d_distance")).value),
         )
+
+        new_params.buffer_size = max(1, int(new_params.buffer_size))
+        new_params.min_cluster_detections = max(1, int(new_params.min_cluster_detections))
+        new_params.buffer_ttl_sec = max(0.0, float(new_params.buffer_ttl_sec))
+        new_params.cluster_radius_m = max(1e-6, float(new_params.cluster_radius_m))
+        new_params.assignment_gate_m = max(1e-6, float(new_params.assignment_gate_m))
+        new_params.exclusion_radius_m = max(0.0, float(new_params.exclusion_radius_m))
+        new_params.min_instance_separation_m = max(0.0, float(new_params.min_instance_separation_m))
+        new_params.centroid_ema_alpha = min(1.0, max(0.0, float(new_params.centroid_ema_alpha)))
+
+        self.params = new_params

         # re-wrap the existing samples in case buffer_size changed
         self.samples = deque(self.samples, maxlen=max(1, self.params.buffer_size))
🤖 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 `@riptide_mapping/riptide_mapping2/binary_classifier.py` around lines 117 -
128, In the update_params() method where BinaryClassifierParams is instantiated,
add validation and clamping logic before assigning parameter values to
self.params. Validate that centroid_ema_alpha is within the range [0, 1], ensure
all distance/radius/gate parameters (cluster_radius_m, assignment_gate_m,
exclusion_radius_m, min_instance_separation_m) are positive, verify
buffer_ttl_sec and max_cluster_variance_m2 are non-negative, and confirm
buffer_size and min_cluster_detections are positive integers. Either clamp
out-of-range values to acceptable bounds or raise appropriate errors to prevent
invalid configurations that could destabilize tracking behavior.
tensor_detector/src/detection.py (1)

493-499: 💤 Low value

Style: avoid multiple statements on one line.

The semicolons make this line harder to read. Split into separate statements for clarity.

♻️ Suggested refactor
     def _union_bbox(self, boxes):
         """Axis-aligned union of several boxes"""
         xs0, ys0, xs1, ys1 = [], [], [], []
         for box in boxes:
             x_min, y_min, x_max, y_max = map(int, box.xyxy[0])
-            xs0.append(x_min); ys0.append(y_min); xs1.append(x_max); ys1.append(y_max)
+            xs0.append(x_min)
+            ys0.append(y_min)
+            xs1.append(x_max)
+            ys1.append(y_max)
         return (min(xs0), min(ys0), max(xs1), max(ys1))
🤖 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 `@tensor_detector/src/detection.py` around lines 493 - 499, The _union_bbox
method contains multiple append statements on a single line separated by
semicolons, which reduces readability. Split the line `xs0.append(x_min);
ys0.append(y_min); xs1.append(x_max); ys1.append(y_max)` into four separate
lines, each containing one append statement on its own line. This will improve
code clarity and follow Python style conventions against multiple statements on
one line.

Source: Linters/SAST tools

tensor_detector/CMakeLists.txt (1)

41-43: ⚡ Quick win

Add CONFIGURE_DEPENDS to avoid stale Python executable installs.

Current globbing may miss newly added src/*.py files in incremental builds unless CMake is reconfigured manually.

Proposed fix
-file(GLOB src_py_files RELATIVE ${PROJECT_SOURCE_DIR} src/*.py)
+file(GLOB src_py_files CONFIGURE_DEPENDS RELATIVE ${PROJECT_SOURCE_DIR} src/*.py)
🤖 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 `@tensor_detector/CMakeLists.txt` around lines 41 - 43, The file(GLOB
src_py_files RELATIVE ${PROJECT_SOURCE_DIR} src/*.py) command is missing the
CONFIGURE_DEPENDS keyword, which causes CMake to not automatically reconfigure
when new Python files are added to the src directory during incremental builds.
Add the CONFIGURE_DEPENDS keyword to the file(GLOB) command to ensure that the
glob pattern is re-evaluated whenever files matching src/*.py change, preventing
stale installs of Python executables.
tensor_detector/src/yolo_orientation.py (1)

227-230: ⚡ Quick win

Narrow exception handling in subscription teardown.

Line 229 catches Exception, which can mask unrelated errors and make teardown faults hard to diagnose. Catch only the attribute-read failure path.

Proposed fix
-                except Exception:
+                except AttributeError:
                     topic = "unknown"
🤖 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 `@tensor_detector/src/yolo_orientation.py` around lines 227 - 230, In the
subscription teardown try-except block where sub.topic_name is accessed, the
except clause catches the overly broad Exception type which masks unrelated
errors and complicates debugging. Replace the except Exception catch with a more
specific exception type that only targets the attribute-read failure,
specifically except AttributeError, which is the exception raised when accessing
a non-existent attribute on an object.

Source: Linters/SAST tools

🤖 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 `@riptide_mapping/config/dummy_detections.yaml`:
- Around line 4-14: The objects list in the YAML file starting at line 4
contains legacy detection class IDs that no longer match the renamed IDs
expected by mapping.py. Update all object entries in the objects list to use the
new naming convention: replace gate with gate_rescue, gate_shark and gate_saw
with gate_repair, replace slalom entries with fire_hole_large and
fire_hole_small as appropriate, replace torpedo and its variants with the
corresponding fire hole names, and update bin_target to bin_target1 and
bin_target2. This same update must be applied to all object entries throughout
the file, including the additional entries mentioned at lines 33-104, so that
the dummy detections are recognized by mapping.py and not skipped as unknown.

In `@riptide_mapping/riptide_mapping2/dummydetections.py`:
- Around line 55-56: Rename the loop variable `object` to a different name (such
as `obj`, `item`, or `detection`) in the for loop on line 55 where it iterates
over self.objects. Update all references to this variable within the loop body
to use the new name, including the reference in the f-string passed to
create_publisher. Also apply the same fix to the similar loop mentioned at line
79 to resolve the Ruff A001 lint violation.

In `@riptide_mapping/riptide_mapping2/mapping.py`:
- Around line 545-546: The downward-orientation normalization at line 545 is
incorrectly checking result.hypothesis.class_id instead of the assigned mapping
target. In binary-classifier mode, the detection class can differ from the
assigned target (for example, detection class 'blood' versus assigned target
'bin_target1'), which causes the downward normalization condition to be skipped.
Replace the check on result.hypothesis.class_id in the downwards_objects.keys()
comparison with a check on the actual assigned mapping target value. Apply this
same fix to the other similar checks in the same function around lines 602-654
where the same pattern occurs.

In `@tensor_detector/src/geometry.py`:
- Around line 49-60: The rotation_from_normal function incorrectly handles the
case when axis_length equals zero by always applying a 180-degree rotation. When
the cross product is zero, you must distinguish between two cases: when normal
equals default_normal (same direction, requiring identity rotation with angle 0)
versus when normal equals the negative of default_normal (requiring 180-degree
rotation). Check the dot product between default_normal and normal within the
axis_length == 0 block - if positive, set angle to 0; if negative, set angle to
pi. Additionally, wrap the dot product argument to arccos with np.clip(..., -1,
1) to avoid numerical precision errors that could cause arccos to fail when the
dot product is slightly outside the valid range.

In `@tensor_detector/src/yolo_model.py`:
- Around line 15-16: The engine_model_path derivation on line 15 uses the string
replace method which will replace all occurrences of '.pt' in the path,
potentially corrupting path segments that contain '.pt' earlier in the directory
structure. Replace the replace method call with a suffix-safe approach that only
substitutes the file extension at the end of the path, such as using pathlib's
Path.with_suffix() method or manually extracting the path without the extension
before appending the new suffix.

In `@tensor_detector/src/yolo_orientation.py`:
- Around line 271-275: The `_stamp()` method (defined at lines 271-275)
correctly respects the `use_incoming_timestamp` parameter to return either the
incoming message timestamp or the current clock time, but this method is not
being used consistently across all message building operations. In the detection
frame building code around lines 295-302, `msg.header.stamp` is hardcoded
directly instead of calling `self._stamp(msg)`, which causes the parameterized
timestamp behavior to be bypassed for detections while other outputs like
point-cloud stamping correctly use the `_stamp()` method. Replace all hardcoded
`msg.header.stamp` references in the detection frame building section (around
lines 295-302) with calls to `self._stamp(msg)` to ensure consistent timestamp
handling across all outputs.

---

Nitpick comments:
In `@riptide_mapping/riptide_mapping2/binary_classifier.py`:
- Around line 117-128: In the update_params() method where
BinaryClassifierParams is instantiated, add validation and clamping logic before
assigning parameter values to self.params. Validate that centroid_ema_alpha is
within the range [0, 1], ensure all distance/radius/gate parameters
(cluster_radius_m, assignment_gate_m, exclusion_radius_m,
min_instance_separation_m) are positive, verify buffer_ttl_sec and
max_cluster_variance_m2 are non-negative, and confirm buffer_size and
min_cluster_detections are positive integers. Either clamp out-of-range values
to acceptable bounds or raise appropriate errors to prevent invalid
configurations that could destabilize tracking behavior.

In `@tensor_detector/CMakeLists.txt`:
- Around line 41-43: The file(GLOB src_py_files RELATIVE ${PROJECT_SOURCE_DIR}
src/*.py) command is missing the CONFIGURE_DEPENDS keyword, which causes CMake
to not automatically reconfigure when new Python files are added to the src
directory during incremental builds. Add the CONFIGURE_DEPENDS keyword to the
file(GLOB) command to ensure that the glob pattern is re-evaluated whenever
files matching src/*.py change, preventing stale installs of Python executables.

In `@tensor_detector/src/detection.py`:
- Around line 493-499: The _union_bbox method contains multiple append
statements on a single line separated by semicolons, which reduces readability.
Split the line `xs0.append(x_min); ys0.append(y_min); xs1.append(x_max);
ys1.append(y_max)` into four separate lines, each containing one append
statement on its own line. This will improve code clarity and follow Python
style conventions against multiple statements on one line.

In `@tensor_detector/src/yolo_orientation.py`:
- Around line 227-230: In the subscription teardown try-except block where
sub.topic_name is accessed, the except clause catches the overly broad Exception
type which masks unrelated errors and complicates debugging. Replace the except
Exception catch with a more specific exception type that only targets the
attribute-read failure, specifically except AttributeError, which is the
exception raised when accessing a non-existent attribute on an object.
🪄 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 Plus

Run ID: a074ec82-3fe0-4c9b-9f3e-f288b0e2ba80

📥 Commits

Reviewing files that changed from the base of the PR and between 9c1b2de and e823e27.

📒 Files selected for processing (21)
  • .gitignore
  • .gitmodules
  • riptide_mapping/config/binary_classifier.yaml
  • riptide_mapping/config/config.yaml
  • riptide_mapping/config/dummy_detections.yaml
  • riptide_mapping/launch/mapping.launch.py
  • riptide_mapping/riptide_mapping2/binary_classifier.py
  • riptide_mapping/riptide_mapping2/dummydetections.py
  • riptide_mapping/riptide_mapping2/mapping.py
  • tensor_detector/CMakeLists.txt
  • tensor_detector/config/yolo_orientation.yaml
  • tensor_detector/launch/tensorrt.launch.py
  • tensor_detector/src/colors.py
  • tensor_detector/src/detection.py
  • tensor_detector/src/geometry.py
  • tensor_detector/src/outputs.py
  • tensor_detector/src/pointcloud.py
  • tensor_detector/src/yolo_model.py
  • tensor_detector/src/yolo_orientation.py
  • tensor_detector/weights/dfc_rs_26.pt
  • tensor_detector/weights/ffc_rs_26.pt
💤 Files with no reviewable changes (1)
  • .gitmodules

Comment on lines +4 to +14
objects:
- gate
- gate_shark
- gate_saw
- slalom_front
- slalom_middle
- slalom_back
- torpedo
- torpedo_shark_hole
- torpedo_sawfish_hole
- bin_target

@coderabbitai coderabbitai Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Dummy detection class IDs are out of sync with mapping target names.

Line 4 onward still uses legacy object IDs, and those entries publish the same class IDs by default. mapping.py now accepts renamed IDs (gate_rescue, gate_repair, fire_hole_large, fire_hole_small, bin_target1/2), so these simulated detections are treated as unknown and skipped.

🔧 Minimal aliasing fix in YAML
       gate_shark:
+        class_id: gate_rescue
         pose: [2.0, 0.5, -0.8, 0.0, 0.0, 3.1415]
@@
       gate_saw:
+        class_id: gate_repair
         pose: [2.0, -0.5, -0.8, 0.0, 0.0, 3.1415]
@@
       torpedo_shark_hole:
+        class_id: fire_hole_large
         pose: [12.0, -3.970028, -2.782576, 0.0, 0.0, 3.1415]
@@
       torpedo_sawfish_hole:
+        class_id: fire_hole_small
         pose: [12.0, 4.11176, -3.0889, 0.0, 0.0, 3.1415]
@@
       bin_target:
+        class_id: bin_target1
         pose: [11.0, -6.0, -3.25, 0.0, 0.0, 0.0]

Also applies to: 33-104

🤖 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 `@riptide_mapping/config/dummy_detections.yaml` around lines 4 - 14, The
objects list in the YAML file starting at line 4 contains legacy detection class
IDs that no longer match the renamed IDs expected by mapping.py. Update all
object entries in the objects list to use the new naming convention: replace
gate with gate_rescue, gate_shark and gate_saw with gate_repair, replace slalom
entries with fire_hole_large and fire_hole_small as appropriate, replace torpedo
and its variants with the corresponding fire hole names, and update bin_target
to bin_target1 and bin_target2. This same update must be applied to all object
entries throughout the file, including the additional entries mentioned at lines
33-104, so that the dummy detections are recognized by mapping.py and not
skipped as unknown.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdityaGupta0 you can fix this one for tree testing

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Comment on lines +55 to 56
for object in self.objects:
self.pubs.append(self.create_publisher(PoseWithCovarianceStamped, f"dummydetections/{object}", 10))

@coderabbitai coderabbitai Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ruff check riptide_mapping/riptide_mapping2/dummydetections.py --select A001

Repository: osu-uwrt/riptide_perception

Length of output: 1268


Avoid shadowing Python builtins with loop variable object.

Using object as a loop variable (lines 55 and 79) triggers Ruff A001 and can block lint checks.

♻️ Rename loop variable
-        for object in self.objects:
-            self.pubs.append(self.create_publisher(PoseWithCovarianceStamped, f"dummydetections/{object}", 10))
+        for object_name in self.objects:
+            self.pubs.append(self.create_publisher(PoseWithCovarianceStamped, f"dummydetections/{object_name}", 10))
@@
-        for object in self.objects:
-            self.declare_parameter(f"detection_data.{object}.class_id", object)
-            self.declare_parameter(f"detection_data.{object}.pose", [0.0, 0.0, 0.0, 0.0, 0.0, 0.0])
-            self.declare_parameter(f"detection_data.{object}.noise", 0.0)
-            self.declare_parameter(f"detection_data.{object}.score", 0.0)
-            self.declare_parameter(f"detection_data.{object}.downward", False)
-            self.declare_parameter(f"detection_data.{object}.publish_invalid_orientation", False)
-            self.declare_parameter(f"detection_data.{object}.pub_invalid_orientation", False)
-            self.declare_parameter(f"detection_data.{object}.min_dist", 0.0)
-            self.declare_parameter(f"detection_data.{object}.max_dist", 0.0)
+        for object_name in self.objects:
+            self.declare_parameter(f"detection_data.{object_name}.class_id", object_name)
+            self.declare_parameter(f"detection_data.{object_name}.pose", [0.0, 0.0, 0.0, 0.0, 0.0, 0.0])
+            self.declare_parameter(f"detection_data.{object_name}.noise", 0.0)
+            self.declare_parameter(f"detection_data.{object_name}.score", 0.0)
+            self.declare_parameter(f"detection_data.{object_name}.downward", False)
+            self.declare_parameter(f"detection_data.{object_name}.publish_invalid_orientation", False)
+            self.declare_parameter(f"detection_data.{object_name}.pub_invalid_orientation", False)
+            self.declare_parameter(f"detection_data.{object_name}.min_dist", 0.0)
+            self.declare_parameter(f"detection_data.{object_name}.max_dist", 0.0)
🧰 Tools
🪛 Ruff (0.15.17)

[error] 55-55: Variable object is shadowing a Python builtin

(A001)

🤖 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 `@riptide_mapping/riptide_mapping2/dummydetections.py` around lines 55 - 56,
Rename the loop variable `object` to a different name (such as `obj`, `item`, or
`detection`) in the for loop on line 55 where it iterates over self.objects.
Update all references to this variable within the loop body to use the new name,
including the reference in the f-string passed to create_publisher. Also apply
the same fix to the similar loop mentioned at line 79 to resolve the Ruff A001
lint violation.

Source: Linters/SAST tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdityaGupta0 you can do this when making dummy dets changes as well, not like we use a linter yet anyways lol

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Comment thread riptide_mapping/riptide_mapping2/mapping.py
Comment thread tensor_detector/src/geometry.py
Comment thread tensor_detector/src/yolo_model.py Outdated
Comment thread tensor_detector/src/yolo_orientation.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants