refactor(api)!: split Pachner moves from vertex lifecycle edits#477
Merged
Conversation
- Add a unified PachnerMove request/result API with attempt_pachner dispatch and a focused prelude::pachner import surface. - Move vertex deletion into its own Delaunay module with typed DeleteVertexError and keep insertion/deletion terminology explicit through public docs and examples. - Keep primitive bistellar flip APIs available from delaunay::flips while hiding them from focused preludes intended for workflow users. - Add Pachner stress coverage, delete_vertex benchmarks, and Semgrep rules that require fallible results to be consumed and workflow fixtures to use vertex!. - Split long agent/development guidance into focused dev and architecture docs, including release citation and performance-tuning checklists. BREAKING CHANGE: DelaunayTriangulation::insert and remove_vertex are replaced by insert_vertex and delete_vertex, and focused Pachner workflows now import from prelude::pachner instead of prelude::flips. Primitive flip APIs remain available from delaunay::flips for tests, benchmarks, and low-level expert use. Closes #252
Contributor
|
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 (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughAdds a unified Pachner-move API, a transactional vertex-deletion path, and ChangesPachner API, deletion, and API renames
Possibly related issues
Possibly related PRs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 119 |
🟢 Coverage 97.66% diff coverage · +0.07% coverage variation
Metric Results Coverage variation ✅ +0.07% coverage variation (-1.00%) Diff coverage ✅ 97.66% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (db3fcb3) 72534 66383 91.52% Head commit (4893b18) 72771 (+237) 66650 (+267) 91.59% (+0.07%) 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 (#477) 684 668 97.66% 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/delaunay/insertion.rs (1)
55-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove stale deletion references from the insertion module comments.
These comments still describe deletion as part of this module/path, which conflicts with the insertion-only API split.
Suggested cleanup
-// Incremental insertion, deletion, and post-insertion repair/check helpers. +// Incremental insertion and post-insertion repair/check helpers. // These require an f64-backed kernel for spatial-index construction, -// Triangulation-layer insertion, and Triangulation-layer deletion. +// and Triangulation-layer insertion.- // - Future: Delaunay property restoration after deletion + // - Policy-controlled Delaunay property restoration after insertionAlso applies to: 184-184
🤖 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/delaunay/insertion.rs` around lines 55 - 57, The comments in the insertion path still mention deletion, which no longer matches the insertion-only split. Update the module documentation near the insertion helpers in insertion.rs so it only describes incremental insertion and post-insertion repair/check behavior, and remove any reference to Triangulation-layer deletion or deletion-related support from the affected comment blocks.src/lib.rs (1)
1387-1391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this compile-fail guard the new Pachner prelude contract.
This currently proves only that the old
prelude::flipspath is gone. Point it atprelude::pachnerso it catches accidental re-export ofBistellarFlips.As per coding guidelines, “Focused preludes must stay narrow and workflow-specific; use
delaunay::prelude::pachner::*for Pachner workflows, and import primitive bistellar flips directly only for testing, benchmarking, or documenting that primitive layer.”Suggested doctest adjustment
/// ```compile_fail -/// use delaunay::prelude::flips::BistellarFlips; +/// use delaunay::prelude::pachner::BistellarFlips; /// ```🤖 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/lib.rs` around lines 1387 - 1391, Update the compile_fail doctest in the focused prelude docs to validate the new Pachner prelude contract instead of the removed flips path. In the `src/lib.rs` prelude documentation block, change the import used in the guard to `delaunay::prelude::pachner::BistellarFlips` so the test catches any accidental re-export from `prelude::pachner`. Keep the check scoped to the existing doctest near the focused prelude explanation.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 `@docs/workflows.md`:
- Around line 456-457: Reword the delete_vertex documentation to match the
actual behavior: successful deletions do not reset the spatial index, they only
remove the deleted vertex entry and rely on live TDS validation for lookups.
Update the wording around the delete_vertex workflow in docs/workflows.md to
avoid implying a full spatial-index invalidation while still describing that
stale locate hints are no longer used.
In `@src/delaunay/deletion.rs`:
- Around line 229-266: The delete_vertex path in deletion::delete_vertex
currently returns success when
should_run_delaunay_repair_after_mutation(topology) is false, without enforcing
the Level-4 Delaunay invariant. Update this flow so it either performs the
repair via repair_delaunay_with_flips_k2_k3 and
normalize_and_promote_positive_orientation, or explicitly runs the Level-4
validation and maps any failure to DeleteVertexError::InvariantViolation with
rollback instead of returning Ok(simplices_removed).
- Around line 232-236: The vertex-deletion repair path in `deletion.rs`
currently calls `repair_delaunay_with_flips_k2_k3` with `None`, leaving the
flip-repair work unbounded and implicit. Update the `repair_result` block in
`delete_vertex` to derive or accept a deletion-specific maximum flip budget and
pass it explicitly as `Some(max_flips)` to `repair_delaunay_with_flips_k2_k3`,
so the repair work stays bounded and non-convergence can be surfaced
consistently.
---
Nitpick comments:
In `@src/delaunay/insertion.rs`:
- Around line 55-57: The comments in the insertion path still mention deletion,
which no longer matches the insertion-only split. Update the module
documentation near the insertion helpers in insertion.rs so it only describes
incremental insertion and post-insertion repair/check behavior, and remove any
reference to Triangulation-layer deletion or deletion-related support from the
affected comment blocks.
In `@src/lib.rs`:
- Around line 1387-1391: Update the compile_fail doctest in the focused prelude
docs to validate the new Pachner prelude contract instead of the removed flips
path. In the `src/lib.rs` prelude documentation block, change the import used in
the guard to `delaunay::prelude::pachner::BistellarFlips` so the test catches
any accidental re-export from `prelude::pachner`. Keep the check scoped to the
existing doctest near the focused prelude explanation.
🪄 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: 5f45e8b1-ab58-45dc-8213-882c1fe704d3
📒 Files selected for processing (61)
AGENTS.mdCargo.tomlREADME.mdREFERENCES.mdbenches/PERFORMANCE_RESULTS.mdbenches/README.mdbenches/ci_performance_suite.rsbenches/common/flip_workflows.rsbenches/delete_vertex.rsbenches/pachner_stress.rsdocs/README.mddocs/RELEASING.mddocs/api_design.mddocs/architecture/README.mddocs/architecture/module_map.mddocs/architecture/module_patterns.mddocs/architecture/prelude_reference.mddocs/architecture/project_structure.mddocs/code_organization.mddocs/dev/README.mddocs/dev/commands.mddocs/dev/docs.mddocs/dev/git.mddocs/dev/perf-tuning.mddocs/dev/rust.mddocs/dev/testing.mddocs/dev/tooling-alignment.mddocs/numerical_robustness_guide.mddocs/production_review_remediation_checklist.mddocs/workflows.mdexamples/README.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/topology_editing.rssemgrep.yamlsrc/core/algorithms/flips.rssrc/core/repair.rssrc/core/tds/mutation.rssrc/core/tds/storage.rssrc/core/traits/facet_cache.rssrc/delaunay/construction.rssrc/delaunay/deletion.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/pachner.rssrc/delaunay/query.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/lib.rstests/README.mdtests/benchmark_flip_fixtures.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_delaunay_triangulation.rstests/proptest_flips.rstests/semgrep/src/project_rules/rust_style.rstests/semgrep/src/project_rules/workflow_vertex_macro.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #477 +/- ##
==========================================
+ Coverage 91.49% 91.56% +0.06%
==========================================
Files 78 80 +2
Lines 72313 72550 +237
==========================================
+ Hits 66164 66431 +267
+ Misses 6149 6119 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Validate the Level 4 Delaunay invariant when automatic post-deletion repair is disabled, rolling back instead of returning success on violations. - Bound vertex-deletion flip repair with the shared local repair budget so non-convergence is surfaced consistently. - Document that successful deletion prunes only the removed spatial-index entry while live TDS validation protects later lookups. - Refine maintainer guidance for choosing focused Rust validators for test-, example-, and benchmark-only changes.
acgetchell
enabled auto-merge (squash)
June 24, 2026 11:24
This was referenced Jun 25, 2026
This was referenced Jul 5, 2026
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.
BREAKING CHANGE: DelaunayTriangulation::insert and remove_vertex are replaced by insert_vertex and delete_vertex, and focused Pachner workflows now import from prelude::pachner instead of prelude::flips. Primitive flip APIs remain available from delaunay::flips for tests, benchmarks, and low-level expert use.
Closes #252