This repository was archived by the owner on Aug 12, 2026. It is now read-only.
Repository navigation
feat(api)!: expose reported Metropolis transitions - #153
Merged
Merged
Conversation
- return typed reports that distinguish candidate success from acceptance - add a compiled ten-proposal quickstart with replayable provenance - pin and validate the self-contained archival Doxygen site BREAKING CHANGE: attempt_transition(manifold, move, trial) now returns MetropolisTransition instead of bool; use .accepted() for the previous result. Closes #114
acgetchell
enabled auto-merge
August 8, 2026 16:02
|
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 (1)
WalkthroughThe pull request adds a compiled C++ API quickstart, structured Metropolis transition results, reproducibility metadata, public API documentation, and generated-site validation with pinned Doxygen and Python tooling. ChangesPublic API and archival documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CTest
participant Quickstart
participant Metropolis
participant Persistence
CTest->>Quickstart: Run compiled example
Quickstart->>Metropolis: Execute ten transitions
Metropolis-->>Quickstart: Return transition reports and counters
Quickstart->>Persistence: Write and reload triangulation
Persistence-->>Quickstart: Return validated persisted state
Quickstart-->>CTest: Return validation status
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Metropolis.hpp (1)
490-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate unsupported
MoveTypevalues before counter indexing.Line 491 indexes
MoveTrackerbeforepropose_candidate()can returnUNKNOWN_MOVE. An invalid enum value can therefore fail ingsl::atinstead of returning the documented structured outcome.Reject unsupported values before any statistics mutation, or add an explicit unknown-move accounting bucket. Add a public-transition test for this contract.
As per path instructions, focus on interfaces and encapsulation in
include/**/*.hpp.🤖 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/Metropolis.hpp` around lines 490 - 501, Update the public transition flow around propose_candidate and MoveTracker counter accesses so unsupported MoveType values are validated before indexing or mutating statistics, returning the documented UNKNOWN_MOVE outcome instead of triggering gsl::at. Preserve normal accounting for supported moves, and add a public-transition test covering an invalid enum value.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 `@cmake/RunCppApiQuickstartTest.cmake`:
- Around line 9-10: Before execute_process in RunCppApiQuickstartTest, remove
both the ${TEST_OUTPUT} artifact and its sidecar file, then run the existing
test command so validation cannot use stale outputs from a prior invocation.
In `@docs/metropolis-hastings.md`:
- Line 133: Update the protocol fixture link in the Metropolis-Hastings
documentation to reference the immutable v1.0.0 tag or a specific commit instead
of the mutable main branch, preserving the existing fixture path and link
purpose.
In `@include/Ergodic_moves_3.hpp`:
- Around line 1404-1408: Update the documentation for propose_62_move() so the
generator description says it is advanced by site sampling and ordering
incident-edge flip paths, removing the incorrect reference to replacement-point
construction.
In `@include/Move_outcome.hpp`:
- Around line 117-122: Update Move_outcome.hpp lines 117-122 in successful() to
return true for MoveOutcome::SUCCEEDED, preserving the existing accepted and
rejected cases; add a compile-time assertion covering the selected SUCCEEDED
contract in tests/Move_outcome_test.cpp lines 106-115.
In `@REFERENCES.md`:
- Line 6: Pin the repository-owned links to the archival revision: update the
CITATION.cff link in REFERENCES.md (line 6), the tests/cgal_no_gmp_probe link in
docs/arithmetic-backend-evaluation.md (line 108), and the schema link in
docs/comparison-harness.md (line 13) to the exact release tag or commit SHA used
by the archival site, keeping the schema aligned with DEFAULT_RESULT_SCHEMA.
In `@scripts/tests/test_validate_generated_site.py`:
- Around line 51-121: Add tests in the existing validator test suite for the
uncovered enforcement branches: delete a required page and asset, remove
dark-mode styling, remove the generated Graphviz SVG, and add a local reference
escaping the site root. Each test should call validate_generated_site.validate
and assert GeneratedSiteError with the corresponding diagnostics: “missing
required page,” “missing required asset,” “missing automatic dark-mode styling,”
“no generated Graphviz SVG was found,” and “local reference escapes the site.”
In `@scripts/validate_generated_site.py`:
- Around line 376-388: Cache each generated file’s decoded text once and reuse
it across validation checks. Update the document parsing and the relevant
feature, Mermaid, and CSS validation paths around _parse_document,
_validate_required_features, _validate_css, and the Mermaid scan to consume
cached content instead of rereading files; preserve all existing validation
behavior.
- Line 20: Update the INTERNAL_SYMBOL regex in
scripts/validate_generated_site.py so the detail alternative matches only
namespace-qualified forms anchored to the :: scope separator, never a standalone
“detail” preceded by a word boundary; preserve matching for namespace
cdt::detail is not documented and the existing cdt::experimental:: form. Re-run
scripts/tests/test_validate_generated_site.py.
---
Outside diff comments:
In `@include/Metropolis.hpp`:
- Around line 490-501: Update the public transition flow around
propose_candidate and MoveTracker counter accesses so unsupported MoveType
values are validated before indexing or mutating statistics, returning the
documented UNKNOWN_MOVE outcome instead of triggering gsl::at. Preserve normal
accounting for supported moves, and add a public-transition test covering an
invalid enum value.
🪄 Autofix
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: de7fa04d-eaee-447d-abe2-3b82a8e825f9
📒 Files selected for processing (41)
CMakeLists.txtJustfileREADME.mdREFERENCES.mdcmake/RunCppApiQuickstartTest.cmakedocs/Doxyfiledocs/api-boundary.mddocs/arithmetic-backend-evaluation.mddocs/comparison-harness.mddocs/cpp-api-quickstart.mddocs/metropolis-hastings.mddocs/reproducibility.mdexamples/CMakeLists.txtexamples/cpp_api_quickstart.cppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Geometry.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_outcome.hppinclude/Move_run.hppinclude/Move_strategy.hppinclude/Move_tracker.hppinclude/Mpfr_value.hppinclude/Random.hppinclude/Runtime_config.hppinclude/S3Action.hppinclude/Settings.hppinclude/Utilities.hpppyproject.tomlscripts/doxygen.shscripts/tests/test_validate_generated_site.pyscripts/validate_generated_site.pytests/Metropolis_test.cpptests/Move_outcome_test.cpptests/Move_tracker_test.cpptests/Public_api_consumer.cpptests/Utilities_test.cpp
- Reject unknown move kinds before counter access and recognize completed transitions as successful. - Prevent stale quickstart artifacts and strengthen generated-site contract enforcement. - Pin scientific fixtures and metadata to the reviewed immutable revision.
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 `@tests/Metropolis_test.cpp`:
- Around line 738-765: Extend the unsupported-move test around
Metropolis_3::attempt_transition by capturing strategy.transition_trace() before
the call and asserting it remains equal afterward. Keep the existing state,
accounting, and transition_count checks unchanged.
🪄 Autofix
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: 4f9af388-6c05-410c-ac17-1d15e8349976
📒 Files selected for processing (12)
REFERENCES.mdcmake/RunCppApiQuickstartTest.cmakedocs/arithmetic-backend-evaluation.mddocs/comparison-harness.mddocs/metropolis-hastings.mdinclude/Ergodic_moves_3.hppinclude/Metropolis.hppinclude/Move_outcome.hppscripts/tests/test_validate_generated_site.pyscripts/validate_generated_site.pytests/Metropolis_test.cpptests/Move_outcome_test.cpp
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
BREAKING CHANGE: attempt_transition(manifold, move, trial) now returns MetropolisTransition instead of bool; use .accepted() for the previous result.
Closes #114
Summary by CodeRabbit
New Features
Documentation
Bug Fixes