perf: repair insertion neighbors locally (#335)#366
Merged
acgetchell merged 2 commits intomainfrom May 8, 2026
Merged
Conversation
- Replace post-insertion global neighbor rebuilds with seeded local repair over new cells, removed-cell frontiers, and facet-issue survivors. - Preserve facet-compatible existing neighbor pointers while repairing empty, dangling, or stale local slots. - Add a release-mode escape hatch for forcing global neighbor rebuilds during A/B isolation. - Expose the low-level local repair helper through the insertion prelude and preserve typed neighbor-repair error sources. Closes #335
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 17 |
🟢 Coverage 85.43% diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ 85.43% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (83d9619) 52924 47454 89.66% Head commit (3af15be) 53526 (+602) 47995 (+541) 89.67% (+0.00%) 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 (#366) 645 551 85.43% 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.
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis pull request implements Phase 2/3 of local neighbor-pointer repair for triangulation incremental insertion: it adds a scoped repair API seeded by removed-cell frontiers, threads frontier results through local facet repair and post-removal orchestration, introduces an env-gated global-rebuild fallback, adjusts error shapes, and expands unit and integration tests. ChangesLocal Neighbor-Pointer Repair for Removed-Cell Insertion Path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
========================================
Coverage 89.64% 89.64%
========================================
Files 58 58
Lines 52733 53335 +602
========================================
+ Hits 47270 47811 +541
- Misses 5463 5524 +61
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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: 1
🧹 Nitpick comments (4)
tests/delaunay_incremental_insertion.rs (1)
316-371: ⚡ Quick winAdd at least one adversarial near-boundary insertion per dimension in these new guardrail cases.
The new guardrail suite is useful, but it currently uses only well-conditioned interior points.
Proposed minimal extension (example pattern)
test_local_neighbor_repair_guardrails!( 2, [[0.0, 0.0], [4.0, 0.0], [0.0, 4.0]], - [[0.8, 0.8], [1.6, 0.7], [0.7, 1.5], [1.2, 1.2]] + [[0.8, 0.8], [1.6, 0.7], [0.7, 1.5], [1.2, 1.2], [1.0e-9, 1.3]] );Apply the same idea for 3D–5D with one point near a facet/hyperfacet (tiny coordinate on one axis).
As per coding guidelines
tests/**/*.rs: “Include adversarial inputs (near-boundary points, cospherical sets, degenerate simplices, large coordinates) alongside well-conditioned inputs in both tests and benchmarks”.🤖 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/delaunay_incremental_insertion.rs` around lines 316 - 371, The test suite invocations of the macro test_local_neighbor_repair_guardrails! only use well-conditioned interior points; add at least one adversarial near-boundary insertion for each dimension case (2,3,4,5) by adding a point with a very small coordinate on one axis (e.g., epsilon on one coordinate to place it near a facet/hyperfacet) to the inserted-points list passed into test_local_neighbor_repair_guardrails! so each test includes one near-boundary case alongside the existing interior points; update the parameter arrays for the 2D, 3D, 4D, and 5D macro calls accordingly.src/core/triangulation.rs (1)
248-251: ⚡ Quick winCache the force-global-rebuild flag like the other env-gated switches.
Line 250 does a fresh
env::var_oslookup on every repair-path call, while the nearby debug toggles already useOnceLock<bool>. Since this sits on the insertion hot path in a perf-focused change, caching it would remove repeated environment lookups for free.♻️ Suggested change
+static FORCE_GLOBAL_NEIGHBOR_REBUILD_ENABLED: OnceLock<bool> = OnceLock::new(); + /// Returns whether local neighbor repair should be bypassed for regression isolation. fn force_global_neighbor_rebuild_enabled() -> bool { - env::var_os("DELAUNAY_FORCE_GLOBAL_NEIGHBOR_REBUILD").is_some() + *FORCE_GLOBAL_NEIGHBOR_REBUILD_ENABLED + .get_or_init(|| env::var_os("DELAUNAY_FORCE_GLOBAL_NEIGHBOR_REBUILD").is_some()) }🤖 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/triangulation.rs` around lines 248 - 251, The helper force_global_neighbor_rebuild_enabled currently calls env::var_os("DELAUNAY_FORCE_GLOBAL_NEIGHBOR_REBUILD") on every call; change it to cache the result in a static OnceLock<bool> (like the other env-gated toggles) so subsequent calls return the stored bool instead of performing repeated env lookups; implement initialization that sets the OnceLock from the env var (is_some()) and have force_global_neighbor_rebuild_enabled return the locked value.src/core/algorithms/incremental_insertion.rs (2)
2671-2730: ⚡ Quick winExtract a shared BFS helper to eliminate duplication between the two
validate_no_neighbor_cycles*functions.
validate_no_neighbor_cycles_for_cells(lines 2671–2730) replicates the entire BFS body ofvalidate_no_neighbor_cycles(lines 2611–2669) verbatim; the only difference is how the sample set is sourced. A bug fix or improvement (e.g., the missingtracing::trace!at the end of the local variant that the global one has) would need to be applied in both places.♻️ Proposed refactor — shared BFS helper
+#[cfg(debug_assertions)] +fn validate_no_neighbor_cycles_impl<T, U, V, const D: usize>( + tds: &Tds<T, U, V, D>, + sample_cells: impl Iterator<Item = CellKey>, +) -> Result<(), InsertionError> +where + U: DataType, + V: DataType, +{ + let max_cells = tds.number_of_cells(); + for start_cell in sample_cells.take(10) { + let mut visited: FastHashSet<CellKey> = FastHashSet::default(); + let mut to_visit = vec![start_cell]; + visited.insert(start_cell); + while let Some(current) = to_visit.pop() { + let cell = tds + .cell(current) + .ok_or(NeighborWiringError::MissingCell { cell_key: current })?; + let Some(neighbors) = cell.neighbors() else { continue; }; + for &neighbor_opt in neighbors { + let Some(neighbor_key) = neighbor_opt else { continue; }; + if neighbor_key == current { + return Err(NeighborWiringError::SelfNeighbor { cell_key: current }.into()); + } + if !tds.contains_cell(neighbor_key) { + return Err(NeighborWiringError::MissingNeighborTarget { + cell_key: current, neighbor_key, + }.into()); + } + if visited.insert(neighbor_key) { + to_visit.push(neighbor_key); + if visited.len() > max_cells { + return Err(NeighborWiringError::NeighborWalkExceededCellCount { + visited: visited.len(), total: max_cells, + }.into()); + } + } + } + } + } + Ok(()) +} #[cfg(debug_assertions)] fn validate_no_neighbor_cycles<T, U, V, const D: usize>( tds: &Tds<T, U, V, D>, ) -> Result<(), InsertionError> where U: DataType, V: DataType, { - // ... (entire existing BFS body) + let result = validate_no_neighbor_cycles_impl(tds, tds.cells().map(|(k, _)| k)); + tracing::trace!("validate_no_neighbor_cycles: neighbor walk terminated"); + result } #[cfg(debug_assertions)] fn validate_no_neighbor_cycles_for_cells<T, U, V, const D: usize>( tds: &Tds<T, U, V, D>, sample_cells: &FastHashSet<CellKey>, ) -> Result<(), InsertionError> where U: DataType, V: DataType, { - // ... (entire existing BFS body) + validate_no_neighbor_cycles_impl(tds, sample_cells.iter().copied()) }As per coding guidelines: "Keep code simple and maintainable when multiple correct solutions exist."
4798-4968: ⚡ Quick winAdd dimension-generic tests for
repair_neighbor_pointers_localcovering D=3 through D=5.All five new tests use D=2 exclusively. The existing
test_repair_neighbors!macro demonstrates thepastey-based pattern for the global rebuild; the same approach should be applied to at least the basic local-repair case (reconstructs_missing_slotandreports_non_manifold_incidence).♻️ Sketch of a dimension-generic macro
macro_rules! test_repair_neighbor_pointers_local { ($dim:literal, $initial_vertices:expr) => { pastey::paste! { #[test] fn [<test_repair_neighbor_pointers_local_reconstructs_missing_slot_ $dim d>]() { let vertices = $initial_vertices; let mut dt = DelaunayTriangulation::<_, (), (), $dim>::new(&vertices).unwrap(); let tds = dt.tds_mut(); let (cell_key, facet_idx, neighbor_key, _) = first_neighbor_pair(tds).expect("adjacent cells"); set_neighbor(tds, cell_key, facet_idx, None).unwrap(); let repaired = repair_neighbor_pointers_local(tds, &[cell_key], Some(&[neighbor_key])) .expect("local repair"); assert_eq!(repaired, 1); assert!(tds.is_valid().is_ok()); } } }; } test_repair_neighbor_pointers_local!(3, vec![ vertex!([0.0,0.0,0.0]), vertex!([1.0,0.0,0.0]), vertex!([0.0,1.0,0.0]), vertex!([0.0,0.0,1.0]), vertex!([0.25,0.25,0.25]), ]); // ... D=4, D=5 analoguesAs per coding guidelines: "Dimension-generic tests MUST cover D=2 through D=5 whenever feasible; use
pasteymacros for per-dimension tests."🤖 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/algorithms/incremental_insertion.rs` around lines 4798 - 4968, The tests under repair_neighbor_pointers_local currently only cover D=2; add dimension-generic variants for D=3..=5 by converting the core tests (test_repair_neighbor_pointers_local_reconstructs_missing_slot and test_repair_neighbor_pointers_local_reports_non_manifold_incidence) into a pastey-powered macro that instantiates per-dimension tests, e.g. a macro named test_repair_neighbor_pointers_local that takes $dim and an initial vertex list and emits pasted test function names, and inside each generated test use DelaunayTriangulation::<_,(),(),$dim>, first_neighbor_pair, set_neighbor, and repair_neighbor_pointers_local exactly as the D=2 tests do; ensure you supply appropriate minimal vertex sets for D=3..=5 so first_neighbor_pair returns adjacent cells and keep the same assertions (repaired count and is_valid / NonManifoldTopology match).
🤖 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/triangulation.rs`:
- Around line 4152-4177: The helper repair_neighbors_after_local_cell_removal
currently re-runs a full triangulation facet-sharing validation via
self.tds.validate_facet_sharing(), causing an unnecessary O(N·D) scan; remove
the duplicate validation and the facet_valid variable and associated debug
field/conditional check (including the early Err return of
CavityFillingError::InvalidFacetSharingAfterRepair { stage }) so this function
relies on the callers' prior global validation; keep only the localized debug
information that doesn't call validate_facet_sharing() and ensure any logic that
depended on facet_valid is removed or handled by the callers instead.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 4798-4968: The tests under repair_neighbor_pointers_local
currently only cover D=2; add dimension-generic variants for D=3..=5 by
converting the core tests
(test_repair_neighbor_pointers_local_reconstructs_missing_slot and
test_repair_neighbor_pointers_local_reports_non_manifold_incidence) into a
pastey-powered macro that instantiates per-dimension tests, e.g. a macro named
test_repair_neighbor_pointers_local that takes $dim and an initial vertex list
and emits pasted test function names, and inside each generated test use
DelaunayTriangulation::<_,(),(),$dim>, first_neighbor_pair, set_neighbor, and
repair_neighbor_pointers_local exactly as the D=2 tests do; ensure you supply
appropriate minimal vertex sets for D=3..=5 so first_neighbor_pair returns
adjacent cells and keep the same assertions (repaired count and is_valid /
NonManifoldTopology match).
In `@src/core/triangulation.rs`:
- Around line 248-251: The helper force_global_neighbor_rebuild_enabled
currently calls env::var_os("DELAUNAY_FORCE_GLOBAL_NEIGHBOR_REBUILD") on every
call; change it to cache the result in a static OnceLock<bool> (like the other
env-gated toggles) so subsequent calls return the stored bool instead of
performing repeated env lookups; implement initialization that sets the OnceLock
from the env var (is_some()) and have force_global_neighbor_rebuild_enabled
return the locked value.
In `@tests/delaunay_incremental_insertion.rs`:
- Around line 316-371: The test suite invocations of the macro
test_local_neighbor_repair_guardrails! only use well-conditioned interior
points; add at least one adversarial near-boundary insertion for each dimension
case (2,3,4,5) by adding a point with a very small coordinate on one axis (e.g.,
epsilon on one coordinate to place it near a facet/hyperfacet) to the
inserted-points list passed into test_local_neighbor_repair_guardrails! so each
test includes one near-boundary case alongside the existing interior points;
update the parameter arrays for the 2D, 3D, 4D, and 5D macro calls accordingly.
🪄 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: d13e8809-342e-4181-bbc9-ad450c720fc3
📒 Files selected for processing (6)
docs/dev/debug_env_vars.mdsrc/core/algorithms/incremental_insertion.rssrc/core/triangulation.rssrc/lib.rstests/delaunay_incremental_insertion.rstests/prelude_exports.rs
- Avoid duplicate facet-sharing scans inside local neighbor repair after cell removal while preserving stage-specific failure when repair loops do not restore valid facet sharing. - Cache the force-global-neighbor-rebuild diagnostic toggle after the first environment lookup. - Extend local neighbor repair coverage across dimensions 2 through 5, including non-manifold incidence and near-boundary insertion cases. Closes #335
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.
Closes #335