fix(moves): enforce causal ergodic move contracts - #116
Conversation
- reject malformed cavities, invalid time assignments, and stale metadata - preserve source manifolds and rebuild handle-bearing state after mutation - validate complete CDT invariants and literature-derived move deltas - document admissibility, inverse relationships, and failure atomicity - update the repository UV pin to 0.11.30 Closes #106
|
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 (2)
WalkthroughThe PR tightens validation for all five supported 2+1D CDT moves, adds post-mutation checks, documents move contracts and deltas, and introduces independent canonicalization, failure-atomicity, inverse-round-trip, and seeded replay tests. Changes2+1D CDT ergodic move audit
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MoveCommand
participant MoveStrategy
participant ErgodicMoves
MoveCommand->>ErgodicMoves: apply_random_move
MoveCommand->>ErgodicMoves: check_move(current, result, move_type)
MoveStrategy->>ErgodicMoves: check_move(current, candidate, move)
ErgodicMoves-->>MoveCommand: accept or reject result
ErgodicMoves-->>MoveStrategy: accept or reject candidate
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}
- compare preserved move configuration values by representation to satisfy strict floating-point warnings without weakening drift detection - allow coverage-instrumented unit tests enough time to complete
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 (4)
tests/Ergodic_moves_3_audit_test.cpp (2)
301-310: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep the invariant audit independent of production predicates.
check_independent_invariantsstill relies onmanifold.is_foliated(),check_vertices(),check_simplices(), andis_correct(). If a move implementation and one of these predicates share the same causal or metadata defect, the audit can pass without detecting it. Make raw TDS, vertex-time, cell-type, and neighbor checks authoritative; treat production predicates only as supplemental diagnostics.As per path instructions, tests must provide comprehensive coverage.
🤖 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_audit_test.cpp` around lines 301 - 310, Update check_independent_invariants to make independent raw TDS, vertex-time, cell-type, and neighbor validations authoritative, using counts and triangulation data rather than manifold.is_foliated(), check_vertices(), check_simplices(), and is_correct(). Retain those production predicates only as supplemental diagnostics, and ensure the audit comprehensively covers the stated invariants.Source: Path instructions
629-836: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftExercise post-mutation failure atomicity.
The rejection cases here fail during site selection or preflight. They do not exercise the
(2,6)/(6,2)or bistellar-flip paths that mutate a private copy and then reject after a TDS/count validation fails. Add deterministic post-operation failure fixtures and compare the original canonical state afterward.As per path instructions, tests must provide comprehensive coverage.
🤖 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_audit_test.cpp` around lines 629 - 836, The existing rejection tests cover only preflight failures; add deterministic tests for post-mutation validation failures in the (2,6)/(6,2) and bistellar-flip paths. Extend the audit scenario using the relevant move helpers, fixtures, and validation hooks to force TDS or count validation to fail after mutation, then assert the operation rejects and the original canonical state remains unchanged. Ensure both mutation paths and their failure-atomic behavior are covered comprehensively.Source: Path instructions
include/Ergodic_moves_3.hpp (2)
749-754: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the edge before reading its vertices.
is_well_formed_edgeonly checks the handle shape;find_bistellar_flip_locationstill dereferencest_edge_candidate.firstbefore anytriangulation.tds().is_edge(...)membership check. Move that check ahead of the vertex lookups, or reusefinite_incident_cellshere.🤖 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/Ergodic_moves_3.hpp` around lines 749 - 754, Update find_bistellar_flip_location to validate t_edge_candidate against triangulation.tds().is_edge(...) immediately after the is_well_formed_edge check and before either vertex(...)->info() lookup. Reuse finite_incident_cells if that is the established membership-validation path, while preserving the existing early-return behavior for invalid edges.
81-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject infinite incident cells here.
finite_incident_cells()should returnnulloptas soon as any incident cell is infinite; filtering them out lets boundary edges satisfy the size checks inis_32_movable()andfind_bistellar_flip_location().🤖 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/Ergodic_moves_3.hpp` around lines 81 - 106, Update finite_incident_cells() to return std::nullopt immediately when any incident cell is infinite, rather than filtering infinite cells from incident_cells. Preserve the existing edge validation and successful collection of all finite incident cells for fully finite edges.
🤖 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/Ergodic_moves_3_audit_test.cpp`:
- Around line 850-861: Extend the test case around make_44_fixture and
check_move with a second doctest subcase that changes only foliation_spacing
while preserving initial_radius, using -0.0 in the constructed Manifold and
asserting its representation is retained. Verify after.is_correct() and that
check_move rejects the FOUR_FOUR transition, while keeping the existing
initial_radius coverage unchanged.
---
Outside diff comments:
In `@include/Ergodic_moves_3.hpp`:
- Around line 749-754: Update find_bistellar_flip_location to validate
t_edge_candidate against triangulation.tds().is_edge(...) immediately after the
is_well_formed_edge check and before either vertex(...)->info() lookup. Reuse
finite_incident_cells if that is the established membership-validation path,
while preserving the existing early-return behavior for invalid edges.
- Around line 81-106: Update finite_incident_cells() to return std::nullopt
immediately when any incident cell is infinite, rather than filtering infinite
cells from incident_cells. Preserve the existing edge validation and successful
collection of all finite incident cells for fully finite edges.
In `@tests/Ergodic_moves_3_audit_test.cpp`:
- Around line 301-310: Update check_independent_invariants to make independent
raw TDS, vertex-time, cell-type, and neighbor validations authoritative, using
counts and triangulation data rather than manifold.is_foliated(),
check_vertices(), check_simplices(), and is_correct(). Retain those production
predicates only as supplemental diagnostics, and ensure the audit
comprehensively covers the stated invariants.
- Around line 629-836: The existing rejection tests cover only preflight
failures; add deterministic tests for post-mutation validation failures in the
(2,6)/(6,2) and bistellar-flip paths. Extend the audit scenario using the
relevant move helpers, fixtures, and validation hooks to force TDS or count
validation to fail after mutation, then assert the operation rejects and the
original canonical state remains unchanged. Ensure both mutation paths and their
failure-atomic behavior are covered comprehensively.
🪄 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: d9a83ac6-11c0-448f-9421-b6ba7618d79a
📒 Files selected for processing (3)
.github/workflows/codecov-upload.ymlinclude/Ergodic_moves_3.hpptests/Ergodic_moves_3_audit_test.cpp
Reject boundary and foreign edges before accessing triangulation metadata, and treat any infinite incident cell as an invalid move site. Preserve source manifolds when validation rejects post-mutation states for (2,6), (6,2), and (4,4) transitions.
All valid findings were addressed in cec15b5. The remaining signed-zero foliation-spacing suggestion was verified to require an invalid acausal manifold state, documented in the thread, and resolved.
1 participant
Closes #106
Summary by CodeRabbit
uvversion and increased CI test timeout for coverage runs.