fix(mapping): exclude current scan from PGO loop target submap - #4201
Draft
RaymondLmx wants to merge 1 commit into
Draft
RaymondLmx wants to merge 1 commit into
RaymondLmx wants to merge 1 commit into
Conversation
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
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.
Contribution path
Linked issue: #4143 (awaiting maintainer triage). Submitted as a draft pending scope acceptance and completion of contributor review; not requesting merge yet.
Problem
The current keyframe is appended before loop search. Although candidate centers exclude it, the target submap window can include it. A scan with no overlap with any historical cloud can therefore match itself and produce an accepted loop.
Solution
Exclude the current keyframe index from the target submap. Keep the original window, member order, source submap, thresholds, candidate selection and optimizer unchanged. An empty target stays empty and is rejected by the existing ICP point-count gate.
Fixes #4143
How to Test
uv run pytest dimos/mapping/loop_closure/test_pgo.py -vValidated the full PGO test file on main commit 47b6e22 with this patch: 25 passed, including a positive closed trajectory, the no-overlap negative and six parametrized submap membership cases. The unpatched main fails the same negative (one score-zero loop). No memory-package substitution was used in this main-source run.
The isolated run used Python 3.12 on Linux aarch64, pytest 8.3.5, gtsam-extended 4.3a1.post1 and the existing scientific dependencies. Pytest flags:
-c /dev/null --noconftest -p no:cacheprovider, withPYTEST_DISABLE_PLUGIN_AUTOLOAD=1and the main package source first among source directories in PYTHONPATH. This tests the actual PGO implementation but does not reproduce the complete upstream lockfile/pytest plugin environment. Ruff 0.14.3 check/format and git diff --check pass. The unchanged official pre-commit configuration passes with pre-commit 4.3.0 for the two changed files, including always-run hooks; non-applicable hooks are skipped. The checked file hashes match the regression-tested files. Full-repository pytest/CI and pre-commit --all-files were not run.Completed offline checks on pinned source with the same implementation patch: stock adversarial no-overlap case accepts one score-zero loop; patched case accepts none. Five submap membership/boundary tests, three real ICP/positive-loop tests, two GTSAM/PGO tests, seven input-adapter tests and four export fixtures pass. No algorithm mocks or threshold tuning. The positive loop still improves synthetic position error. Two existing frozen recordings export and reload; the unaffected recording is byte-identical, while the affected recording changes keyframe positions by at most about 2 mm. No field data are included; these are diagnostic results, not localization safety qualification.
AI assistance
OpenAI Codex assisted with implementation, regression execution and draft preparation. The contributor's own review and understanding remain to be confirmed; this draft does not attest that review has happened.
Checklist
Signing is a contributor action; the first PR can present the signing prompt and signing is required before merge. This is not a dependency for local validation.