feat(api)!: add Pachner feasibility checks#489
Conversation
- Add immutable flip and Pachner feasibility reports for dry-run workflows. - Route Pachner feasibility through the shared bistellar preflight used by mutating flips. - Export Pachner feasibility from the focused Pachner prelude. BREAKING CHANGE: External BistellarFlips implementors must add the new can_flip_* feasibility methods. Closes #419
WalkthroughAdds immutable flip-feasibility validation to bistellar flips, introduces topology-owner provenance, and replaces direct Pachner mutation with a staged proposal-and-attempt workflow. Downstream docs, examples, benchmarks, and tests are updated to use and verify the new APIs. ChangesPachner feasibility and provenance workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
api
breaking change
enhancement
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 99 |
🟢 Coverage 90.91% diff coverage · +0.01% coverage variation
Metric Results Coverage variation ✅ +0.01% coverage variation (-1.00%) Diff coverage ✅ 90.91% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (abd466f) 76956 70043 91.02% Head commit (1cb06d8) 77334 (+378) 70395 (+352) 91.03% (+0.01%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#489) 429 390 90.91% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/pachner_roundtrip.rs (1)
603-649: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse
assert_topology_and_delaunay_validto close a Delaunay-invariant coverage gap.This generic D=2..5 smoke test only calls
validate()andis_valid_embedding()after the mutation, but skipsis_valid_delaunay(), unlikeassert_topology_and_delaunay_valid(lines 66-78) used by the 2D toroidal test. Since this is the only test exercising K1Insert across D=2..5, a Delaunay-property regression in higher dimensions specifically would go undetected here.♻️ Proposed fix
- trial - .as_triangulation() - .validate() - .unwrap_or_else(|err| panic!("{D}D k=1 mutation should preserve topology: {err}")); - trial - .as_triangulation() - .is_valid_embedding() - .unwrap_or_else(|err| panic!("{D}D k=1 mutation should preserve embedding: {err}")); + assert_topology_and_delaunay_valid(&trial, &format!("{D}D k=1 mutation"));🤖 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/pachner_roundtrip.rs` around lines 603 - 649, The generic K1Insert smoke test in assert_public_k1_insert_feasibility_smoke only checks topology and embedding after mutation, leaving the Delaunay invariant unverified. Update this test to reuse assert_topology_and_delaunay_valid for the post-mutation assertions, or add the same is_valid_delaunay check alongside validate() and is_valid_embedding(). Keep the existing feasibility/mutation flow and ensure the postcondition coverage matches the 2D toroidal test.Source: Coding guidelines
🤖 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/pachner_roundtrip.rs`:
- Around line 317-355: Add the missing parity assertion for the toroidal
K1Insert roundtrip test so it checks `result.removed_face_vertices` against
`feasibility.removed_face_vertices` just like
`assert_public_k1_insert_feasibility_smoke` does. Update
`pachner_feasibility_agrees_with_toroidal_2d_k1_insert` to compare this field
alongside `kind`, `direction`, `removed_simplices`, and `inserted_face_vertices`
to fully verify `can_attempt_pachner` and `attempt_pachner` stay in sync.
---
Nitpick comments:
In `@tests/pachner_roundtrip.rs`:
- Around line 603-649: The generic K1Insert smoke test in
assert_public_k1_insert_feasibility_smoke only checks topology and embedding
after mutation, leaving the Delaunay invariant unverified. Update this test to
reuse assert_topology_and_delaunay_valid for the post-mutation assertions, or
add the same is_valid_delaunay check alongside validate() and
is_valid_embedding(). Keep the existing feasibility/mutation flow and ensure the
postcondition coverage matches the 2D toroidal 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 3752e0bc-9e0f-41a0-a161-9af129dcc164
📒 Files selected for processing (9)
src/core/algorithms/flips.rssrc/core/simplex.rssrc/delaunay/construction.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/pachner.rssrc/lib.rstests/pachner_roundtrip.rstests/prelude_exports.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
==========================================
+ Coverage 90.99% 91.00% +0.01%
==========================================
Files 86 87 +1
Lines 76734 77112 +378
==========================================
+ Hits 69823 70175 +352
- Misses 6911 6937 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Add provenanced PachnerProposal values that carry topology owner, generation, and feasibility evidence. - Route public Pachner dry-run and mutation workflows through propose_pachner, can_attempt_on, and attempt_on. - Reject wrong-owner and stale-generation proposals with typed FlipError variants before interpreting topology-local keys. - Update public examples, docs, benchmarks, preludes, and roundtrip tests for the proposal workflow. BREAKING CHANGE: PachnerMoves no longer exposes raw can_attempt_pachner or attempt_pachner entry points. Callers must parse a PachnerMove with propose_pachner(...) and then use PachnerProposal::can_attempt_on(...) or PachnerProposal::attempt_on(...).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
benches/pachner_stress.rs (1)
200-220: 🚀 Performance & Scalability | 🔵 TrivialBenchmark now measures the full propose+attempt cost, not raw flip mutation.
Each sample now pays for
propose_pachner(feasibility computation) plusattempt_on(full revalidation before mutating), whereas the priorattempt_pachnerpath paid for the mutation's validation once. This is required correctness-wise (each cloned triangulation gets a freshTopologyOwnerId, so proposals can't be pre-minted once and replayed across the batch), but it does shift what these numbers represent. Worth a brief note (here or in the benchmark's README/changelog) so future perf-regression comparisons against pre-PR baselines account for the added propose-stage cost rather than reading it as a flip-primitive regression.🤖 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 `@benches/pachner_stress.rs` around lines 200 - 220, Update the benchmark documentation or an adjacent comment in bench_pachner_move to note that the measurement now includes both propose_pachner feasibility work and attempt_on revalidation before mutation, not just the raw flip mutation. Use the bench_pachner_move and attempt_pachner_move symbols to make clear this is a benchmark semantics change, so future comparisons against older results account for the added propose-stage cost.
🤖 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.
Nitpick comments:
In `@benches/pachner_stress.rs`:
- Around line 200-220: Update the benchmark documentation or an adjacent comment
in bench_pachner_move to note that the measurement now includes both
propose_pachner feasibility work and attempt_on revalidation before mutation,
not just the raw flip mutation. Use the bench_pachner_move and
attempt_pachner_move symbols to make clear this is a benchmark semantics change,
so future comparisons against older results account for the added propose-stage
cost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8c781190-e2d5-42c6-a09b-7859e552024a
📒 Files selected for processing (17)
benches/pachner_stress.rsdocs/api_design.mddocs/dev/rust.mddocs/limitations.mddocs/roadmap.mddocs/workflows.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/topology_editing.rssrc/core/algorithms/flips.rssrc/core/tds/storage.rssrc/core/triangulation.rssrc/delaunay/pachner.rssrc/delaunay/triangulation.rssrc/lib.rstests/pachner_roundtrip.rstests/prelude_exports.rs
✅ Files skipped from review due to trivial changes (3)
- docs/limitations.md
- docs/dev/rust.md
- docs/roadmap.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/algorithms/flips.rs
1 participant
BREAKING CHANGE: External BistellarFlips implementors must add the new can_flip_* feasibility methods.
Closes #419