perf(core): localize remove_vertex repair#401
Merged
Conversation
Owner
acgetchell
commented
May 19, 2026
acgetchell
commented
- Avoid global simplex scans and incident-simplex rebuilds on successful vertex removal by using the affected vertex star and scoped repair.
- Promote and validate orientation over the touched removal scope, with the full global orientation path retained as a fallback.
- Add a Criterion remove_vertex benchmark covering successful removals and invalid-remnant rollback across 2D through 5D.
- Avoid global simplex scans and incident-simplex rebuilds on successful vertex removal by using the affected vertex star and scoped repair. - Promote and validate orientation over the touched removal scope, with the full global orientation path retained as a fallback. - Add a Criterion remove_vertex benchmark covering successful removals and invalid-remnant rollback across 2D through 5D.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRefactors vertex removal into a transactional fan-retriangulation flow with bounded orientation normalization and incidence-pointer repair; adds insertion repair-budgeting; introduces a Criterion benchmark for remove_vertex (2D–5D) and updates Cargo/docs, CI/nextest installation, justfile recipes, and example-run script. ChangesVertex Removal Refactoring and Benchmarking
Sequence Diagram(s)sequenceDiagram
participant Client
participant RemoveVertex as remove_vertex
participant Finder as find_simplices_containing_vertex
participant FanHelper as remove_vertex_with_fan_retriangulation
participant Cavity as fan_fill_cavity / fan_boundary_facets_excluding_apex
participant Validator as validate_vertex_removal_postconditions
Client->>RemoveVertex: request remove(vertex_key)
RemoveVertex->>Finder: find incident simplices
RemoveVertex->>RemoveVertex: compute boundary & affected_vertices
RemoveVertex->>FanHelper: snapshot, choose apex, delegate retriangulation
FanHelper->>Cavity: fan_fill_cavity (exclude apex)
FanHelper->>FanHelper: canonicalize orientations
FanHelper->>FanHelper: wire cavity neighbors, remove old simplices
FanHelper->>Validator: run localized normalization & postcondition checks
FanHelper->>Client: return Ok or Err (with rollback)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 | 53 |
🟢 Coverage 90.61% diff coverage · +0.03% coverage variation
Metric Results Coverage variation ✅ +0.03% coverage variation (-1.00%) Diff coverage ✅ 90.61% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (dadb724) 61899 55992 90.46% Head commit (50ed347) 62304 (+405) 56379 (+387) 90.49% (+0.03%) 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 (#401) 479 434 90.61% 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
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
benches/remove_vertex.rs (1)
59-175: ⚖️ Poor tradeoffConsider expanding adversarial geometric coverage in success fixtures.
The success benchmark fixtures currently use well-conditioned interior points placed within a canonical simplex. The rollback case covers a degenerate scenario (minimal simplex), but the success path would benefit from additional adversarial geometric inputs such as near-boundary points, cospherical sets, and large coordinates to stress-test removal performance under challenging conditions.
As per coding guidelines: "Include adversarial inputs (near-boundary points, cospherical sets, degenerate simplices, large coordinates) alongside well-conditioned inputs in both tests and benchmarks in Rust"
🤖 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/remove_vertex.rs` around lines 59 - 175, The success fixtures only generate well-conditioned interior points inside a canonical simplex (generate_vertices, normalized_positive_direction, simplex_points), so expand adversarial geometric coverage by adding alternative point generators (near-boundary, cospherical, near-degenerate/simplex, large-coordinate) and trying them when building the source in build_success_source; implement small helper generators (e.g., generate_near_boundary_points, generate_cospherical_points, generate_large_coordinate_points, generate_near_degenerate_simplex) that accept the same seed and count and return Point<f64, D> arrays, then update generate_vertices to select between the original interior generator and these adversarial generators based on the seed or an attempt index and include their points into vertices before calling DelaunayTriangulation::new; ensure successful_removal_vertex is still used to validate and that seed search attempts exercise all generator types so benchmarks include near-boundary, cospherical, degenerate, and large-coordinate cases.src/core/repair.rs (1)
456-473: 💤 Low valueConsider naming the iteration bound constant.
The hardcoded
3in the orientation normalization loop lacks documentation. Per coding guidelines, repair paths should use explicit budgets. A named constant would clarify the convergence rationale.💡 Suggested improvement
+/// Maximum local orientation-normalization attempts before falling back to global promotion. +const MAX_LOCAL_ORIENTATION_ATTEMPTS: usize = 3; + fn normalize_vertex_removal_orientation( &mut self, validation_scope: &SimplexKeyBuffer, ) -> Result<(), InvariantError> { - for _ in 0..3 { + for _ in 0..MAX_LOCAL_ORIENTATION_ATTEMPTS {🤖 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/core/repair.rs` around lines 456 - 473, Replace the magic number 3 in the retry loop with a named constant to document the orientation-normalization budget: define a constant (e.g., ORIENTATION_NORMALIZATION_ATTEMPTS or ORIENTATION_NORMALIZATION_BUDGET) and use it in the for loop that calls canonicalize_positive_orientation_for_simplices, tds.normalize_coherent_orientation, and validate_geometric_simplex_orientation_for_simplices; keep the existing behavior but centralize the value so future readers and maintainers understand the convergence attempt limit.
🤖 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/remove_vertex.rs`:
- Around line 59-175: The success fixtures only generate well-conditioned
interior points inside a canonical simplex (generate_vertices,
normalized_positive_direction, simplex_points), so expand adversarial geometric
coverage by adding alternative point generators (near-boundary, cospherical,
near-degenerate/simplex, large-coordinate) and trying them when building the
source in build_success_source; implement small helper generators (e.g.,
generate_near_boundary_points, generate_cospherical_points,
generate_large_coordinate_points, generate_near_degenerate_simplex) that accept
the same seed and count and return Point<f64, D> arrays, then update
generate_vertices to select between the original interior generator and these
adversarial generators based on the seed or an attempt index and include their
points into vertices before calling DelaunayTriangulation::new; ensure
successful_removal_vertex is still used to validate and that seed search
attempts exercise all generator types so benchmarks include near-boundary,
cospherical, degenerate, and large-coordinate cases.
In `@src/core/repair.rs`:
- Around line 456-473: Replace the magic number 3 in the retry loop with a named
constant to document the orientation-normalization budget: define a constant
(e.g., ORIENTATION_NORMALIZATION_ATTEMPTS or ORIENTATION_NORMALIZATION_BUDGET)
and use it in the for loop that calls
canonicalize_positive_orientation_for_simplices,
tds.normalize_coherent_orientation, and
validate_geometric_simplex_orientation_for_simplices; keep the existing behavior
but centralize the value so future readers and maintainers understand the
convergence attempt limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6630e8ef-9d3f-4fdd-882f-a49c6722045d
📒 Files selected for processing (4)
Cargo.tomlbenches/README.mdbenches/remove_vertex.rssrc/core/repair.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
==========================================
+ Coverage 90.44% 90.48% +0.03%
==========================================
Files 71 71
Lines 61695 62100 +405
==========================================
+ Hits 55802 56189 +387
- Misses 5893 5911 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Add adversarial remove_vertex benchmark fixtures for near-boundary, cospherical, near-degenerate, and large-coordinate point sets. - Include the selected fixture kind in Criterion case names so benchmark output shows which geometry each run exercises. - Name the vertex-removal orientation normalization budget.
Add focused regression coverage for vertex-removal helper behavior: - affected-vertex collection and missing-simplex error preservation - local validation-scope deduplication and live-simplex filtering - incident-simplex repair, isolated-vertex reporting, and postcondition failure - fan-boundary filtering for facets that already contain the apex
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/core/repair.rs`:
- Around line 415-447: repair_vertex_removal_facet_issues currently calls
repair_local_facet_issues_with_frontier without a budget; add a
max_simplices_removed parameter to repair_vertex_removal_facet_issues and thread
it into repair_local_facet_issues_with_frontier (and any intermediate helpers)
so the local-facet repair honors the same explicit budget as the public repair
entry (e.g., repair_facet_oversharing). Change
repair_local_facet_issues_with_frontier to accept the limit and return a typed
non-convergence error when the limit is exceeded, map that error into the
existing Result<..., InvariantError> path (same pattern used by the public entry
point), only apply removed counts when within the budget, and ensure callers
handle and propagate the typed failure instead of allowing unbounded removals.
- Around line 441-447: The current branch calls repair_neighbor_pointers(&mut
self.tds) after a successful removal which triggers a full-TDS scan; instead,
reuse the scoped neighbor-repair helper used elsewhere (the logic in
repair_neighbors_after_local_simplex_removal) and invoke it with the
post_repair_frontier so neighbor fixes remain local and use the existing
local-repair/fallback flow. Replace the repair_neighbor_pointers(&mut self.tds)
call in the removed > 0 path with the scoped helper invocation that accepts
self.tds (or a mutable reference) and post_repair_frontier, preserving error
mapping via insertion_error_to_invariant_error as before.
🪄 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: 3ef66234-834f-409d-b8a9-ae393dc971fc
📒 Files selected for processing (1)
src/core/repair.rs
- Preserve local repair removal budgets before mutating the TDS. - Reuse scoped neighbor repair after vertex-removal facet cleanup. - Install and verify cargo-nextest for faster CI test recipes. - Build examples once before running their compiled binaries.
Contributor
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 (1)
src/core/insertion.rs (1)
1695-1729:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBudget the interior repair loop from
new_simplices, not the removed conflict set.This loop only detects and removes simplices from
new_simplices, but the cap is based onconflict_simplices.len(). In higher dimensions the cavity boundary can produce more new simplices than conflicted ones, so a valid local repair can now fail early withMaxSimplicesRemovedExceededeven though it never leaves the inserted star.♻️ Suggested fix
- let max_repair_simplices_removed = conflict_simplices.len(); + let max_repair_simplices_removed = new_simplices.len();🤖 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/core/insertion.rs` around lines 1695 - 1729, The repair loop budgets removals using max_repair_simplices_removed = conflict_simplices.len(), but the loop only scans and removes from new_simplices, so in high-dimensions the number of new_simplices can exceed the conflict set and trigger MaxSimplicesRemovedExceeded prematurely; change the budget to derive from new_simplices.len() (or the remaining size of new_simplices) instead of conflict_simplices.len() so repair_budget_remaining is calculated against the pool actually being modified (update the binding that sets max_repair_simplices_removed and the subsequent use in repair_budget_remaining passed to repair_local_facet_issues_with_frontier, referencing new_simplices, total_removed, and repair_local_facet_issues_with_frontier).
🤖 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 `@src/core/repair.rs`:
- Around line 416-460: The current repair_vertex_removal_facet_issues does
one-pass repair; change it to an iterative, budgeted loop that repeatedly calls
detect_local_facet_issues() and then
repair_local_facet_issues_with_frontier(&issues,
max_simplices_removed_remaining) until either detect_local_facet_issues()
returns None (no issues), validate_facet_sharing() (or equivalent check)
succeeds, or the max_simplices_removed budget is exhausted. In each iteration
update *simplices_removed, decrement the remaining budget, extend
post_repair_frontier with repair_outcome.frontier_simplices, and call
repair_neighbors_after_local_simplex_removal(new_simplices,
post_repair_frontier) when removed>0; if the budget is exhausted return a typed
non-convergence InvariantError (similar to insertion path error handling)
instead of silently returning Ok.
---
Outside diff comments:
In `@src/core/insertion.rs`:
- Around line 1695-1729: The repair loop budgets removals using
max_repair_simplices_removed = conflict_simplices.len(), but the loop only scans
and removes from new_simplices, so in high-dimensions the number of
new_simplices can exceed the conflict set and trigger
MaxSimplicesRemovedExceeded prematurely; change the budget to derive from
new_simplices.len() (or the remaining size of new_simplices) instead of
conflict_simplices.len() so repair_budget_remaining is calculated against the
pool actually being modified (update the binding that sets
max_repair_simplices_removed and the subsequent use in repair_budget_remaining
passed to repair_local_facet_issues_with_frontier, referencing new_simplices,
total_removed, and repair_local_facet_issues_with_frontier).
🪄 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: 3b52fbe5-ab56-4bf3-8da4-d339d2e635e2
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/codecov.ymldocs/dev/tooling-alignment.mdjustfilescripts/run_all_examples.shsrc/core/insertion.rssrc/core/repair.rs
✅ Files skipped from review due to trivial changes (1)
- docs/dev/tooling-alignment.md
- Iterate vertex-removal facet repair until facet sharing is valid or the explicit simplex-removal budget is exhausted. - Preserve typed invariant failures when local repair cannot converge. - Size insertion repair budgets from the new simplex pool being repaired.
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.