Matcher_Cov2Cov: optional range-adaptive matching distance and ambiguity gate - #89
Merged
Merged
Conversation
…ity gate Introduce mp2p_icp::MatchingDistanceProfile, the acceptance criteria used by NearestPointWithCovCapable::nn_search_cov2cov(). It replaces the flat `float max_search_distance` argument, and is implicitly constructible from a float, so every existing caller keeps compiling and behaving as before. Two opt-in refinements over a single flat distance, both disabled by default: - Range-adaptive distance: `thresholdFar`, `thresholdKneeRange` and `thresholdTransitionWidth` turn the acceptance distance into a logistic function of the query point's range from the sensor. Map point density falls off with range, so one flat threshold is loose near the sensor and tight far away. - Ambiguity test: `firstToSecondDistanceMin` rejects a correspondence whose runner-up candidate is within that ratio of the winner's distance, i.e. the match is too close to call. `firstToSecondMinRange` restricts the test to beyond a given range, leaving the dense near field, where the runner-up is the same surface seen again, untouched. All five are declared with DECLARE_PARAMETER_OPT so they accept dynamic formulas like "3.0*ADAPTIVE_THRESHOLD_SIGMA", exactly as `threshold` does. A static YAML load would convert such a string by stopping at the first non-numeric character and silently yield a fixed value in meters. No shipped pipeline enables either refinement; defaults reproduce the previous behavior bit for bit, including the k=1 KD-tree query and no per-point range computation.
📝 WalkthroughWalkthrough
ChangesMatching distance profile integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Matcher_Cov2Cov
participant MatchingDistanceProfile
participant NearestPointWithCovCapable
Matcher_Cov2Cov->>MatchingDistanceProfile: build effective threshold profile
Matcher_Cov2Cov->>NearestPointWithCovCapable: call covariance matching with profile
NearestPointWithCovCapable->>NearestPointWithCovCapable: use flat threshold or reject adaptive profile
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
…ions The flat-threshold overload goes back to being the pure virtual, and the MatchingDistanceProfile one becomes a non-pure overload whose default implementation forwards to it. A map class written against an earlier release therefore keeps compiling, overriding and behaving exactly as before, which is what the downstream CI jobs need: they resolve mola_metric_maps to the last released binary package, which is older than this source tree. The forwarding default refuses, rather than silently ignores, a profile such an implementation cannot honor: a range-adaptive distance or an active ambiguity gate throws instead of quietly degrading to a flat threshold. Adds MP2P_ICP_HAS_MATCHING_DISTANCE_PROFILE so downstream packages can detect the new API. Since MatchingDistanceProfile.h is an entirely new header, a plain __has_include() settles it for consumers; the macro is what survives if the struct later grows members. Also documents the name-hiding consequence: a derived class declaring only one of the two overloads hides the other for calls on that derived type. Harmless for the matchers, which always dispatch through a base reference.
firstToSecondDistanceMin/firstToSecondMinRange were not yet justified by results (regressed translation error ~62% on a held-out sequence relative to the range-adaptive distance alone). Drop the fields, Matcher_Cov2Cov params, NearestPointWithCovCapable's guard against them, and the corresponding test coverage, keeping only the range-adaptive matching distance.
jlblancoc
marked this pull request as ready for review
August 12, 2026 08:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@mp2p_icp_core/mp2p_icp_map/include/mp2p_icp/MatchingDistanceProfile.h`:
- Around line 61-69: Correct the near-value semantics in MatchingDistanceProfile
and its operator() implementation: either redefine the near documentation as the
lower-range asymptote, or normalize the logistic calculation so operator()(0.0f)
returns near. Keep the far asymptote and flat-profile fast path behavior
unchanged, and update the related comments consistently.
In `@mp2p_icp_core/tests/test-mp2p_cov2cov.cpp`:
- Around line 600-627: Extend the dynamic-formula test around
Matcher_Cov2Cov::matchingDistanceProfile by defining thresholdKneeRange and
thresholdTransitionWidth from variables instead of static YAML values.
Initialize and realize those variables, then update them and assert the
profile’s knee range and transition width reflect the new values while
preserving the existing threshold and thresholdFar checks.
🪄 Autofix
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: 61b7f40a-a640-431d-9abe-c887db9ac2e0
📒 Files selected for processing (6)
agents.mdmp2p_icp_core/mp2p_icp/include/mp2p_icp/Matcher_Cov2Cov.hmp2p_icp_core/mp2p_icp/src/Matcher_Cov2Cov.cppmp2p_icp_core/mp2p_icp_map/include/mp2p_icp/MatchingDistanceProfile.hmp2p_icp_core/mp2p_icp_map/include/mp2p_icp/NearestPointWithCovCapable.hmp2p_icp_core/tests/test-mp2p_cov2cov.cpp
Comment on lines
+61
to
+69
| /** Distance at range=0. Also the flat value when far==near. */ | ||
| float near = 0.40f; | ||
| /** Distance as range -> infinity. Equal to `near` means "flat" (the | ||
| * common case, and the fast path below). */ | ||
| float far = 0.40f; | ||
| /** Range [m] at which the logistic transition is centered. */ | ||
| float kneeRange = 15.0f; | ||
| /** Logistic transition width [m]. Smaller = closer to a hard knee. */ | ||
| float width = 5.0f; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the near range semantics.
Line 61 states that near is the distance at range zero. For an adaptive profile, operator()(0.0f) returns near + (far - near) / (1 + exp(kneeRange / width)), not near.
Define near as the lower-range asymptote, or normalize the logistic curve if range zero must return near.
Also applies to: 77-85
🤖 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 `@mp2p_icp_core/mp2p_icp_map/include/mp2p_icp/MatchingDistanceProfile.h` around
lines 61 - 69, Correct the near-value semantics in MatchingDistanceProfile and
its operator() implementation: either redefine the near documentation as the
lower-range asymptote, or normalize the logistic calculation so operator()(0.0f)
returns near. Keep the far asymptote and flat-profile fast path behavior
unchanged, and update the related comments consistently.
Comment on lines
+600
to
+627
| // (b) All of them accept formulas and re-evaluate when the source changes. | ||
| { | ||
| mp2p_icp::Matcher_Cov2Cov m; | ||
| mrpt::containers::yaml p; | ||
| p["threshold"] = "2.0*SIGMA"; | ||
| p["thresholdFar"] = "5.0*SIGMA"; | ||
| p["thresholdKneeRange"] = 25.0; | ||
| p["thresholdTransitionWidth"] = 5.0; | ||
| m.initialize(p); | ||
|
|
||
| mp2p_icp::ParameterSource globalParams; | ||
| globalParams.attach(m); | ||
|
|
||
| globalParams.updateVariable("SIGMA", 0.1); | ||
| globalParams.realize(); | ||
|
|
||
| auto prof = m.matchingDistanceProfile(); | ||
| ASSERT_NEAR_(prof.near, 0.2, 1e-6); | ||
| ASSERT_NEAR_(prof.far, 0.5, 1e-6); | ||
| ASSERT_(!prof.isFlat()); | ||
|
|
||
| // A static load would have frozen these at their first value: | ||
| globalParams.updateVariable("SIGMA", 0.2); | ||
| globalParams.realize(); | ||
|
|
||
| prof = m.matchingDistanceProfile(); | ||
| ASSERT_NEAR_(prof.near, 0.4, 1e-6); | ||
| ASSERT_NEAR_(prof.far, 1.0, 1e-6); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test dynamic formulas for all adaptive parameters.
The test verifies dynamic formulas only for threshold and thresholdFar. It uses static values for thresholdKneeRange and thresholdTransitionWidth.
Set these two parameters from variables, update the variables, and assert that matchingDistanceProfile() returns the updated values. This will detect a regression from DECLARE_PARAMETER_OPT to a static YAML load.
🤖 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 `@mp2p_icp_core/tests/test-mp2p_cov2cov.cpp` around lines 600 - 627, Extend the
dynamic-formula test around Matcher_Cov2Cov::matchingDistanceProfile by defining
thresholdKneeRange and thresholdTransitionWidth from variables instead of static
YAML values. Initialize and realize those variables, then update them and assert
the profile’s knee range and transition width reflect the new values while
preserving the existing threshold and thresholdFar checks.
Member
Author
|
Mergin. The crash in tests is an ABI mismatch due to mixing nanoflann versions. Will get fixed after rebuilding against nanoflann_vendor, nothing to fix here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an opt-in, disabled-by-default range-adaptive acceptance criterion to
Matcher_Cov2Cov, and the small value type that carries it.Companion PR (required to build): MOLAorg/mola#195 —
mola_metric_mapsimplements the changed virtual.What changes
mp2p_icp::MatchingDistanceProfile(new header inmp2p_icp_map) replaces the flatfloat max_search_distanceargument ofNearestPointWithCovCapable::nn_search_cov2cov(). It is implicitly constructible from afloat, so every existing caller compiles and behaves unchanged.Matcher_Cov2CovparamthresholdFar0(off)threshold→thresholdFarthresholdKneeRange15.0mthresholdTransitionWidth5.0mRationale: map point density falls off with range, so a single flat threshold is simultaneously loose near the sensor and tight far away.
All three use
DECLARE_PARAMETER_OPT, notMCP_LOAD_OPT, so they accept dynamic formulas such as"3.0*ADAPTIVE_THRESHOLD_SIGMA"exactly likethresholddoes. This is deliberate and is covered by a regression test: a static YAML load converts that string by stopping at the first non-numeric character, silently yielding a fixed value in meters instead of a multiple of sigma.Removed: ambiguity gate
An earlier revision of this PR also added a first-to-second-nearest ambiguity test (
firstToSecondDistanceMin/firstToSecondMinRange). It was not yet justified by results — on a held-out sequence it regressed translation error by ~62% relative to the range-adaptive distance alone — so it has been dropped from this PR along with its test coverage and the corresponding guard inNearestPointWithCovCapable's defaultnn_search_cov2cov()overload.Backward compatibility
exp(), no per-point range computation, and a plaink=1KD-tree query.nn_search_cov2cov()is source-breaking only for out-of-tree implementers ofNearestPointWithCovCapable. Thefloat→ profile conversion is implicit, so call sites are unaffected; overriders must update their declaration.Tests
test-mp2p_cov2covgainstest_cov2cov_matcher_acceptance_params, covering (a) defaults stay flat and (b) the range-adaptive parameters re-evaluate when their formula's variables change. All existing suites pass unmodified:test-mp2p_cov2cov,test-mola_metric_maps_keyframemap,test-mola_metric_maps_incrementalpointcloud.Status / what still needs deciding
Draft, because the configuration question is still open even though the code is ready:
IncrementalPointCloudmap class. A full 11-sequence KITTI evaluation on both map classes is running now; a partial 4-sequence result was favorable onIncrementalPointCloudbut a smaller and partly opposite effect was measured onKeyframePointCloudMap, which is the shipped default. Nothing here should be enabled in a shipped pipeline until that finishes.Known gap
The range-adaptive distance is covered at the
Matcher_Cov2Covparameter level. There is no map-level unit test asserting the shape of the logistic distance profile end to end throughnn_search_cov2cov(). Happy to add one if you want it before merge.Summary by CodeRabbit
New Features
Bug Fixes
Tests