Binary classifier#23
Conversation
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
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis 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. ChangesBinary Classifier Feature and Mapping Integration
Tensor Detector Refactoring and Modularization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (7)
riptide_mapping/config/binary_classifier.yamlriptide_mapping/config/config.yamlriptide_mapping/config/dummy_detections.yamlriptide_mapping/launch/mapping.launch.pyriptide_mapping/riptide_mapping2/binary_classifier.pyriptide_mapping/riptide_mapping2/dummydetections.pyriptide_mapping/riptide_mapping2/mapping.py
Changed image subscription topic to compressed. Published image topic name change, also compressed. Performance improvement with SVD. Not using outlier removal for now, commented out (performance hit). Marker QOL improvements.
zehdari
left a comment
There was a problem hiding this comment.
Tested Tirpak test, big W
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
riptide_mapping/riptide_mapping2/binary_classifier.py (1)
117-128: ⚡ Quick winValidate 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 assigningself.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 valueStyle: 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 winAdd
CONFIGURE_DEPENDSto avoid stale Python executable installs.Current globbing may miss newly added
src/*.pyfiles 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 winNarrow 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
📒 Files selected for processing (21)
.gitignore.gitmodulesriptide_mapping/config/binary_classifier.yamlriptide_mapping/config/config.yamlriptide_mapping/config/dummy_detections.yamlriptide_mapping/launch/mapping.launch.pyriptide_mapping/riptide_mapping2/binary_classifier.pyriptide_mapping/riptide_mapping2/dummydetections.pyriptide_mapping/riptide_mapping2/mapping.pytensor_detector/CMakeLists.txttensor_detector/config/yolo_orientation.yamltensor_detector/launch/tensorrt.launch.pytensor_detector/src/colors.pytensor_detector/src/detection.pytensor_detector/src/geometry.pytensor_detector/src/outputs.pytensor_detector/src/pointcloud.pytensor_detector/src/yolo_model.pytensor_detector/src/yolo_orientation.pytensor_detector/weights/dfc_rs_26.pttensor_detector/weights/ffc_rs_26.pt
💤 Files with no reviewable changes (1)
- .gitmodules
| objects: | ||
| - gate | ||
| - gate_shark | ||
| - gate_saw | ||
| - slalom_front | ||
| - slalom_middle | ||
| - slalom_back | ||
| - torpedo | ||
| - torpedo_shark_hole | ||
| - torpedo_sawfish_hole | ||
| - bin_target |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@AdityaGupta0 you can fix this one for tree testing
There was a problem hiding this comment.
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!
| for object in self.objects: | ||
| self.pubs.append(self.create_publisher(PoseWithCovarianceStamped, f"dummydetections/{object}", 10)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ruff check riptide_mapping/riptide_mapping2/dummydetections.py --select A001Repository: 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
There was a problem hiding this comment.
@AdityaGupta0 you can do this when making dummy dets changes as well, not like we use a linter yet anyways lol
There was a problem hiding this comment.
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!
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
bin,bin_target1,bin_target2).use_sim_timesupport.