fix(moves)!: make Metropolis transitions reversible and reproducible#112
Conversation
- complete failure-atomic 3D Pachner moves and Hastings proposal ratios - centralize seeded PCG streams across initialization and transitions - standardize local and GitHub Actions validation through just ci and Semgrep - synchronize v1.0.0-rc1 metadata, citations, and uv experiment entry points BREAKING CHANGE: move helpers now require caller-owned RNG state, and generated OFF filenames include seed and pass metadata. Closes #92 Closes #105
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe release candidate introduces caller-owned reproducible RNG streams, seed-aware CLI execution and output metadata, revised Metropolis-Hastings transitions, release and citation metadata, vcpkg and CI tooling, Semgrep policies, documentation workflows, and expanded deterministic tests. ChangesRelease and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/Ergodic_moves_3_test.cpp (1)
577-595: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
CAPTURE(random.seed())for this scenario's RNG.Every other seeded block in this file captures
random.seed()for failure diagnosis, but this one doesn't. Given the PR's reproducibility goal (reporting effective seeds for randomized test failures), this block should follow suit.🩹 Proposed fix
cdt::Random random{92}; + CAPTURE(random.seed()); WHEN("A (2,6) move is proposed")🤖 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 `@tests/Ergodic_moves_3_test.cpp` around lines 577 - 595, Add CAPTURE(random.seed()) to the seeded test scenario containing the do_26_move call, before executing the move, matching the failure-diagnosis pattern used by the other randomized blocks in this test.include/Foliated_triangulation.hpp (2)
899-914: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
@tparam/@paramdocs for the new caller-owned generator.
make_foliated_ballandmake_triangulationgained a requiredGenerator&template parameter andgeneratorparameter, but the Doxygen blocks above them weren't updated to document either. Since these are public template APIs whose signature just became a breaking change (callers must now own and pass RNG state), documenting ownership/lifetime expectations here matters.As per path instructions for
include/**/*.hpp: "Focus on design patterns, interfaces, and encapsulation."Also applies to: 985-998
🤖 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 `@include/Foliated_triangulation.hpp` around lines 899 - 914, Update the Doxygen comments for make_foliated_ball and make_triangulation to document the Generator template parameter with `@tparam` and the caller-owned generator argument with `@param`, including that callers provide and maintain its RNG state for the call.Source: Path instructions
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new caller-owned generator parameter across all its introduction sites. This PR makes RNG ownership an explicit, breaking part of the public interface (
Generator&/cdt::Random&/cdt::Random&&), but none of the sites that introduce it add@tparam/@paramdocs, unlike sibling functions/constructors in the same files that do document their parameters.
include/Foliated_triangulation.hpp#L899-914,985-998: add@tparam Generatorand@param generatortomake_foliated_ballandmake_triangulation.include/Foliated_triangulation.hpp#L1248-1269: add@param generator/@param t_simplices/etc. to the two newFoliatedTriangulation<3>constructors, matching the detail level of theDelaunay-based constructor above them.include/Manifold.hpp#L111-130: add@param generator(and the other parameters) to the two newManifold<3>constructors, matching theCausal_verticesconstructor immediately below.As per path instructions for
include/**/*.hpp: "Focus on design patterns, interfaces, and encapsulation."🤖 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 `@include/Foliated_triangulation.hpp` at line 1, Document every newly introduced generator and constructor parameter in the listed public APIs: add `@tparam` Generator and `@param` generator to make_foliated_ball and make_triangulation, and add complete `@param` entries for both new FoliatedTriangulation<3> and Manifold<3> constructors. Match the detail and naming style of the adjacent documented constructors, including all other parameters.Source: Path instructions
🤖 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 `@scripts/optimize_initialize.py`:
- Around line 65-120: Remove the unreachable
cm.exceptions.NoMoreSuggestionsAvailable exception handler and ensure each
Experiment created in the parameters loop always calls experiment.end(),
including when qx or later processing raises an error. Scope cleanup to the
current experiment using a finally-based structure while preserving iteration
over remaining parameter pairs according to the existing error behavior.
- Around line 66-69: Replace the unsupported Experiment.log_multiple_params call
in the parameter loop with the current SDK method Experiment.log_parameters,
preserving the existing hyper_params and experiment flow.
In `@scripts/tests/test_optimize_initialize.py`:
- Around line 29-34: Extend the tests around test_initializer_output_is_parsed
to cover both RuntimeError branches in _parse_initializer_output: assert an
output without the final simplex count raises the expected error, and assert an
output without any volume-profile entries raises the expected error. Keep the
existing happy-path assertion unchanged.
In `@tests/Random_benchmark.cpp`:
- Around line 67-69: Validate owned_time.count() before computing speedup in the
benchmark measurement flow, rejecting or skipping runs whose duration is zero
after nanosecond truncation. Keep the existing speedup calculation unchanged for
positive owned durations, and anchor the change around owned_ns and speedup.
In `@tests/S3Action_test.cpp`:
- Around line 166-173: Update the test description around the THEN block to
explicitly state that the values remain distinct at MPFR precision but collapse
to the same value when converted to double. Split the assertions into separate
THEN/AND_THEN blocks if appropriate, with names covering both the nonzero MPFR
delta and equal double conversions.
---
Outside diff comments:
In `@include/Foliated_triangulation.hpp`:
- Around line 899-914: Update the Doxygen comments for make_foliated_ball and
make_triangulation to document the Generator template parameter with `@tparam` and
the caller-owned generator argument with `@param`, including that callers provide
and maintain its RNG state for the call.
- Line 1: Document every newly introduced generator and constructor parameter in
the listed public APIs: add `@tparam` Generator and `@param` generator to
make_foliated_ball and make_triangulation, and add complete `@param` entries for
both new FoliatedTriangulation<3> and Manifold<3> constructors. Match the detail
and naming style of the adjacent documented constructors, including all other
parameters.
In `@tests/Ergodic_moves_3_test.cpp`:
- Around line 577-595: Add CAPTURE(random.seed()) to the seeded test scenario
containing the do_26_move call, before executing the move, matching the
failure-diagnosis pattern used by the other randomized blocks in this test.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0a21636-e52e-4d29-84d4-9c3a08191eba
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.github/CONTRIBUTING.md.github/workflows/ci.yml.semgrepignoreCITATION.cffCMakeLists.txtCMakePresets.jsonJustfileREADME.mdREFERENCES.mdcmake/RunCdtNoOutputTest.cmakecmake/Version.hpp.indocs/Doxyfiledocs/metropolis-hastings.mddocs/reproducibility.mdinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_tracker.hppinclude/Mpfr_value.hppinclude/Random.hppinclude/S3Action.hppinclude/Utilities.hpppyproject.tomlscripts/__init__.pyscripts/clang-tidy.shscripts/mnist_experiment.pyscripts/optimize_initialize.pyscripts/release_check.pyscripts/semgrep_fixture_config.pyscripts/tests/__init__.pyscripts/tests/test_optimize_initialize.pyscripts/tests/test_release_check.pysemgrep.yamlsrc/CMakeLists.txtsrc/cdt-viewer.cppsrc/cdt.cppsrc/initialize.cppsrc/optimize-initialize.pysrc/test.pytests/Apply_move_test.cpptests/CMakeLists.txttests/Ergodic_moves_3_test.cpptests/Foliated_triangulation_test.cpptests/Function_ref_test.cpptests/Geometry_test.cpptests/Manifold_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Random_benchmark.cpptests/Random_header_consumer.cpptests/Random_test.cpptests/S3Action_test.cpptests/Settings_test.cpptests/Tetrahedron_test.cpptests/Utilities_test.cpptests/Vertex_test.cpptests/main.cpptests/semgrep/doctest_hygiene.cpptests/semgrep/move_hot_path_logging.cpptests/semgrep/random_ownership.cppvcpkg.json
💤 Files with no reviewable changes (3)
- tests/main.cpp
- src/test.py
- src/optimize-initialize.py
- run pinned Doxygen and Graphviz checks through just ci on every platform - publish the same validated documentation output to gh-pages - install Windows tooling without Chocolatey - keep sanitizer workloads deterministic and within runner timeouts - ensure Comet experiments clean up and reject malformed initializer output - complete RNG documentation and guard benchmark timing edge cases
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/Utilities.hpp (1)
378-389: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the RNG documentation with the generic API.
The template accepts any
std::uniform_random_bit_generator, but the preceding text still says it uses PCG. Limit that claim tocdt::Randomcallers or describe the API as generator-agnostic.Proposed fix
- /// Uses Melissa E. O'Neill's Permuted Congruential Generator for high-quality - /// RNG which passes the TestU01 statistical tests. + /// Samples from the caller-owned uniform random bit generator. + /// `cdt::Random` uses Melissa E. O'Neill's Permuted Congruential Generator.🤖 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 `@include/Utilities.hpp` around lines 378 - 389, Update the documentation for the generic RNG API near the NumberType, Distribution, and Generator template declarations so it no longer claims that all callers use PCG; describe the function as generator-agnostic, or explicitly scope the PCG description to cdt::Random callers while preserving the existing generator parameter documentation.README.md (1)
142-145: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the cross-platform build entry point accurately.
Line 142 says
build.shruns on every supported platform, but Windows dispatches toscripts/build.bat. Refer tojust buildor the platform-specific scripts instead.🤖 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 `@README.md` around lines 142 - 145, Update the README build-entry description to avoid claiming build.sh runs on every supported platform; refer to the cross-platform just build command and acknowledge the platform-specific scripts, including scripts/build.bat for Windows, while preserving the stated test coverage.
🤖 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 `@scripts/sanitizer.sh`:
- Around line 39-40: Update the cdt invocation in scripts/sanitizer.sh to pass
the same deterministic --seed 92 option already used by initialize, while
preserving cdt’s existing arguments.
---
Outside diff comments:
In `@include/Utilities.hpp`:
- Around line 378-389: Update the documentation for the generic RNG API near the
NumberType, Distribution, and Generator template declarations so it no longer
claims that all callers use PCG; describe the function as generator-agnostic, or
explicitly scope the PCG description to cdt::Random callers while preserving the
existing generator parameter documentation.
In `@README.md`:
- Around line 142-145: Update the README build-entry description to avoid
claiming build.sh runs on every supported platform; refer to the cross-platform
just build command and acknowledge the platform-specific scripts, including
scripts/build.bat for Windows, while preserving the stated test coverage.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9738c6a-dccb-4d84-8af7-c6f91d211a94
📒 Files selected for processing (19)
.github/actions/setup-just/action.yml.github/workflows/ci.yml.github/workflows/doxygen.ymlJustfileREADME.mddocs/Doxyfileinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Manifold.hppinclude/Utilities.hppscripts/doxygen.shscripts/optimize_initialize.pyscripts/sanitizer.shscripts/tests/test_optimize_initialize.pytests/CMakeLists.txttests/Ergodic_moves_3_test.cpptests/Random_benchmark.cpptests/S3Action_test.cpp
- prepare dependencies before CodeQL and trace only CDT++ production targets - make sanitizer and initializer optimization runs replayable with recorded seeds - reject malformed initializer profiles instead of silently ignoring them - clarify cross-platform build entry points and generator-agnostic RNG documentation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Utilities.hpp (1)
418-423: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccept the timeslice bound by value.
generate_random_timeslicetakest_max_timesliceas a forwarding reference, so a mutable lvalue likeint max = 8;deducesIntegerTypetoint&and makesstatic_cast<IntegerType>(1)ill-formed. Taking the bound by value, or decaying the type before the cast, keeps this public helper usable for ordinary named bounds.🤖 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 `@include/Utilities.hpp` around lines 418 - 423, Update generate_random_timeslice to accept t_max_timeslice by value rather than as a forwarding reference, while preserving the existing integer-bound behavior and forwarding it appropriately to generate_random_int. Ensure ordinary mutable lvalue bounds such as named integers compile successfully.Source: Path instructions
🤖 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.
Outside diff comments:
In `@include/Utilities.hpp`:
- Around line 418-423: Update generate_random_timeslice to accept
t_max_timeslice by value rather than as a forwarding reference, while preserving
the existing integer-bound behavior and forwarding it appropriately to
generate_random_int. Ensure ordinary mutable lvalue bounds such as named
integers compile successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfa47bb8-4ec4-412a-affe-43edc0d1b890
📒 Files selected for processing (9)
.github/workflows/codeql.ymlJustfileREADME.mdinclude/Utilities.hppscripts/codeql-build.shscripts/optimize_initialize.pyscripts/pkgx-build.shscripts/sanitizer.shscripts/tests/test_optimize_initialize.py
- pass the Unix pkgx toolchain as one whitespace-separated package list - propagate verified Windows tools through PowerShell Core environment files - accept mutable lvalue bounds in generate_random_timeslice
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/Utilities.hpp (1)
368-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
die_rolldocumentation with its generic interface.
die_rollaccepts anystd::uniform_random_bit_generator, not specifically a PCG engine. Update the brief to describe a caller-supplied generator, or constrain the template if PCG-only behavior is intentional.Suggested change
- /// `@brief` Roll a die with PCG + /// `@brief` Roll a die using a caller-supplied generatorAs per path instructions, this public header should keep its interface documentation aligned with the actual abstraction.
🤖 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 `@include/Utilities.hpp` around lines 368 - 376, Update the Doxygen brief for die_roll to describe rolling a die using a caller-supplied std::uniform_random_bit_generator rather than referring specifically to PCG; keep the generic template interface unchanged.Source: Path instructions
.github/workflows/ci.yml (1)
125-134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the Windows archive downloads with explicit timeouts.
.github/workflows/ci.yml:125-134—Invoke-WebRequestuses an indefinite timeout by default, so a stalled Doxygen or Graphviz download can hang the Windows CI job. Add-TimeoutSecto both calls.🤖 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 @.github/workflows/ci.yml around lines 125 - 134, Add an explicit -TimeoutSec value to both Invoke-WebRequest calls downloading the Doxygen and Graphviz archives in the Windows CI setup, using the same bounded timeout for each and leaving their existing URLs and output paths unchanged.
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 125-134: Add an explicit -TimeoutSec value to both
Invoke-WebRequest calls downloading the Doxygen and Graphviz archives in the
Windows CI setup, using the same bounded timeout for each and leaving their
existing URLs and output paths unchanged.
In `@include/Utilities.hpp`:
- Around line 368-376: Update the Doxygen brief for die_roll to describe rolling
a die using a caller-supplied std::uniform_random_bit_generator rather than
referring specifically to PCG; keep the generic template interface unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: db06e032-8c60-48d2-95d0-c9aac13ecb52
📒 Files selected for processing (3)
.github/workflows/ci.ymlinclude/Utilities.hpptests/Utilities_test.cpp
- Keep documentation validation separate from the portable just ci contract. - Launch pinned Doxygen and Graphviz ephemerally in the publishing workflow. - Document die_roll using its generic caller-supplied RNG contract.
|
@coderabbitai review |
✅ Action performedReview finished.
|
- verify the pinned vcpkg executable with native certutil - align the action checkout with the repository vcpkg cache - limit MSVC build parallelism to the proven two-job configuration
Capture the configured remote before comparing it so the Windows bootstrap does not reject the checkout prepared by run-vcpkg. Continue requiring the official microsoft/vcpkg repository while accepting canonical HTTPS URLs with or without the .git suffix.
Replace the platform-specific bootstrap validators with one Python implementation shared by Windows, macOS, and Linux. Preserve origin, baseline, executable-integrity, and version checks while allowing action-owned untracked vcpkg metadata.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- provision sanitizer CMake and Ninja from shared Justfile pins - isolate sanitizer builds from pkgx transitive build variables - canonicalize Windows cache paths and avoid false secret classification
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/_sanitizer.yml:
- Around line 87-90: Update the sanitizer workflow’s tool-version configuration
so the llvm.org entry uses an exact LLVM package version or resolved digest
instead of the major-version selector from steps.tool-versions.outputs.llvm.
Keep the existing CMake and Ninja entries unchanged.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1906d63f-8519-4eb1-bf9e-bd4e1fa6d338
📒 Files selected for processing (1)
.github/workflows/_sanitizer.yml
2 participants
BREAKING CHANGE: move helpers now require caller-owned RNG state, and generated OFF filenames include seed and pass metadata.
Closes #92
Closes #105
Summary by CodeRabbit
--seedsupport tocdtandinitialize, including “Effective random seed” output and seed-aware deterministic replay/output.CITATION.cff, and introducedREFERENCES.md.--no-outputvalidation to ensure correct seed/help behavior.