Run the dilation filter regardless of the traversability filter - #4
Conversation
update_map_with_kernel ends with update_normal(self.traversability_input), but the dilation that fills that buffer sat inside the `if self.traversability_filter is not None:` guard added when the filter was made optional. The buffer is not the filter's private input: it holds the dilated upper-bound surface, which update_normal consumes too (hence its `dilated_map` parameter name). When the filter fails to load -- no weights.dat, or torch missing or not CUDA-capable -- traversability_input therefore kept the all-zero contents from compile_kernels(). update_normal still ran, computing normals over a flat zero plane, so normal_x/normal_y/normal_z silently became (0, 0, 1) everywhere regardless of terrain. Nothing surfaced it: the only log line mentions the traversability filter, and (0, 0, 1) is a plausible normal -- on flat ground it is even correct. The dilation does not depend on the learned weights, and upstream runs it unconditionally, so move it back out of the guard and keep only the traversability_filter() call and its elevation_map[3] write behind it. Measured on a 15 degree ramp with the filter disabled: normal tilt was -0.0 deg before, ~15 deg after.
The two normals tests skipped whenever the traversability filter was unavailable, because the normal layers were fed from traversability_input, which only got filled inside the filter's guard -- so with the filter off every normal was (0, 0, 1) and the tests would have asserted against known-bad output. That is fixed in #4 (the dilation now runs unconditionally), so the skip guard has nothing left to protect against: the normals are correct with the filter enabled or disabled. Drop _require_working_normals and let both tests run in either configuration, which turns them into the regression test for #4 -- the ramp test is the one that discriminates, reading -0.0 deg of tilt against the bug and ~15 deg once fixed. test_traversability_layer keeps its skip; it genuinely needs the learned weights. Update the README section that documented this as a live defect.
There was a problem hiding this comment.
Pull request overview
This PR fixes an implicit dependency between the optional traversability filter and normal-layer computation by ensuring the upper-bound dilation step always runs, so update_normal(...) receives a correctly populated dilated surface even when the learned filter is unavailable.
Changes:
- Run the dilation filter unconditionally to populate
traversability_inputfor downstream consumers (including normals). - Keep the learned
traversability_filter(...)invocation and traversability-layer write guarded behindself.traversability_filter is not None.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The harness in #3 covers this, but sim/ is not on dev yet, so nothing here would catch the dilation moving back inside the filter's guard. Drive the real input path with weight_file="" -- the supported way to run without the learned filter, and what a host with no CUDA-capable torch falls back to -- over a synthetic 15 degree plane, and assert the normal layers recover the slope. Against the bug the dilated surface stays all zeros and the recovered tilt reads -0.0 deg instead of ~15 deg, so the slope test fails; with the fix it passes. The flat-ground case cannot discriminate on its own, since (0, 0, 1) is the right answer there, but it pins the sign convention that makes the slope number meaningful. No new dependencies: the plane is generated directly, so this runs anywhere the existing cupy tests do. Deliberately not asserting that the enabled and disabled configurations agree cell-for-cell. add_points_kernel accumulates with atomics, so upper_bound depends on the order points land and two identical runs already differ by up to 0.29 in normal_x -- such a test would measure determinism, not this fix.
|
Good catch on the missing regression test — that was a real gap. Added Verified it actually discriminates, by reverting the source fix and re-running:
The flat-ground case passing in both columns is expected, and worth being explicit about: One thing I tried and deliberately dropped: asserting the filter-enabled and filter-disabled configurations produce identical normals. That looks like the tightest possible check, but Full unit suite on this branch: 90 passed. |
The problem
update_map_with_kernelends with:but the dilation that fills
traversability_inputsits inside theif self.traversability_filter is not None:guard immediately above it. That buffer is not the filter's private input — it holds the dilated upper-bound surface, andupdate_normalconsumes it too.update_normal's parameter is nameddilated_map, which is the honest name for it.So when the traversability filter fails to load —
weight_fileempty,weights.datmissing, or torch absent / not CUDA-capable —traversability_inputkeeps the all-zero contents it got fromcompile_kernels().update_normalstill runs, because it was never guarded, and computes normals over a flat zero plane. Every cell comes out(0, 0, 1)regardless of terrain.Nothing surfaces this. The only log line mentions the traversability filter, giving no hint the normal layers went with it, and
(0, 0, 1)is a plausible normal — on flat ground it is even the correct answer. Sonormal_x/normal_y/normal_zare indistinguishable from genuinely flat terrain.The fix
The dilation is plain CUDA arithmetic with no dependency on the learned weights, so it moves back out of the guard; only the
traversability_filter(...)call and itselevation_map[3]write stay behind it.This restores upstream behaviour rather than inventing new semantics — two independent confirmations:
git log -Lon those lines shows the block arrived unconditional in0f2f505(port fromrelease/jazzy); the guard was added locally inb496fd4when the filter was made optional.leggedrobotics/elevation_mapping_cupymainstill runs the dilation unconditionally, under the comment# dilation before traversability_filter— i.e. the dilation is a precondition of the filter, not a part of it.Verification
Measured with the MuJoCo harness from #3, forcing the broken path via
weight_file="":-0.0°The flat-ground case passes even with the bug, since
(0, 0, 1)is genuinely right there; the ramp is what discriminates, and-0.0°is the unmistakable signature of normals taken over a zero plane.With the filter enabled (the normal configuration) behaviour is unchanged:
elevation_mapping_cupy/tests87 passed, and the full harness suite 107 passed.Note on sequencing
The regression test for this lives in
sim/tests/test_map_layers.py, which only exists on #3 —sim/is not ondevyet. That PR is updated to drop its_require_working_normalsskip guard, since the normals are now correct with the filter disabled too. Merging this first is the cleaner order: if #3 lands first and its CI runs without a CUDA-capable torch, those two normals tests will fail until this is in.Related
Spotted in passing, not addressed here:
traversability(layer 3) is initialized to1.0and never written when the filter is disabled, so it reports fully-traversable everywhere rather than unknown.test_erosion_layer_is_boundedcannot catch that either — a constant1.0is within[0, 1].