feat(cdt): add simulation output and resumable checkpoints#121
Merged
acgetchell merged 2 commits intomainfrom May 6, 2026
Merged
feat(cdt): add simulation output and resumable checkpoints#121acgetchell merged 2 commits intomainfrom
acgetchell merged 2 commits intomainfrom
Conversation
- Add CSV measurement and JSON summary output paths for configured simulation runs. - Add serde-backed CDT triangulation and MCMC checkpoints with validated resume state, preserved RNG streams, and typed checkpoint errors. - Add an output-and-checkpoint example with semantic example validation through `just examples-validate`. - Document output files, resumable checkpoints, example validation, and Rust review guidance updates. Closes #60 Closes #116
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughAdds serde-backed serialization and checkpointing across CDT types, CSV/JSON output writers, CLI flags for output paths, RNG changes to Xoshiro, new error variants for I/O/checkpointing, example + CI wiring for output/checkpoint validation, and tests exercising serialization and resume semantics. ChangesSerialization & Output Infrastructure
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant Sim as Simulation
participant MCMC as MCMC_Engine
participant Serializer as Serializer
participant FS as FileIO
User->>CLI: invoke run_simulation (--output-csv/--output-json)
CLI->>Sim: build CDT (or run Metropolis)
Sim->>MCMC: run iterations / resume from checkpoint
MCMC->>Sim: return steps, measurements, move stats
Sim->>Serializer: prepare measurement CSV and JSON summary
Serializer->>FS: write CSV (if configured)
Serializer->>FS: write JSON (if configured)
FS-->>CLI: files persisted / errors reported
CLI-->>User: return SimulationResultsBackend (or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
- Coverage 92.52% 91.82% -0.71%
==========================================
Files 19 19
Lines 8661 9642 +981
==========================================
+ Hits 8014 8854 +840
- Misses 647 788 +141
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔴 Performance Regression DetectedPerformance Analysis ReportCDT Performance Analysis ReportGenerated: 2026-05-06T22:08:27+00:00 Summary
🔴 Performance Regressions
🟢 Performance Improvements
🆕 New Benchmarks
✅ Stable BenchmarksNo significant changes detected in 12 benchmarks. Performance analysis powered by Criterion.rs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cdt/foliation.rs (1)
339-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
Foliationduring deserialization.Deriving
Deserializehere bypassesFoliation::from_slice_sizes(), so inputs like a mismatchednum_slicesor an empty slice can now construct aFoliationvalue that the public constructor explicitly rejects. For checkpoint/resume state, this should fail at the serde boundary instead of leaking an invalid public state.🤖 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 `@src/cdt/foliation.rs` around lines 339 - 345, The derived Deserialize for struct Foliation allows invalid states to be created; replace the auto-derived Deserialize with a custom impl that validates inputs (e.g., ensure slice_sizes is non-empty and that the provided num_slices matches slice_sizes.len()) by either calling the public constructor Foliation::from_slice_sizes(slice_sizes) inside deserialization or performing the same checks and returning an error on mismatch so invalid checkpoint/resume data fails at the serde boundary.
🤖 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 `@src/cdt/ergodic_moves.rs`:
- Around line 16-17: The code imports rand::rngs::Xoshiro256PlusPlus which isn’t
re-exported by rand 0.10.1; either add the rand_xoshiro crate to Cargo.toml and
change the import to use rand_xoshiro::Xoshiro256PlusPlus (keeping
SeedableRng/RngExt usage), or replace all Xoshiro256PlusPlus usages with a
rand-provided RNG such as StdRng and update the imports (e.g., use
rand::rngs::StdRng) as used in the functions/blocks referencing
Xoshiro256PlusPlus (search for the Xoshiro256PlusPlus symbol and the
SeedableRng/RngExt uses around the ergodic_moves logic) so the code compiles
without missing-reexport errors.
In `@src/cdt/metropolis.rs`:
- Line 1042: The struct initializer currently reserves capacity for steps using
Vec::with_capacity(usize::try_from(self.config.steps).unwrap_or(0)), which can
pre-allocate a huge buffer and cause OOM; change the initialization of steps to
avoid reserving the full self.config.steps up front (use Vec::new() or a small
heuristic like min(self.config.steps as usize, 16)) in the
constructor/initializer where steps is set so the Step history grows
incrementally instead of allocating based on self.config.steps.
In `@src/cdt/results.rs`:
- Around line 138-139: The Summary struct currently hardcodes steps: &'a
[MonteCarloStep] and measurements: &'a [Measurement]; change both to optional
types (e.g., Option<&'a [MonteCarloStep]> and Option<&'a [Measurement]>) and add
serde attributes like #[serde(skip_serializing_if = "Option::is_none")] to those
fields so they are omitted from the JSON when None; update any constructors or
places that build the Summary (functions creating the Summary) to pass
Some(slice) when full telemetry is desired or None when emitting a lightweight
manifest, and adjust any consumers that iterate these fields to handle the
Option (unwrap_or(&[]) or pattern-match) accordingly.
- Around line 598-605: The aggregate.average_action currently calls
self.average_action() which includes the step-0 snapshot and pre-thermalization
measurements, biasing the result when thermalization_steps > 0; update the
call/site so that average_action uses only equilibrium measurements (i.e. skip
the first snapshot and all measurements before self.thermalization_steps),
either by adding a skip/offset parameter to average_action (e.g.
average_action(skip=self.thermalization_steps)) or by adding a new method that
computes the mean from measurements[self.thermalization_steps..], and replace
the call here so average_action is consistent with how average_volume_profile
and other volume aggregates already exclude warmup samples.
In `@src/config.rs`:
- Around line 27-28: The serde serialization for the enum CdtTopology currently
uses default variant names (PascalCase) which differs from clap::ValueEnum's
kebab-case, breaking round-tripping; add the serde attribute to unify them by
annotating the CdtTopology enum with #[serde(rename_all = "kebab-case")] so
serialized JSON uses the same kebab-case vocabulary as the CLI (affecting places
that serialize CdtTopology such as output_json and CdtConfigOverrides).
In `@src/geometry/backends/delaunay.rs`:
- Around line 111-130: into_global_topology() currently only verifies domain
length for SerializableGlobalTopology::Toroidal but must also reject NaN,
infinite, or non-positive periods; update the Toroidal branch in
into_global_topology<const D: usize, E: DeError> to, after converting domain
into [f64; D], iterate the array and validate each element is finite
(f.is_finite()) and > 0.0, returning Err(E::custom(...)) on the first offending
index/value with a clear message like "invalid toroidal period at index i:
value" so bad checkpoints fail deserialization instead of later panicking or
producing invalid topology.
- Around line 173-205: The Serialize and Deserialize impls for DelaunayBackend
must include explicit serde trait bounds for the triangulation Tds type because
SerializedDelaunayBackend::serialize()/deserialize() require them; update the
impl headers for DelaunayBackend<VertexData, CellData, D> (the impl Serialize
and impl<'de> Deserialize<'de>) to add where clauses requiring Tds<f64,
VertexData, CellData, D>: Serialize (for Serialize impl) and Tds<f64,
VertexData, CellData, D>: Deserialize<'de> (for Deserialize impl), and any
equivalent bounds on VertexData and CellData if needed so that calling
SerializedDelaunayBackend::serialize()/deserialize() compiles. Ensure the
changes reference the same generic parameters used in the current impls
(DelaunayBackend, VertexData, CellData, const D).
In `@src/lib.rs`:
- Around line 518-527: Pre-resolve both config.output_csv and config.output_json
using CdtConfig::resolve_path with base_dir, compare the resulting PathBufs for
equality, and if they are identical return an Err (or exit early) with a clear
error before calling results.write_measurements_csv or
results.write_summary_json; update the block around config.output_csv /
config.output_json to perform this preflight equality check and abort before any
file is written so one artifact cannot be silently overwritten.
In `@tests/cli.rs`:
- Around line 17-23: The temp_output_dir helper (and the similar test helpers in
the test modules in src/lib.rs and src/cdt/results.rs) uses
thread::current().name().unwrap_or("test") directly in a filename which can
contain ':' on Rust test thread names and break Windows paths; update these
helpers (e.g., temp_output_dir) to sanitize the thread name before joining into
the PathBuf by mapping invalid filename characters (at minimum ':' → '_', and
ideally other OS-invalid chars) to safe replacements and use the sanitized
string in the format! call so test-generated temp directories are cross-platform
safe.
---
Outside diff comments:
In `@src/cdt/foliation.rs`:
- Around line 339-345: The derived Deserialize for struct Foliation allows
invalid states to be created; replace the auto-derived Deserialize with a custom
impl that validates inputs (e.g., ensure slice_sizes is non-empty and that the
provided num_slices matches slice_sizes.len()) by either calling the public
constructor Foliation::from_slice_sizes(slice_sizes) inside deserialization or
performing the same checks and returning an error on mismatch so invalid
checkpoint/resume data fails at the serde boundary.
🪄 Autofix (Beta)
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: aba7fd83-8e84-4b0b-9f13-f39cd4b6768b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
AGENTS.mdCargo.tomlREADME.mddocs/code_organization.mddocs/dev/commands.mddocs/dev/rust.mddocs/dev/testing.mddocs/dev/tooling-alignment.mdexamples/output_and_checkpoint.rsjustfilescripts/run_all_examples.shsrc/cdt/action.rssrc/cdt/ergodic_moves.rssrc/cdt/foliation.rssrc/cdt/metropolis.rssrc/cdt/results.rssrc/cdt/triangulation.rssrc/config.rssrc/errors.rssrc/geometry/backends/delaunay.rssrc/lib.rstests/cli.rstests/integration_tests.rs
Comment on lines
+16
to
+17
| use rand::{RngExt, SeedableRng, rngs::Xoshiro256PlusPlus}; | ||
| use serde::{Deserialize, Serialize}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the Cargo.toml for rand version and any rand_xoshiro dependency
fd -t f Cargo.toml | head -1 | xargs cat | grep -A 5 "rand\|xoshiro" | head -20Repository: acgetchell/causal-triangulations
Length of output: 251
🏁 Script executed:
# Check the full ergodic_moves.rs file to see the actual imports and usage
head -50 src/cdt/ergodic_moves.rsRepository: acgetchell/causal-triangulations
Length of output: 1936
🏁 Script executed:
# Also check lines around 239-245, 284-288, 665-669 to see the usage context
sed -n '235,250p; 280,295p; 660,675p' src/cdt/ergodic_moves.rsRepository: acgetchell/causal-triangulations
Length of output: 1268
Add rand_xoshiro as a dependency or switch to a RNG type provided by rand.
The code imports rand::rngs::Xoshiro256PlusPlus, but rand 0.10.1 does not re-export Xoshiro types. Either add rand_xoshiro to Cargo.toml and use use rand_xoshiro::Xoshiro256PlusPlus;, or replace the RNG with one that rand provides directly (e.g., StdRng). This applies to all usages at lines 16, 239–245, 284–288, and 665–669.
🤖 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 `@src/cdt/ergodic_moves.rs` around lines 16 - 17, The code imports
rand::rngs::Xoshiro256PlusPlus which isn’t re-exported by rand 0.10.1; either
add the rand_xoshiro crate to Cargo.toml and change the import to use
rand_xoshiro::Xoshiro256PlusPlus (keeping SeedableRng/RngExt usage), or replace
all Xoshiro256PlusPlus usages with a rand-provided RNG such as StdRng and update
the imports (e.g., use rand::rngs::StdRng) as used in the functions/blocks
referencing Xoshiro256PlusPlus (search for the Xoshiro256PlusPlus symbol and the
SeedableRng/RngExt uses around the ergodic_moves logic) so the code compiles
without missing-reexport errors.
Comment on lines
+138
to
+139
| steps: &'a [MonteCarloStep], | ||
| measurements: &'a [Measurement], |
There was a problem hiding this comment.
Make the large telemetry arrays optional in the JSON summary.
Issue #60 called for the full measurement array to be optional, but this schema hardwires both steps and measurements into every summary. On long simulations that turns the “summary” file into a second full-data export and removes any way to emit a lightweight run manifest.
🤖 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 `@src/cdt/results.rs` around lines 138 - 139, The Summary struct currently
hardcodes steps: &'a [MonteCarloStep] and measurements: &'a [Measurement];
change both to optional types (e.g., Option<&'a [MonteCarloStep]> and Option<&'a
[Measurement]>) and add serde attributes like #[serde(skip_serializing_if =
"Option::is_none")] to those fields so they are omitted from the JSON when None;
update any constructors or places that build the Summary (functions creating the
Summary) to pass Some(slice) when full telemetry is desired or None when
emitting a lightweight manifest, and adjust any consumers that iterate these
fields to handle the Option (unwrap_or(&[]) or pattern-match) accordingly.
- Reject invalid checkpoint state at serde boundaries, including empty foliation bookkeeping and invalid toroidal periods. - Prevent CSV and JSON outputs from resolving to the same file before writing either artifact. - Align serialized topology names with CLI vocabulary and report equilibrium-only average action in JSON summaries. - Avoid preallocating simulation step history from the full configured step count. Closes #60 Closes #116
🔴 Performance Regression DetectedPerformance Analysis ReportCDT Performance Analysis ReportGenerated: 2026-05-06T23:39:19+00:00 Summary
🔴 Performance Regressions
🟢 Performance Improvements
🆕 New Benchmarks
✅ Stable BenchmarksNo significant changes detected in 1 benchmarks. Performance analysis powered by Criterion.rs |
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.
just examples-validate.Closes #60
Closes #116