Skip to content

fix(mapping): exclude current scan from PGO loop target submap - #4201

Draft
RaymondLmx wants to merge 1 commit into
dimensionalOS:mainfrom
RaymondLmx:fix/pgo-current-frame-pr
Draft

RaymondLmx wants to merge 1 commit into
dimensionalOS:mainfrom
RaymondLmx:fix/pgo-current-frame-pr

Conversation

@RaymondLmx

Copy link
Copy Markdown

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 -v

Validated 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, with PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 and 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

  • I have read and approved the CLA.

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.

@github-actions github-actions Bot added the first-time-contributor PR opened by an author who had not previously committed to this repository label Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first-time-contributor PR opened by an author who had not previously committed to this repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Offline PGO: target submap can include current scan and accept unsupported loop closures

1 participant