Adopt simplified cube stores: seed precedence, batch-regen, sufficient-statistic backscatter (#96)#97
Merged
Merged
Conversation
Seed precedence, batch-regen, and 3-band backscatter on top of the uma#248 SourceLayer/MbesCell migration
…, live-node→survey, welford n≥2 edge, ADR-0002 resolution)
Adopt the simplified stores from unh_marine_autonomy#248 across the CUBE
store-I/O layer (cube_bathymetry#96):
- SourceLayer: Draft/Processed/Chart collapse to Survey/Reference. The live
node and offline importer now write the `survey` layer; the read-only prior
is `reference`.
- BathyCell is 2-band {depth, uncertainty} (per-cell timestamp/source_index
dropped); geoGridToTile/mapSheetToTiles lose their timestamp/source args.
- MbesCell is the 3-band Welford sufficient statistic {mean, standard_error,
sample_sd}. geoGridToBackscatterCells encodes it (n=1 sentinel + n>=2
formulas, kBackscatterConfidenceScale=1.96); welfordFromCell is the exact
inverse used to seed backscatter accumulation from the survey layer.
- Registry: the per-cell SourceRegistry interning table is gone; finalize()
now writes an optional store-level StoreMetadata sidecar.
- Seed precedence: ImportAccumulator gains a seeded_ set + seedNewTile(),
run once per tile on first touch — survey warm-start (settled + backscatter
Welford restore) > reference gate (predicted-only, not measured, no
backscatter) > blank. Replaces the upfront whole-sheet prior load.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire the seed-precedence config into the offline importer (cube_bathymetry#96): - Replace --prior (upfront whole-store Chart load into the sheet, which defeated bounded-RAM eviction) with --reference-store, feeding the accumulator's lazy per-tile seedNewTile via ImportAccumulatorConfig::reference_store_dir. - Drop --bathy-layer (draft/processed collapsed to the single survey layer). - Replace the per-cell SourceRegistry/SourceRecord with a store-level StoreMetadata sidecar; --platform/--sensor map through and --campaign becomes the survey id. --source-id/--sensor-class are retired (no per-cell provenance). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Migrate test_store_import/test_persistence/test_tile_eviction_rss/ test_import_eviction to the Survey/Reference layers, 2-band BathyCell, and 3-band MbesCell (compare raw stored bands for the bounded==unbounded losslessness check). - Add backscatter Welford round-trip tests (n=1 sentinel, no-data cell, n>=2 real-dispersion) exercising geoGridToBackscatterCells + welfordFromCell. - Add ReferenceSeedDoesNotAddMeasuredData: a reference-layer seed gates but is never settled, so a sparse survey writes only its own cells — far fewer than the reference tile's finite cells and equal in count to a no-reference run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the authoritative off-boat rebuild path (cube_bathymetry#96). Where the
bounded-RAM ImportAccumulator re-derives a revisited tile's depth uncertainty
from a reseeded hypothesis, batch-regen is bit-exact: every tile is built in a
single unbounded pass over the complete set of soundings that touch it.
- BatchRegen (batch_regen.{h,cpp}) in the store_import library: streaming
scatter routes each sounding to a per-tile binary bucket on disk (its
one-cell-expanded window, matching addSoundings' spread) under a bounded LRU
of open streams; the gather replays each bucket in global order into a fresh
single-tile ImportAccumulator (seed precedence honoured) and writes only the
target tile via the new ImportAccumulator::persistResidentTile. Scratch is
cleaned up in finalize (and by the destructor on an exception).
- batch_regen_bag binary: import_bag's projection pipeline, scattering instead
of accumulating; a SheetFactory rebuilds identically-configured gather sheets.
- test_batch_regen: multi-tile + revisit builds are byte-exact vs a single-pass
unbounded ImportAccumulator (depth, uncertainty, 3-band backscatter); scratch
cleanup asserted.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- ADR-0001 addendum: two-rung seed precedence + the seed_settled measured/prior contract, batch-regen as the exact rebuild path, live-node -> survey layer. - ADR-0007 addendum: 3-band MbesCell write/reconstruct formulas (n=1 sentinel, n>=2 sample_sd/standard_error, kScale=1.96 shared constant), the exact welfordFromCell inverse + round-trip guarantee, and the accepted n>=2 identical-sample limitation (not code-guarded). - README: offline import vs batch-regen (bounded-RAM vs bit-exact), seed precedence (--reference-store), and backscatter fidelity notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Record the Implementation entry (all plan steps done, cube_bathymetry build+test GREEN in-container: 441 tests, 0 failures) and the plan deviations (StoreMetadata registry migration, dead-param removal, BatchRegen as a library class). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#96) Addresses local-review findings on ImportAccumulator: - welfordFromCell: guard se<=0 / non-finite before lround(ratio^2). A corrupt sample_sd!=0 / standard_error==0 cell drove ratio to inf/NaN and lround(inf) is UB with an out-of-range uint32 cast; fall back to the n=1 sentinel. (must-fix) - seedNewTile now returns bool: a rung-1 survey-seed READ error protects the intact-but-unreadable on-disk tile (caller drops it like a failed reload) instead of overwriting it from scratch -- symmetric with reloadEvictedTile. (suggestion) - add ImportAccumulatorConfig::skip_survey_seed so the batch-regen gather can force a from-scratch rebuild (used by the batch_regen change). (suggestion) - correct the overstated "atomic temp-then-rename" comments: tile_io::saveTile writes direct to the final path (flush/close checked, NOT crash-atomic); making that write atomic is a marine_tiled_raster_store follow-up. (watch)
Addresses local-review findings on the scatter/gather rebuild: - addBatch routes the WHOLE batch to every tile in the batch's one-cell-expanded bounds, mirroring GeoMapSheet::addSoundings exactly. A near-seam sounding deposits into a neighbour tile out to max_radius=CONF_99PC*sqrt(horizontal_error) (multiple cells for realistic TPU); the old per-sounding +/-1-cell scatter dropped those cross-sounding far-radius deposits, so the gather was not bit-exact near seams -- masked by the sub-cell horizontal_error the tests used. The gather's GeoGrid::insert re-applies the true radius test, so the wider bucket is a superset trimmed back to the exact single-pass deposit set. Adds SeamCrossingExactMatch, a multi-cell-radius seam regression test. (must-fix) - gather sets skip_survey_seed so a re-run into a populated -o store no longer silently blends onto tiles it rebuilds; warns when the output survey layer is non-empty (untouched tiles would be left stale). (suggestion) - flushOpenStreams + a flush/check on eviction: a disk-full close-time flush is now a hard error rather than a silently-truncated bucket / wrong tile. (suggestion)
Addresses local-review findings on the batch_regen_bag CLI: - parse -r / -l / --minimum-range / --maximum-range through guarded helpers that catch std::stod/std::stoi exceptions (and reject trailing junk), routing to usage() instead of std::terminate on a bad flag value. (suggestion) - reject a non-positive -r resolution up front (it drives Level::fromCellSize). (suggestion)
#96) Gather read-back was best-effort: a reopen failure on a bucket this run created was silently continue'd, and the read loop could not distinguish a badbit I/O error (or a truncated partial trailing record) from a clean EOF -- either yields a silently-missing/truncated tile, breaking the bit-exact rebuild contract. Make it symmetric with the hard-throwing scatter/flush path: throw on reopen failure, on in.bad(), and on a non-zero gcount at loop exit. Also document closeAllStreams' swallow-close-errors invariant (safe only because flushOpenStreams runs first on the success path; the destructor path must not throw). Addresses Local Review (Pre-Push) round 2 findings 1 (must-fix) and 2.
Raw std::stod/std::stoi/std::stoll on -r/--max-resident-tiles/-l/ --minimum-range/--maximum-range throw std::invalid_argument on a non-numeric value; uncaught in main that std::terminate's the process with an opaque message. Mirror the guarded parse_double/parse_int/parse_long helpers from the sibling batch_regen_main: catch and route to usage() with a clear diagnostic, and reject trailing junk. Addresses Local Review (Pre-Push) round 2 finding 3 (suggestion).
cppcheck: the bag-topic lookup loop var is only read; take it by const &. Addresses Local Review (Pre-Push) round 2 finding 4 (suggestion).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates cube_bathymetry to the simplified two-layer store model (survey/reference) and adds the offline behaviors needed for #96: lazy per-tile seed precedence, an exact scatter/gather batch regen path, and 3-band backscatter sufficient-statistic encoding + reconstruction.
Changes:
- Migrates store layer usage to
Survey/Reference, removes per-cell provenance bands, and switches provenance to store-levelStoreMetadata. - Implements seed precedence on first tile touch:
surveywarm-start (settled + backscatter restore) →referencepredicted-only gate → blank. - Adds
batch_regen_bag/BatchRegenscatter-gather rebuild with bit-exact equivalence tests, plus Welford sufficient-statistic backscatter round-trip tests and docs/ADRs.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents offline import/rebuild tools, seed precedence, and backscatter fidelity. |
| cube_bathymetry/CMakeLists.txt | Adds BatchRegen sources, batch_regen_bag binary, and new tests. |
| cube_bathymetry/include/cube_bathymetry/store_import.h | Updates store import API/docs, adds Welford reconstruct API + config changes. |
| cube_bathymetry/include/cube_bathymetry/batch_regen.h | Declares scatter/gather BatchRegen rebuild class. |
| cube_bathymetry/src/store_import.cpp | Implements 2-band bathy tile write, 3-band backscatter write, Welford reconstruction, seed precedence, and metadata finalize. |
| cube_bathymetry/src/import_bag_main.cpp | Updates CLI: --reference-store, StoreMetadata provenance, numeric parsing hardening, and survey-layer writes. |
| cube_bathymetry/src/cube_bathymetry_node.cpp | Migrates live node persistence/prime from Draft to Survey layer. |
| cube_bathymetry/src/batch_regen.cpp | Implements bucketed scatter/gather rebuild with FD-bounded LRU and I/O error hardening. |
| cube_bathymetry/src/batch_regen_main.cpp | Adds batch regen CLI tool mirroring import projection pipeline. |
| cube_bathymetry/test/test_tile_eviction_rss.cpp | Updates tests to new layer + tile conversion API. |
| cube_bathymetry/test/test_store_import.cpp | Updates backscatter tests for 3-band sufficient statistic + Welford reconstruction. |
| cube_bathymetry/test/test_persistence.cpp | Updates persistence tests to Survey layer and new tile conversion signature. |
| cube_bathymetry/test/test_import_eviction.cpp | Updates eviction/import tests and adds explicit reference-seed-not-measured-data test. |
| cube_bathymetry/test/test_batch_regen.cpp | Adds bit-exact batch regen equivalence tests + scratch cleanup checks. |
| cube_bathymetry/docs/decisions/0001-tile-eviction-and-incremental-publish.md | ADR-0001 addendum documenting seed precedence, batch-regen, and live-node → survey. |
| cube_bathymetry/docs/decisions/0007-mbes-backscatter-store-addendum-phase-b-transition.md | ADR-0007 addendum documenting 3-band write/reconstruct formulas and limitations. |
| .agent/work-plans/issue-96/plan.md | Agent plan artifact for issue #96. |
| .agent/work-plans/issue-96/progress.md | Agent progress artifact for issue #96. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+40
| `batch_regen` scatters each projected sounding to a per-tile bucket on disk, then | ||
| gathers each tile in a single unbounded pass (no eviction) — so no tile is ever | ||
| evicted mid-disambiguation. Its scratch scatter directory is cleaned up at the end. |
Comment on lines
+74
to
+75
| std::cout << "usage: batch_regen [options] -o <store_dir> " | ||
| "-d <detections_topic> <bag> [<bag> ...]\n"; |
Comment on lines
52
to
56
| /// `geoGridToTile` / `mapSheetToTiles` persist live data directly into the | ||
| /// `marine_bathymetry_store` `draft/` layer tiles via `tile_io` (atomic | ||
| /// temp-then-rename) with NO lossy Cartesian→geographic resample. The single | ||
| /// `marine_bathymetry_store` `draft/` layer tiles via `tile_io` (a direct, | ||
| /// flush/close-checked write — not crash-atomic) with NO lossy Cartesian→geographic | ||
| /// resample. The single | ||
| /// fused `draft` grid (no per-day epochs, unh_marine_autonomy#221) accumulates |
Comment on lines
781
to
+784
| // Persist every grid touched since the last save as a marine_bathymetry_store | ||
| // draft tile (atomic temp-then-rename via tile_io::saveTile), then clear the | ||
| // dirty set. A no-op when persistence is disabled or nothing changed. | ||
| // draft tile (via tile_io::saveTile -- a direct, flush/close-checked write, not | ||
| // crash-atomic), then clear the dirty set. A no-op when persistence is disabled or | ||
| // nothing changed. |
Comment on lines
+610
to
+613
| std::cerr << "import_bag: could not survey-seed tile on first touch: " | ||
| << e.what() << " (dropping this batch's soundings on the tile to " | ||
| "protect the on-disk surface; a later batch retries the seed, but these " | ||
| "dropped soundings are not re-processed)" << std::endl; |
Comment on lines
+636
to
+638
| std::cerr << "import_bag: could not reference-seed tile on first touch: " | ||
| << e.what() << " (no prior gate for this tile)" << std::endl; | ||
| } |
Comment on lines
450
to
454
| if (store_dir.empty() || detections_topic.empty() || bagfile_names.empty()) { | ||
| std::cerr << "error: -o <store_dir>, -d <detections_topic>, and " | ||
| "at least one bag are all required\n"; | ||
| usage(); | ||
| } |
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.
Closes #96
Adopts the simplified store format (unh_marine_autonomy#248, already merged) on the CUBE side, plus three new features. #248 landed first, so this merges on its own — no co-land constraint.
Summary
SourceLayerDraft/Processed/Chart →Survey/Reference;MbesCell→ 3-band{mean, standard_error, sample_sd}; per-cellSourceRegistry→ store-levelStoreMetadata. The live CUBE node now writes thesurveylayer (draft+processed collapsed;referenceis the read-only prior).survey(settled warm-start + backscatter Welford restore) →reference(predicted-only,seed_settled=false, not counted as measured data) → blank.import_baggains--reference-store(replacing--prior); the upfront whole-sheet load is gone.batch_regen_bagbinary (+ testableBatchRegenclass): scatter soundings to per-tile disk buckets → gather each tile in one unbounded pass → write once. Bit-exact vs a single-pass unbounded run, including across GGGS seams (a sounding's TPU-radius deposits into neighboring tiles are preserved).n=(sample_sd/standard_error)²,M2=sample_sd²(n−1), n=1 sentinel;kScale=1.96).ADRs
seed_settledcontract; batch-regen as the exact rebuild path; live-node →survey.Verification
Host build + test GREEN — 442 tests, 0 failures. Key regression tests:
BatchRegen.SeamCrossingExactMatch(multi-tile radius, bit-exact vs single-pass), Welford round-trips (incl. n=1 sentinel),ReferenceSeedDoesNotAddMeasuredData. Lint (uncrustify + cpplint) clean.Review
Driven via
/run-issue: 2 review-code rounds. Round 1 (2 must-fix: near-seam bit-exactness +welfordFromCelldivision UB) → fixed + re-verified under adversarial tracing. Round 2 (1 mechanical must-fix: gather read-back I/O-error symmetry) → fixed. Round 2 Ship: recommended.Follow-ups (out of scope)
build_massabesic_store.sh(unh_echoboats_project11) flag migration: drop--bathy-layer/--prior/--source-id/--sensor-class, adopt--reference-store+--platform/--sensor/--campaign.BagReaders/projection helper frombatch_regen_main+import_bag_main(~800-line dedup).marine_tiled_raster_store::saveTilecrash-atomic (temp-then-rename).Authored-By:
Claude Code AgentModel:
Claude Opus 4.8 (1M context)