Loop closure / map Reconstruction first pass#2242
Conversation
Replaces the monolithic pgo_then_voxels with four primitives over mem2 Streams and Transforms: pgo_keyframes(lidar) -> Stream[Keyframe] keyframes_to_corrections(kfs) -> Stream[Transform] (world_corrected <- world_raw) make_interpolator(corrections) -> (ts) -> Transform (SLERP + linear, endpoint-clipped) apply_corrections(stream, corr) -> Stream[T] (shuffles obs.pose) Drift correction is now a first-class, reusable Stream[Transform] that any pose-stamped consumer can apply — same math as before (rigid T_corr = T_global @ T_local^-1 per keyframe, SLERP + linear interpolation between bracketing keyframes), just composable. dimos map --pgo migrates to the new primitives; pgo_then_voxels is deleted. The internal _SimplePGO / PGOConfig / _KeyPose machinery stays in pgo.py and is imported by pgo2.
Renames: pgo.py -> pgo_internals.py (gtsam/ICP machinery) pgo2.py -> pgo.py (public Stream-shaped API) `dimos.mapping.relocalization.pgo` is now the canonical import path (`pgo_keyframes`, `keyframes_to_corrections`, `make_interpolator`, `apply_corrections`, `correction_at`, `Keyframe`). The internal _SimplePGO / PGOConfig / _KeyPose / _icp / _voxel_downsample helpers live in pgo_internals.py and are imported lazily inside pgo.py to keep gtsam off the public-API import path.
Adds test_pgo.py covering pose normalization on Observation, the Transform↔Pose3 conversion helpers, the interpolator edge cases, apply_corrections behavior, and a real-recording smoke test (skipped when data/go2_short.db is absent). Annotates the two map.py helpers so they pass mypy. Removes leftover section divider comments to satisfy the no-section-markers project rule.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The colors-copy block in `PointCloud2.transform` calls `self.pointcloud` to check `has_colors()`, which forces a tensor->legacy conversion on every invocation. With `pgo --full-pgo` rebuilding from hundreds of lidar frames, that hidden allocation dominates the per-frame cost. The colors path was lidar-irrelevant (lidar clouds have no colors anyway) and the feature it was meant to support never landed; remove it so transform() stays a clean numpy round-trip.
…s into feat/ivan/pgo_rewrite
The outer `pose = obs.pose` (Pose | None) was being shadowed by `pose = estimate_marker_pose(...)` (tuple[rvec, tvec] | None), which mypy flags as an incompatible reassignment. Renamed the inner one to `marker_pose`.
NearFilter now holds a typed `Vector3` instead of an `Any` named `pose`. `Stream.near()` does the one-time normalization at construction (Pose/ PoseStamped → .position, then Vector3(...) which accepts tuple/ndarray/ Vector3). `matches()` uses Vector3 subtraction + length_squared(); the sqlite filter compiler reads f.position.x/.y/.z directly. The _xyz() helper is gone.
Greptile SummaryThis PR replaces the numpy-based
Confidence Score: 4/5Most of the bugs from previous rounds are now fixed; the remaining unfixed crash (LookupError on empty PGO output in map.py and eval.py) is the main blocker for recordings with all-placeholder poses. The Observation refactor, GTSAM-based PGO, and marker pipeline are all sound and well-tested. All previously flagged pose-corruption and type-crash bugs are addressed. The one active issue is that dimos/utils/cli/map.py (line 285) and dimos/mapping/loop_closure/eval.py (line 95) — both call Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as dimos map CLI
participant Lidar as Stream[PointCloud2]
participant PGO as PGO Transformer
participant PGOState as _PGOState (ISAM2)
participant PoseGraph as PoseGraph
participant Accum as _accumulate
CLI->>Lidar: store.streams.lidar + spatial dedup
CLI->>Lidar: lidar.tap(prog).transform(PGO())
loop per lidar frame
Lidar->>PGO: Observation[PointCloud2]
PGO->>PGO: filter placeholder poses (is_zero)
PGO->>PGOState: process(pose3, ts, cloud)
PGOState->>PGOState: _add_keyframe (BetweenFactorPose3)
PGOState->>PGOState: _search_for_loops (KDTree + ICP)
alt loop accepted
PGOState->>PGOState: _smooth_and_update (ISAM2)
PGO-->>CLI: yield Observation[PoseGraph] snapshot
end
end
PGO-->>CLI: final yield Observation[PoseGraph]
CLI->>CLI: "graph = .last().data"
CLI->>Accum: "_accumulate(seen.values(), graph=graph)"
loop per deduped frame
Accum->>PoseGraph: correction_at(obs.ts)
PoseGraph->>PoseGraph: slerp/lerp interpolation
PoseGraph-->>Accum: drift-correction Transform
Accum->>Accum: cloud.transform(correction) → VoxelGrid
end
Accum-->>CLI: PointCloud2 (PGO map)
Reviews (24): Last reviewed commit: "Merge branch 'main' into feat/ivan/pgo_r..." | Re-trigger Greptile |
…s into feat/ivan/pgo_rewrite
np.percentile sorts the full z array on every frame; min/max is O(n) and avoids the per-frame cost for visualization color scaling.
map.py imported dimos.mapping/memory2 at module top, which transitively pulls torch, transformers, open3d, sklearn. Since dimos.py eagerly imports map to register the subcommand, dimos --help ballooned to ~7s and tripped test_cli_startup. Move those imports into the function bodies; module top keeps only the typer signature so --help still renders all options.
Integrate origin/main (PR #2242 loop_closure rewrite, #2278 aruco Detection3D, #2316 docs/coding-agents rename, mem2 time windowing, etc.). Took origin's marker-free PGO/PoseGraph rewrite for loop_closure (eval, pgo, test_pgo, markers_rrd), marker_transformer, and the map CLI; kept branch map_rrd. Stripped remaining # ---- section markers from map_rrd.
dimos map hk_village5 --pgo --markers