Feat/locate and insert

[acgetchell]

Simplify and streamline the triangulation architecture by removing legacy code and consolidating APIs:

API Consolidation:

• Migrate DelaunayTriangulation to use internal Tds for all operations
• Remove deprecated Tds::new() constructor (use DelaunayTriangulation::new() instead)
• Consolidate insertion pipeline into unified incremental_insertion module
• Add backward-compatible InsertionError variants (DuplicateCoordinates, DuplicateUuid)
• Fix DelaunayTriangulation kernel type specifications throughout codebase

Code Removal (28,855 deletions):

• Remove obsolete Bowyer-Watson algorithms (bowyer_watson.rs, robust_bowyer_watson.rs)
• Remove unified_insertion_pipeline.rs and insertion_algorithm trait (10,262 lines)
• Remove 9 obsolete test files (tds_basic_integration, insertion_algorithm tests, robust predicates tests)
• Clean up unused boundary analysis and quality metric helpers

Architecture Improvements:

• Consolidate Tds to 9,839 optimized lines (previously duplicated logic)
• Standardize on DelaunayTriangulation::tds() accessor pattern
• Unify boundary analysis through consistent trait implementations
• Streamline facet cache provider with cleaner RCU semantics

Testing & Validation:

• All 721 library tests passing
• All 245 doc tests passing
• Fixed 30+ clippy needless_borrow warnings
• Updated 23 doctests to use delaunay::prelude::*
• Benchmarks compile successfully

Breaking Changes:

• Tds::new() no longer available (use DelaunayTriangulation::new())
• InsertionAlgorithm trait removed (unified into DelaunayTriangulation)
• Some internal quality metric APIs changed signatures

Net: -28,855 lines of code while maintaining full functionality and test coverage.

[coderabbitai]

Walkthrough

Replaces a Tds‑centric API with new Triangulation and DelaunayTriangulation layers, adds kernel abstractions (FastKernel/RobustKernel), implements cavity‑based incremental insertion and point‑location, migrates examples/benches/tests to the new API, removes legacy Bowyer‑Watson modules/tests, and updates CI/benchmark tooling and docs.

Changes

Cohort / File(s)	Summary
Algorithm removals src/core/algorithms/bowyer_watson.rs, src/core/algorithms/unified_insertion_pipeline.rs	Deleted legacy Bowyer‑Watson and unified insertion pipeline implementations, types, trait impls, and associated tests.
New algorithm modules src/core/algorithms/incremental_insertion.rs, src/core/algorithms/locate.rs, src/core/algorithms/flips.rs	Added cavity‑based insertion (fill_cavity, wire_cavity_neighbors, extend_hull) and point‑location/conflict extraction (locate, find_conflict_region, extract_cavity_boundary) with an InsertionError model; flips contains design scaffolding.
Delaunay & Triangulation layers src/core/delaunay_triangulation.rs, src/core/triangulation.rs	Introduced DelaunayTriangulation and Triangulation wrappers with constructors, insert/remove, accessors (tds(), triangulation()), validation, (de)serialization hooks, and tests.
Kernel abstraction src/geometry/kernel.rs	Added Kernel trait plus FastKernel and RobustKernel implementations exposing orientation and in_sphere predicates and config.
API & trait changes src/core/traits/facet_cache.rs, src/core/traits/boundary_analysis.rs	Replaced lenient facet‑cache APIs with fallible try_* variants; relaxed numeric trait bounds on BoundaryAnalysis.
Geometry / quality / util refactor src/geometry/quality.rs, src/core/util.rs, src/geometry/util.rs	Quality/hull/facet utilities now accept Triangulation/DelaunayTriangulation with Kernel generics; generate_random_triangulation now returns DelaunayTriangulation.
Collections & vertex simplification src/core/collections.rs, src/core/vertex.rs	Added FacetIssuesMap, CellSecondaryMap, VertexSecondaryMap aliases; standardized no‑data vertex usage to U = () (macros/docs/tests updated).
Boundary & facet call‑site migrations src/core/boundary.rs, src/core/facet.rs	Updated call sites, docs, and tests to use DelaunayTriangulation and dt.tds() / dt.triangulation(); algorithm internals unchanged.
Benches migrated / removed benches/* (e.g., benches/assign_neighbors_performance.rs removed; benches/microbenchmarks.rs, benches/large_scale_performance.rs, benches/profiling_suite.rs updated)	Removed one bench file; remaining benches refactored to construct and measure DelaunayTriangulation and use dt.tds().
Examples migrated examples/* (convex_hull_3d_20_points.rs, triangulation_3d_20_points.rs, memory_analysis.rs, zero_allocation_iterator_demo.rs, into_from_conversions.rs)	Examples updated to construct/use DelaunayTriangulation (generic kernels) and to call through dt.tds() / dt.triangulation(); several function signatures generalized.
Tests: migration, removals & additions tests/* (many; new: tests/delaunay_edge_cases.rs, tests/delaunay_incremental_insertion.rs, tests/proptest_delaunay_triangulation.rs; many legacy tests removed)	Large migration from Tds → DelaunayTriangulation, vertex generic simplification (Option<()> → ()); many legacy Bowyer‑Watson/robust property tests removed and new Delaunay‑focused tests added.
Docs, tooling & CI WARP.md, cspell.json, justfile, .github/workflows/ci.yml, scripts/*, .github/workflows/benchmarks.yml	Updated WARP.md guidance, added cspell words, CI now runs integration tests on Windows, removed CRITERION_QUIET_MODE and --quiet bench flag, improved benchmark error reporting and scripts, adjusted justfile/workflows.
Misc: deps & small edits Cargo.toml, scripts/changelog_utils.py, scripts/benchmark_utils.py, scripts/tests/test_benchmark_utils.py	Bumped slotmap, uuid, and criterion; removed a bench entry; small script argument/keyword changes; added benchmark error‑file writer and tests.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant DT as DelaunayTriangulation
  participant Tri as Triangulation
  participant Kernel
  Note over Client,DT: High‑level insertion & locate flow
  Client->>DT: insert(vertex)
  DT->>Tri: triangulation.insert(vertex, hint?)
  Tri->>Kernel: orientation / in_sphere(predicate inputs)
  Kernel-->>Tri: predicate result (+1/0/-1 or Err)
  Tri->>Tri: locate / find_conflict_region (facet‑walk / BFS)
  Tri->>Tri: extract_cavity_boundary (orient & dedupe facets)
  Tri->>Tri: fill_cavity (create new cells)
  Tri->>Tri: wire_cavity_neighbors (link new cells & neighbors)
  Tri-->>DT: updated internal TDS view
  DT-->>Client: return VertexKey | InsertionError

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

• Areas needing extra attention:
  • src/core/algorithms/incremental_insertion.rs — cavity creation, neighbor wiring, non‑manifold detection, and retry semantics.
  • src/core/algorithms/locate.rs — facet‑walking correctness, degeneracy handling, cycle detection.
  • src/core/delaunay_triangulation.rs & src/core/triangulation.rs — construction/insert/remove semantics, (de)serialization, public accessors.
  • src/geometry/kernel.rs — numeric predicate correctness and interplay between FastKernel and RobustKernel.
  • Test suite removals — verify coverage gaps from many removed Bowyer‑Watson/robust property suites and ensure new tests sufficiently exercise edge cases.

Possibly related PRs

• Changed: Optimizes internal collections with key-based access #83 — overlaps on key/slotmap refactors and key‑based collection changes affecting core collections and facet maps.
• feat(geometry): migrate to nalgebra and add adaptive tolerance #122 — overlapping changes to geometry/predicate layers (matrix, adaptive tolerance, predicates).
• fix: Handle degenerate configurations and improve test robustness #116 — related work on triangulation construction, duplicate/degenerate handling and proptest improvements.

Poem

🐇 I nibbled kernels, found a hole,

I stitched the facets, made them whole,
Hopped from TDS to DT with cheer,
Wired neighbors, drew them near,
Small hop, big mesh — triangulations steer.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Feat/locate and insert' is vague and does not clearly convey the main changes, which focus on API consolidation, code removal, and architecture improvements rather than 'locate and insert' functionality.	Consider revising the title to better reflect the primary changes, such as 'Refactor: Consolidate triangulation APIs and remove legacy algorithms' or 'Refactor: Streamline insertion pipeline and remove Bowyer-Watson implementations'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/locate-and-insert

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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)

examples/triangulation_3d_20_points.rs (1)

16-16: Documentation typo: example command references wrong file.

The usage section mentions triangulation_3d_100_points but this example is for 20 points.

-//! cargo run --example triangulation_3d_100_points
+//! cargo run --example triangulation_3d_20_points

🧹 Nitpick comments (21)

src/core/algorithms/incremental_insertion.rs (1)

466-494: Accessing internal field directly in test.

The test accesses dt.tri.tds directly to get mutable access to the TDS, which bypasses the public API (dt.tds() returns an immutable reference). While acceptable for testing internal functions like fill_cavity, consider adding a tds_mut() accessor if mutable TDS access is needed in other test scenarios.

src/core/collections.rs (1)

910-995: New CellSecondaryMap/VertexSecondaryMap aliases look correct; consider a tiny smoke test

The CellSecondaryMap<V> = SparseSecondaryMap<CellKey, V> and VertexSecondaryMap<V> = SparseSecondaryMap<VertexKey, V> aliases are an appropriate way to model sparse auxiliary per-key state and the docs clearly emphasize typical algorithmic use cases (visited/marked flags, conflict sets, etc.). As an optional improvement, you could add a trivial instantiation check alongside the existing collection smoke tests to ensure these types stay wired up:

@@ fn test_collection_type_instantiation() {
-        // Test Phase 1 key-based types compile and instantiate
+        // Test Phase 1 key-based types compile and instantiate
         let _cell_set: CellKeySet = CellKeySet::default();
         let _vertex_set: VertexKeySet = VertexKeySet::default();
         let _cell_map: KeyBasedCellMap<i32> = KeyBasedCellMap::default();
         let _vertex_map: KeyBasedVertexMap<String> = KeyBasedVertexMap::default();
+
+        // Test SlotMap secondary maps for auxiliary data compile and instantiate
+        let _cell_aux: CellSecondaryMap<bool> = CellSecondaryMap::new();
+        let _vertex_aux: VertexSecondaryMap<usize> = VertexSecondaryMap::new();

Not required, but it would keep these new aliases covered in the same “type instantiation” test.

Also applies to: 1205-1244

src/core/traits/boundary_analysis.rs (1)

23-41: BoundaryAnalysis examples correctly pivot to DelaunayTriangulation + tds()

The updated docs now show the intended flow of creating a DelaunayTriangulation, obtaining tds via dt.tds(), and then calling the trait methods, which is consistent with the rest of the refactor. As a small optional polish, you might have the number_of_boundary_facets example explicitly call tds.number_of_boundary_facets()? once to demonstrate that API directly instead of only using dt.boundary_facets().count().

Also applies to: 65-80, 107-124, 154-176, 201-214

examples/memory_analysis.rs (1)

6-6: Memory analysis example now correctly operates on DelaunayTriangulation + accessors

Switching to DelaunayTriangulation—captured as dt—and then using dt.tds() for counts and ConvexHull::from_triangulation(dt.triangulation()) keeps the example aligned with the new public API while preserving the allocation-measurement behavior. If you touch this again, renaming tds_res to something like tri_res or dt_res would better reflect the returned type, but it’s not functionally important.

Also applies to: 21-29, 30-40, 43-45

src/core/facet.rs (1)

1006-1760: Prefer the public tds() accessor over reaching into triangulation().tds in tests

In multiple tests here (e.g., Lines 1015, 1033, 1055, 1067, 1087, 1103, 1135–1137, 1158, 1217, 1256–1257, 1273–1274, 1280, 1291–1293, 1311, 1362–1363, 1431, 1435, 1452, 1471–1474, 1502–1503, 1507, 1577, 1582, 1597, 1627, 1631–1640, 1657, 1676, 1703–1705, 1726, 1742) the code accesses the underlying TDS via &dt.triangulation().tds when constructing FacetViews or calling helpers.

Functionally this works, but it bypasses the new wrapper boundary and hard‑codes knowledge of Triangulation’s internal field layout. For consistency with the rest of the PR and to reduce coupling to internals, consider switching these call sites to the public accessor:

- let facet = FacetView::new(&dt.triangulation().tds, cell_key, 0).unwrap();
+ let facet = FacetView::new(dt.tds(), cell_key, 0).unwrap();

and similarly elsewhere (including calls like all_facets_for_cell(&dt.triangulation().tds, cell_key)), so tests exercise the same API surface regular users do.

tests/proptest_duplicates.rs (1)

100-155: Property tests correctly target DelaunayTriangulation, consider trimming #[allow(deprecated)]

The switch to Vertex<f64, (), D> + DelaunayTriangulation::<_, (), (), D>::new(&vertices) and then validating via tds.is_valid() and core::util::is_delaunay(tds) keeps these properties equivalent while exercising the new public API, which is good.

If none of the APIs used here are actually deprecated anymore, you can drop the #[allow(deprecated)] attributes on these tests to avoid accidentally masking future deprecation warnings:

-    #[allow(deprecated)]
     fn prop_cloud_with_duplicates_is_delaunay_for_kept_subset_2d(…)

(and similarly for the 3D variant). This is purely cleanup; behavior is already correct.

tests/proptest_serialization.rs (1)

40-202: Serialization properties correctly target DelaunayTriangulation end‑to‑end

The proptests now round‑trip the full DelaunayTriangulation (including number_of_vertices, number_of_cells, dim, is_valid, vertices, cells) rather than a bare Tds, which better matches real‑world usage while preserving the original checks on structure, validity, coordinates (with tolerance), and neighbor counts.

If you want to reduce verbosity, you could also use the imported DelaunayTriangulation alias instead of fully‑qualified delaunay::core::delaunay_triangulation::DelaunayTriangulation inside the macros, but that’s purely cosmetic.

benches/large_scale_performance.rs (1)

10-11: Consider updating stale documentation reference.

The doc comment mentions "Bowyer-Watson algorithm" but per the PR objectives, bowyer_watson.rs and robust_bowyer_watson.rs have been removed. Consider updating this to reflect the current incremental insertion algorithm.

src/core/vertex.rs (1)

494-506: Note: set_point is marked as dead code.

The #[allow(dead_code)] annotation indicates this method is currently unused. Consider either:

1. Using it in the codebase if it's intended for the new API
2. Removing it if it's truly obsolete after the consolidation

This is a minor cleanup opportunity, not a blocking issue.

tests/proptest_delaunay_condition.rs (2)

81-82: Stale comment references removed Tds::new().

The comment on line 81 mentions "Tds::new()" but the code now uses DelaunayTriangulation::new(). Update for consistency.

-                    // Build Delaunay triangulation using Tds::new() which properly triangulates all vertices
-                    // This ensures the entire triangulation (including initial simplex) satisfies Delaunay property
+                    // Build Delaunay triangulation using DelaunayTriangulation::new() which properly triangulates all vertices
+                    // This ensures the entire triangulation (including initial simplex) satisfies Delaunay property

---

127-138: Stale comments reference old Tds::new() API.

Lines 128, 131-132, and 135 reference Tds::new() but the test now uses DelaunayTriangulation::new(). Consider updating these comments for consistency with the new API.

-    /// **Status**: Ignored - requires algorithmic fixes to Tds::new() for insertion-order determinism.
+    /// **Status**: Ignored - requires algorithmic fixes to DelaunayTriangulation::new() for insertion-order determinism.
     ///
     /// Current findings:
-    /// - `Tds::new()` produces different triangulations (different vertex counts, cell counts)
+    /// - `DelaunayTriangulation::new()` produces different triangulations (different vertex counts, cell counts)
     ///   when the same vertices are provided in different orders

src/core/triangulation.rs (3)

161-169: Consider returning Result instead of panicking.

The boundary_facets method panics on corrupted triangulation structure. While the comment explains this indicates a library bug, returning a Result would be more idiomatic and allow callers to handle corruption gracefully if needed.

If preserving the current behavior is preferred, consider at minimum adding this to the function signature documentation to make the panic behavior explicit in the type system:

-    pub fn boundary_facets(&self) -> BoundaryFacetsIter<'_, K::Scalar, U, V, D> {
+    pub fn boundary_facets(&self) -> BoundaryFacetsIter<'_, K::Scalar, U, V, D>
+    where
+        K::Scalar: crate::geometry::traits::coordinate::CoordinateScalar,
+    {

Alternatively, expose a fallible version alongside the panicking one for users who need error handling.

---

205-206: Unused iteration variable.

The iteration variable is marked with #[allow(unused_variables)] but is never actually used, even in debug builds. If it's intended for future debugging, consider adding a debug_assert! or removing the variable entirely.

-        #[allow(unused_variables)] // iteration only used in debug_assertions
-        for iteration in 0..MAX_FIX_FACET_ITERATIONS {
+        for _iteration in 0..MAX_FIX_FACET_ITERATIONS {

---

346-360: Error suppression may mask issues during topology repair.

The error handling here uses continue on failure, which silently proceeds to the next iteration. This could mask underlying issues with the triangulation. Consider logging these failures or accumulating them for the caller.

If this is intentional (best-effort repair), document this behavior clearly. Otherwise, consider propagating errors:

             let Ok(duplicate_cells_removed) = self.tds.remove_duplicate_cells() else {
-                total_removed += actually_removed;
-                continue;
+                // Log or accumulate the error before continuing
+                #[cfg(debug_assertions)]
+                eprintln!("Warning: remove_duplicate_cells failed during facet repair");
+                total_removed += actually_removed;
+                continue;
             };

benches/microbenchmarks.rs (1)

1-11: Stale documentation: remove_duplicate_cells benchmark removed.

Line 7 mentions remove_duplicate_cells as one of the benchmarked methods, but this benchmark has been removed from the file. Update the documentation to reflect current benchmarks.

 //! 1. **`DelaunayTriangulation::with_kernel`**: Complete triangulation creation
-//! 2. **`remove_duplicate_cells`**: Duplicate cell removal and cleanup
-//! 3. **`is_valid`**: Complete triangulation validation performance
-//! 4. **Individual validation components**: Mapping validation, duplicate detection, etc.
-//! 5. **Incremental construction**: Performance of `insert()` method for vertex insertion
-//! 6. **Memory usage patterns**: Allocation and deallocation patterns
+//! 2. **`is_valid`**: Complete triangulation validation performance
+//! 3. **Individual validation components**: Mapping validation, duplicate detection, etc.
+//! 4. **Incremental construction**: Performance of `insert()` method for vertex insertion
+//! 5. **Memory usage patterns**: Allocation and deallocation patterns

src/lib.rs (1)

48-96: Consider removing no_run or verify it's intentional.

This convex hull example uses no_run, but the code is syntactically valid and all required imports are available from the prelude. However, given that no_run is applied universally across all examples in this codebase (not just this one), this may be an intentional design pattern rather than an oversight. If the project intends to maintain executable doctest examples, consider removing no_run; otherwise, this is consistent with project convention.

src/core/algorithms/locate.rs (1)

30-43: Consider documenting or removing unused LocateResult variants.

The OnFacet, OnEdge, and OnVertex variants are defined but never returned by the current locate() implementation, which only returns InsideCell or Outside. If these are intended for future use, consider adding a note; otherwise, they may cause confusion.

src/geometry/util.rs (2)

2099-2151: API migration looks correct.

The function signature and implementation properly align with the new DelaunayTriangulation-centric API. The construction flow using FastKernel::new() and DelaunayTriangulation::with_kernel() is consistent with the patterns shown in the relevant code snippets.

One minor observation: the error mapping wraps RandomPointGenerationError into TriangulationConstructionError::GeometricDegeneracy, which semantically conflates point generation failures (invalid bounds, RNG issues) with geometric degeneracy. This works but slightly misrepresents the error source. Consider whether a dedicated error variant would be clearer, though this is a low-priority refinement.

---

4864-4908: Track ignored tests for follow-up.

Two tests are marked with #[ignore = "Needs to be reviewed"] which is a generic placeholder. Consider:

1. Creating issues to track when these tests should be re-enabled
2. Documenting the specific failure modes that led to ignoring them

The test at line 4795 has a clear reason ("cavity filling errors"), but the tests at lines 4864 and 4908 would benefit from more specific ignore reasons to aid future debugging.

src/core/delaunay_triangulation.rs (2)

651-658: Consider logging or propagating facet sharing repair failures in release builds.

The current implementation silently discards errors from fix_invalid_facet_sharing() in release builds (line 657). While this prevents failures during insertion, it could mask subtle geometry issues that accumulate over multiple insertions.

Consider at minimum adding a counter or flag that users can query to detect if repairs were needed, or returning a result type that indicates "success with warnings."

-                #[cfg(not(debug_assertions))]
-                let _ = self.tri.fix_invalid_facet_sharing();
+                #[cfg(not(debug_assertions))]
+                if self.tri.fix_invalid_facet_sharing().is_err() {
+                    // Repair failed but triangulation is still usable
+                    // Consider exposing this state via a method like needs_repair()
+                }

---

673-679: Acknowledge TODO for degenerate point locations.

The OnFacet, OnEdge, and OnVertex cases are correctly flagged as unimplemented. The error message is informative for debugging.

Would you like me to open an issue to track implementing these degenerate cases, or help draft the implementation for handling points on facets/edges/vertices?

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14c81a0 and 35ea9bb.

📒 Files selected for processing (54)

• WARP.md (1 hunks)
• benches/assign_neighbors_performance.rs (11 hunks)
• benches/large_scale_performance.rs (7 hunks)
• benches/microbenchmarks.rs (9 hunks)
• benches/profiling_suite.rs (9 hunks)
• cspell.json (2 hunks)
• examples/convex_hull_3d_20_points.rs (18 hunks)
• examples/into_from_conversions.rs (2 hunks)
• examples/memory_analysis.rs (3 hunks)
• examples/triangulation_3d_20_points.rs (10 hunks)
• examples/zero_allocation_iterator_demo.rs (6 hunks)
• src/core/algorithms/bowyer_watson.rs (0 hunks)
• src/core/algorithms/flips.rs (1 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/algorithms/locate.rs (1 hunks)
• src/core/algorithms/unified_insertion_pipeline.rs (0 hunks)
• src/core/boundary.rs (22 hunks)
• src/core/collections.rs (4 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/facet.rs (37 hunks)
• src/core/traits/boundary_analysis.rs (6 hunks)
• src/core/traits/facet_cache.rs (39 hunks)
• src/core/triangulation.rs (1 hunks)
• src/core/util.rs (50 hunks)
• src/core/vertex.rs (36 hunks)
• src/geometry/kernel.rs (1 hunks)
• src/geometry/quality.rs (33 hunks)
• src/geometry/util.rs (21 hunks)
• src/lib.rs (17 hunks)
• tests/allocation_api.rs (4 hunks)
• tests/circumsphere_debug_tools.rs (4 hunks)
• tests/convex_hull_bowyer_watson_integration.rs (0 hunks)
• tests/delaunay_edge_cases.rs (1 hunks)
• tests/delaunay_incremental_insertion.rs (1 hunks)
• tests/integration_robust_bowyer_watson.rs (0 hunks)
• tests/proptest_bowyer_watson.rs (0 hunks)
• tests/proptest_cell.rs (6 hunks)
• tests/proptest_convex_hull.rs (9 hunks)
• tests/proptest_delaunay_condition.proptest-regressions (1 hunks)
• tests/proptest_delaunay_condition.rs (5 hunks)
• tests/proptest_duplicates.rs (2 hunks)
• tests/proptest_facet.rs (6 hunks)
• tests/proptest_facet_cache.rs (0 hunks)
• tests/proptest_invariants.rs (3 hunks)
• tests/proptest_quality.rs (14 hunks)
• tests/proptest_robust_bowyer_watson.rs (0 hunks)
• tests/proptest_serialization.rs (7 hunks)
• tests/proptest_triangulation.rs (10 hunks)
• tests/proptest_vertex.rs (10 hunks)
• tests/regression_delaunay_known_configs.rs (0 hunks)
• tests/robust_predicates_comparison.rs (0 hunks)
• tests/robust_predicates_showcase.rs (0 hunks)
• tests/serialization_vertex_preservation.rs (6 hunks)
• tests/storage_backend_compatibility.rs (15 hunks)

💤 Files with no reviewable changes (10)

• tests/proptest_bowyer_watson.rs
• tests/proptest_robust_bowyer_watson.rs
• tests/robust_predicates_showcase.rs
• tests/proptest_facet_cache.rs
• tests/integration_robust_bowyer_watson.rs
• tests/convex_hull_bowyer_watson_integration.rs
• src/core/algorithms/bowyer_watson.rs
• tests/robust_predicates_comparison.rs
• tests/regression_delaunay_known_configs.rs
• src/core/algorithms/unified_insertion_pipeline.rs

🧰 Additional context used

🧠 Learnings (12)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/algorithms/flips.rs
• tests/proptest_facet.rs
• examples/memory_analysis.rs
• src/core/boundary.rs
• benches/large_scale_performance.rs
• tests/delaunay_incremental_insertion.rs
• tests/delaunay_edge_cases.rs
• examples/zero_allocation_iterator_demo.rs
• src/core/traits/boundary_analysis.rs
• tests/proptest_convex_hull.rs
• tests/proptest_triangulation.rs
• tests/proptest_duplicates.rs
• src/core/facet.rs
• tests/allocation_api.rs
• examples/into_from_conversions.rs
• tests/proptest_cell.rs
• src/geometry/quality.rs
• src/core/algorithms/locate.rs
• src/core/triangulation.rs
• benches/profiling_suite.rs
• src/core/algorithms/incremental_insertion.rs
• tests/storage_backend_compatibility.rs
• benches/microbenchmarks.rs
• src/geometry/kernel.rs
• examples/convex_hull_3d_20_points.rs
• tests/proptest_quality.rs
• src/core/traits/facet_cache.rs
• tests/proptest_invariants.rs
• tests/serialization_vertex_preservation.rs
• src/geometry/util.rs
• examples/triangulation_3d_20_points.rs
• src/core/vertex.rs
• tests/proptest_vertex.rs
• tests/proptest_delaunay_condition.rs
• src/lib.rs
• tests/proptest_serialization.rs
• src/core/delaunay_triangulation.rs
• src/core/util.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/algorithms/flips.rs
• tests/proptest_facet.rs
• examples/memory_analysis.rs
• src/core/boundary.rs
• benches/large_scale_performance.rs
• tests/delaunay_incremental_insertion.rs
• tests/delaunay_edge_cases.rs
• examples/zero_allocation_iterator_demo.rs
• src/core/traits/boundary_analysis.rs
• tests/proptest_convex_hull.rs
• tests/proptest_triangulation.rs
• tests/proptest_duplicates.rs
• src/core/facet.rs
• tests/allocation_api.rs
• tests/proptest_cell.rs
• src/geometry/quality.rs
• src/core/algorithms/locate.rs
• benches/profiling_suite.rs
• src/core/algorithms/incremental_insertion.rs
• tests/storage_backend_compatibility.rs
• benches/microbenchmarks.rs
• examples/convex_hull_3d_20_points.rs
• tests/proptest_quality.rs
• src/core/traits/facet_cache.rs
• tests/proptest_invariants.rs
• tests/serialization_vertex_preservation.rs
• src/geometry/util.rs
• examples/triangulation_3d_20_points.rs
• src/core/vertex.rs
• tests/proptest_delaunay_condition.rs
• src/lib.rs
• tests/proptest_serialization.rs
• src/core/delaunay_triangulation.rs
• src/core/util.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• tests/proptest_facet.rs
• examples/memory_analysis.rs
• src/core/boundary.rs
• benches/large_scale_performance.rs
• tests/circumsphere_debug_tools.rs
• tests/proptest_delaunay_condition.proptest-regressions
• tests/delaunay_incremental_insertion.rs
• tests/delaunay_edge_cases.rs
• examples/zero_allocation_iterator_demo.rs
• src/core/traits/boundary_analysis.rs
• tests/proptest_convex_hull.rs
• tests/proptest_triangulation.rs
• tests/proptest_duplicates.rs
• src/core/facet.rs
• tests/allocation_api.rs
• benches/assign_neighbors_performance.rs
• tests/proptest_cell.rs
• src/geometry/quality.rs
• src/core/algorithms/locate.rs
• benches/profiling_suite.rs
• src/core/algorithms/incremental_insertion.rs
• tests/storage_backend_compatibility.rs
• benches/microbenchmarks.rs
• src/geometry/kernel.rs
• examples/convex_hull_3d_20_points.rs
• tests/proptest_quality.rs
• src/core/traits/facet_cache.rs
• tests/proptest_invariants.rs
• tests/serialization_vertex_preservation.rs
• src/geometry/util.rs
• examples/triangulation_3d_20_points.rs
• src/core/vertex.rs
• tests/proptest_vertex.rs
• tests/proptest_delaunay_condition.rs
• src/lib.rs
• tests/proptest_serialization.rs
• src/core/delaunay_triangulation.rs
• src/core/util.rs

📚 Learning: 2025-08-28T03:54:34.371Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 54
File: scripts/generate_changelog.sh:417-438
Timestamp: 2025-08-28T03:54:34.371Z
Learning: The generate_changelog.sh script uses a deliberate design pattern where the auto-changelog template uses simple generic headers (### Changes, ### Fixed Issues) and the enhancer function transforms these into Keep a Changelog format sections (### Added, ### Changed, ### Fixed, etc.). This separation keeps the template simple while ensuring standardized output format.

Applied to files:

• WARP.md

📚 Learning: 2025-09-02T20:32:05.985Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 60
File: cspell.json:103-103
Timestamp: 2025-09-02T20:32:05.985Z
Learning: In cspell.json for the delaunay project, the word "itional" is intentionally added to the dictionary because it comes from a regex pattern, not a typo.

Applied to files:

• cspell.json

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• tests/storage_backend_compatibility.rs

📚 Learning: 2025-09-17T17:51:52.098Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 83
File: src/core/traits/facet_cache.rs:533-585
Timestamp: 2025-09-17T17:51:52.098Z
Learning: The build_cache_with_rcu method in FacetCacheProvider returns Option<Arc<FacetToCellsMap>> representing the old cache value before the RCU update operation, not a tuple with a "built" flag. First call returns None (no old value), subsequent calls return Some(old_cache).

Applied to files:

• src/core/traits/facet_cache.rs
• src/core/util.rs

📚 Learning: 2025-09-17T17:53:34.532Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 83
File: src/core/traits/facet_cache.rs:588-652
Timestamp: 2025-09-17T17:53:34.532Z
Learning: The build_cache_with_rcu method in FacetCacheProvider correctly returns Option<Arc<FacetToCellsMap>> representing the old cache value before the RCU update operation. The test logic using old_value.is_none() to detect which thread built the cache is correct: None means this thread built it, Some(old_cache) means another thread already built it.

Applied to files:

• src/core/traits/facet_cache.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait guarantees PartialOrd, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• src/core/vertex.rs
• tests/proptest_vertex.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait includes PartialOrd through its Float supertrait bound, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• src/core/vertex.rs
• tests/proptest_vertex.rs

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• src/lib.rs

🧬 Code graph analysis (31)

tests/proptest_facet.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• vertices (439-441)
• tds (468-470)

src/core/boundary.rs (1)

src/core/delaunay_triangulation.rs (6)

• new (102-104)
• vertices (439-441)
• number_of_cells (348-350)
• dim (375-377)
• boundary_facets (556-558)
• tds (468-470)

benches/large_scale_performance.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• vertices (439-441)
• tds (468-470)

tests/circumsphere_debug_tools.rs (2)

src/core/vertex.rs (1)

• point (475-477)

src/geometry/point.rs (1)

• to_array (121-123)

tests/delaunay_incremental_insertion.rs (1)

src/core/delaunay_triangulation.rs (4)

• vertices (439-441)
• new (102-104)
• insert (604-680)
• with_kernel (197-233)

tests/delaunay_edge_cases.rs (6)

src/geometry/util.rs (1)

• core (1809-1809)

src/core/delaunay_triangulation.rs (5)

• vertices (439-441)
• new (102-104)
• number_of_vertices (322-324)
• number_of_cells (348-350)
• with_kernel (197-233)

src/core/triangulation.rs (3)

• vertices (75-77)
• number_of_vertices (81-83)
• number_of_cells (87-89)

src/core/cell.rs (3)

• vertices (593-595)
• new (389-420)
• number_of_vertices (682-684)

src/core/triangulation_data_structure.rs (3)

• vertices (817-819)
• number_of_vertices (910-912)
• number_of_cells (1050-1052)

src/core/util.rs (2)

• geometry (1677-1677)
• geometry (1755-1755)

examples/zero_allocation_iterator_demo.rs (3)

src/geometry/util.rs (17)

• core (1809-1809)
• generate_random_triangulation (2094-2152)
• generate_random_triangulation (4799-4799)
• generate_random_triangulation (4816-4816)
• generate_random_triangulation (4833-4833)
• generate_random_triangulation (4837-4837)
• generate_random_triangulation (4867-4867)
• generate_random_triangulation (4877-4877)
• generate_random_triangulation (4888-4888)
• generate_random_triangulation (4892-4892)
• generate_random_triangulation (4918-4918)
• generate_random_triangulation (4925-4925)
• generate_random_triangulation (4932-4932)
• generate_random_triangulation (4939-4939)
• generate_random_triangulation (4952-4952)
• generate_random_triangulation (4980-4980)
• generate_random_triangulation (4998-4998)

src/core/vertex.rs (2)

• None (1921-1921)
• uuid (531-533)

src/core/cell.rs (2)

• cell (3224-3226)
• uuid (710-712)

tests/proptest_triangulation.rs (1)

src/core/delaunay_triangulation.rs (8)

• new (102-104)
• vertices (439-441)
• is_valid (769-774)
• tds (468-470)
• cells (407-409)
• number_of_vertices (322-324)
• number_of_cells (348-350)
• dim (375-377)

tests/proptest_duplicates.rs (3)

src/core/delaunay_triangulation.rs (4)

• vertices (439-441)
• new (102-104)
• tds (468-470)
• is_valid (769-774)

src/core/vertex.rs (2)

• from_points (419-424)
• is_valid (611-629)

src/core/util.rs (1)

• is_delaunay (1662-1694)

src/core/facet.rs (1)

src/core/delaunay_triangulation.rs (4)

• vertices (439-441)
• triangulation (496-498)
• tds (468-470)
• cells (407-409)

tests/allocation_api.rs (2)

src/core/delaunay_triangulation.rs (4)

• empty (119-121)
• vertices (439-441)
• new (102-104)
• number_of_vertices (322-324)

src/core/triangulation.rs (2)

• vertices (75-77)
• number_of_vertices (81-83)

benches/assign_neighbors_performance.rs (1)

src/core/delaunay_triangulation.rs (3)

• vertices (439-441)
• new (102-104)
• tds (468-470)

tests/proptest_cell.rs (2)

src/core/delaunay_triangulation.rs (4)

• new (102-104)
• vertices (439-441)
• cells (407-409)
• is_valid (769-774)

src/core/triangulation.rs (2)

• vertices (75-77)
• cells (68-70)

src/geometry/quality.rs (4)

src/core/delaunay_triangulation.rs (5)

• triangulation (496-498)
• new (102-104)
• vertices (439-441)
• cells (407-409)
• with_kernel (197-233)

src/core/triangulation_data_structure.rs (3)

• vertices (817-819)
• cells (750-752)
• cells (3686-3695)

src/geometry/point.rs (2)

• new (115-117)
• coords (100-102)

src/core/triangulation.rs (2)

• vertices (75-77)
• cells (68-70)

src/core/algorithms/locate.rs (3)

src/core/facet.rs (11)

• tds (411-413)
• key (580-602)
• cell (561-565)
• cell_key (289-291)
• cell_key (397-399)
• opposite_vertex (529-550)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• vertices (496-518)

src/core/triangulation_data_structure.rs (2)

• vertices (817-819)
• assign_neighbors (605-719)

src/core/boundary.rs (1)

• boundary_facets (73-81)

src/core/triangulation.rs (4)

src/core/facet.rs (9)

• cell (561-565)
• vertices (496-518)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• key (580-602)
• cell_key (289-291)
• cell_key (397-399)

src/geometry/quality.rs (1)

• radius_ratio (223-276)

src/core/triangulation_data_structure.rs (7)

• empty (2347-2356)
• cells (750-752)
• cells (3686-3695)
• vertices (817-819)
• number_of_vertices (910-912)
• dim (991-997)
• default (212-214)

src/core/boundary.rs (1)

• boundary_facets (73-81)

benches/profiling_suite.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• vertices (439-441)
• tds (468-470)

src/core/algorithms/incremental_insertion.rs (10)

src/core/algorithms/locate.rs (1)

• locate (193-266)

src/core/facet.rs (11)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)

src/core/cell.rs (5)

• cell (3224-3226)
• uuid (710-712)
• new (389-420)
• neighbors (561-563)
• vertices (593-595)

src/core/vertex.rs (7)

• point (475-477)
• from (707-709)
• from (720-722)
• from (733-735)
• from (798-800)
• uuid (531-533)
• None (1921-1921)

src/core/delaunay_triangulation.rs (5)

• tds (468-470)
• boundary_facets (556-558)
• new (102-104)
• cells (407-409)
• vertices (439-441)

src/core/boundary.rs (1)

• boundary_facets (73-81)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (81-83)

src/core/triangulation.rs (3)

• boundary_facets (161-169)
• cells (68-70)
• vertices (75-77)

src/core/traits/facet_cache.rs (1)

• new (352-357)

src/core/triangulation_data_structure.rs (4)

• default (212-214)
• cells (750-752)
• cells (3686-3695)
• vertices (817-819)

benches/microbenchmarks.rs (2)

src/core/delaunay_triangulation.rs (9)

• vertices (439-441)
• with_kernel (197-233)
• tds (468-470)
• number_of_vertices (322-324)
• number_of_cells (348-350)
• is_valid (769-774)
• validate_vertex_mappings (784-786)
• validate_cell_mappings (835-837)
• insert (604-680)

src/core/triangulation_data_structure.rs (6)

• vertices (817-819)
• number_of_vertices (910-912)
• number_of_cells (1050-1052)
• is_valid (3252-3266)
• validate_vertex_mappings (2917-2955)
• validate_cell_mappings (2999-3037)

src/geometry/kernel.rs (2)

src/geometry/predicates.rs (2)

• insphere (349-424)
• simplex_orientation (111-159)

src/geometry/robust_predicates.rs (4)

• robust_insphere (149-192)
• robust_orientation (303-337)
• general_triangulation (736-746)
• high_precision (762-773)

examples/convex_hull_3d_20_points.rs (4)

src/geometry/util.rs (28)

• generate_random_triangulation (2094-2152)
• generate_random_triangulation (4799-4799)
• generate_random_triangulation (4816-4816)
• generate_random_triangulation (4833-4833)
• generate_random_triangulation (4837-4837)
• generate_random_triangulation (4867-4867)
• generate_random_triangulation (4877-4877)
• generate_random_triangulation (4888-4888)
• generate_random_triangulation (4892-4892)
• generate_random_triangulation (4918-4918)
• generate_random_triangulation (4925-4925)
• generate_random_triangulation (4932-4932)
• generate_random_triangulation (4939-4939)
• generate_random_triangulation (4952-4952)
• generate_random_triangulation (4980-4980)
• generate_random_triangulation (4998-4998)
• std (251-251)
• std (291-291)
• std (512-512)
• std (520-520)
• std (989-989)
• std (1020-1020)
• std (1060-1060)
• std (1263-1263)
• std (1304-1304)
• std (2382-2382)
• std (2429-2429)
• std (5162-5162)

src/core/delaunay_triangulation.rs (5)

• tds (468-470)
• number_of_vertices (322-324)
• number_of_cells (348-350)
• dim (375-377)
• new (102-104)

src/core/triangulation.rs (3)

• number_of_vertices (81-83)
• number_of_cells (87-89)
• dim (93-95)

src/geometry/algorithms/convex_hull.rs (1)

• from_triangulation (682-728)

src/core/traits/facet_cache.rs (2)

src/core/delaunay_triangulation.rs (4)

• vertices (439-441)
• new (102-104)
• tds (468-470)
• empty (119-121)

src/core/triangulation_data_structure.rs (3)

• vertices (817-819)
• empty (2347-2356)
• generation (1079-1081)

tests/proptest_invariants.rs (2)

src/core/delaunay_triangulation.rs (2)

• new (102-104)
• vertices (439-441)

src/core/vertex.rs (2)

• point (475-477)
• from_points (419-424)

src/geometry/util.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• with_kernel (197-233)
• vertices (439-441)

src/core/vertex.rs (1)

src/geometry/point.rs (3)

• coords (100-102)
• new (115-117)
• get (127-129)

tests/proptest_vertex.rs (1)

src/core/vertex.rs (4)

• map (293-293)
• map (303-303)
• dim (575-577)
• empty (375-385)

tests/proptest_delaunay_condition.rs (4)

src/geometry/util.rs (1)

• core (1809-1809)

src/core/delaunay_triangulation.rs (6)

• vertices (439-441)
• new (102-104)
• tds (468-470)
• is_valid (769-774)
• number_of_vertices (322-324)
• number_of_cells (348-350)

src/core/triangulation.rs (3)

• vertices (75-77)
• number_of_vertices (81-83)
• number_of_cells (87-89)

src/core/vertex.rs (2)

• from_points (419-424)
• is_valid (611-629)

src/lib.rs (1)

src/core/delaunay_triangulation.rs (3)

• triangulation (496-498)
• new (102-104)
• vertices (439-441)

tests/proptest_serialization.rs (3)

src/core/delaunay_triangulation.rs (7)

• new (102-104)
• vertices (439-441)
• number_of_vertices (322-324)
• number_of_cells (348-350)
• dim (375-377)
• is_valid (769-774)
• cells (407-409)

src/core/triangulation_data_structure.rs (7)

• vertices (817-819)
• number_of_vertices (910-912)
• number_of_cells (1050-1052)
• dim (991-997)
• is_valid (3252-3266)
• cells (750-752)
• cells (3686-3695)

src/core/vertex.rs (6)

• dim (575-577)
• is_valid (611-629)
• map (293-293)
• map (303-303)
• point (475-477)
• from_points (419-424)

src/core/delaunay_triangulation.rs (6)

src/core/algorithms/incremental_insertion.rs (3)

• extend_hull (337-365)
• fill_cavity (90-140)
• wire_cavity_neighbors (157-276)

src/core/algorithms/locate.rs (1)

• locate (193-266)

src/core/cell.rs (7)

• cell (3224-3226)
• new (389-420)
• vertices (593-595)
• number_of_vertices (682-684)
• dim (870-872)
• is_valid (984-1024)
• neighbors (561-563)

src/core/util.rs (1)

• is_delaunay (1662-1694)

src/core/triangulation.rs (8)

• vertices (75-77)
• new_empty (58-63)
• number_of_vertices (81-83)
• number_of_cells (87-89)
• dim (93-95)
• cells (68-70)
• facets (125-127)
• boundary_facets (161-169)

src/core/triangulation_data_structure.rs (10)

• vertices (817-819)
• empty (2347-2356)
• vertex_keys (826-828)
• number_of_vertices (910-912)
• number_of_cells (1050-1052)
• dim (991-997)
• cells (750-752)
• cells (3686-3695)
• is_valid (3252-3266)
• validate_delaunay (3299-3333)

src/core/util.rs (4)

src/core/delaunay_triangulation.rs (4)

• triangulation (496-498)
• tds (468-470)
• new (102-104)
• vertices (439-441)

src/core/facet.rs (9)

• tds (411-413)
• cell (561-565)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• vertices (496-518)
• cell_key (289-291)
• cell_key (397-399)

src/core/triangulation_data_structure.rs (1)

• vertices (817-819)

tests/allocation_api.rs (2)

• measure_with_result (36-46)
• measure_with_result (50-56)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Codacy Security Scan
• GitHub Check: build (windows-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (macos-latest)

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/lib.rs (1)

247-272: Degenerate input example contradicts the surrounding error description.

The “Degenerate input handling” section states that construction fails with TriangulationConstructionError::GeometricDegeneracy, but the example below asserts on FailedToAddVertex. For the collinear 2D case shown, the first simplex construction is likely what fails (i.e., GeometricDegeneracy), not a later incremental insertion.

Recommend:

• Decide which variant is actually produced in this case, and
• Make both the prose and the assert!(matches!(...)) agree on that variant.

This is a doc‑only inconsistency but can easily confuse users debugging failures.

src/geometry/util.rs (1)

2019-2024: Doc for generate_random_triangulation error types is mostly correct but can be tightened.

The documentation now notes TriangulationConstructionError as a possible error, which matches the new return type. However, it still implicitly refers to “Bowyer-Watson algorithm” and talks about TDS semantics in some surrounding comments, while the actual construction path is DelaunayTriangulation::with_kernel using the incremental cavity-based algorithm.

It would be clearer to:

• Explicitly mention incremental insertion/DelaunayTriangulation here, and
• Remove or reword any remaining Bowyer–Watson/TDS references to avoid confusion.

Functionally fine; this is purely doc hygiene.

♻️ Duplicate comments (1)

src/core/delaunay_triangulation.rs (1)

1022-1089: Specialized Deserialize impl is now well‑documented and matches the serialization strategy

The custom Serialize impl only writes the Tds, and the specialized Deserialize for DelaunayTriangulation<FastKernel<f64>, (), (), D> now has a thorough doc comment explaining:

• That this is a convenience for the common FastKernel<f64> + no‑data case, and
• How to deserialize Tds directly and reconstruct with from_tds for custom kernels.

The code itself is minimal (Tds::deserialize then Self::from_tds(tds, FastKernel::new())) and aligns with that story. This addresses the earlier gap without adding complexity.

🧹 Nitpick comments (10)

tests/proptest_duplicates.rs (1)

4-12: Stale docstring references removed APIs.

The module docstring references Tds::bowyer_watson_with_diagnostics and "unsalvageable vertices reported by the diagnostics" which were removed in this PR. The test now validates via DelaunayTriangulation::new() and is_delaunay().

Consider updating to reflect the current behavior:

-//! These tests exercise the unified Bowyer–Watson pipeline used by
-//! `Tds::bowyer_watson_with_diagnostics` on randomized inputs that include
-//! exact duplicates and small jittered near-duplicates. The goal is to ensure
-//! that:
+//! These tests exercise the `DelaunayTriangulation` construction pipeline on
+//! randomized inputs that include exact duplicates and small jittered
+//! near-duplicates. The goal is to ensure that:
 //!
 //! - The final triangulation remains structurally valid and globally Delaunay
 //!   for the kept subset of vertices.
-//! - All unsalvageable vertices reported by the diagnostics come from the
-//!   original input set.

benches/large_scale_performance.rs (1)

135-139: Minor: Comment is slightly stale.

The comment mentions "Tds construction" but now measures DelaunayTriangulation::new() which includes the wrapper. The overhead is negligible since DelaunayTriangulation is a thin wrapper, but consider updating the comment for accuracy:

-    // Measure memory before Tds construction to isolate TDS allocation
+    // Measure memory before triangulation construction to isolate allocation

tests/proptest_serialization.rs (1)

216-218: Redundant imports.

These imports are unnecessary since delaunay::prelude::* is already imported at line 12, which typically re-exports Vertex, Point, and Coordinate.

 fn debug_neighbor_preservation_2d_regression() {
-    use delaunay::core::vertex::Vertex;
-    use delaunay::geometry::point::Point;
-    use delaunay::geometry::traits::coordinate::Coordinate;
-
     // Regression case derived from an earlier proptest failure (inlined for stability)

src/core/algorithms/incremental_insertion.rs (5)

90-140: fill_cavity implementation is structurally sound; consider minor robustness tweaks.

The loop correctly:

• collects the facet vertices by omitting facet_idx,
• appends new_vertex_key as apex,
• constructs and inserts one new cell per boundary facet, and
• accumulates keys into CellKeyBuffer.

Error wrapping into InsertionError::CavityFilling is also appropriate.

Two small robustness suggestions:

• Add a debug_assert_eq!(boundary_cell.number_of_vertices(), D + 1) (or similar) before building new_cell_vertices to catch unexpected cell dimensionality early.
• Similarly, a debug_assert_eq!(boundary_facets.len(), new_cells.len()) here or in wire_cavity_neighbors would make the 1:1 correspondence assumption explicit during development.

---

157-276: Neighbor wiring logic is consistent but relies on subtle invariants; make them explicit.

The internal neighbor wiring via a hash of sorted vertex keys is correct for simplicial cells and avoids global recomputation. External wiring also looks logically sound: for each cavity boundary facet you:

• reuse the original facet’s vertex set as the base of the new cell (via fill_cavity),
• wire the new cell’s base facet neighbor to the previous neighbor across that facet (external_neighbor), and
• update that external cell’s neighbor reference from the old boundary cell to the new cell.

A few improvements would help future maintainability:

• Explicitly document (or assert) that boundary_facets are facets on the conflict cells whose neighbors (external_neighbor) are the surviving outside cells. That assumption is critical for the external rewiring to be correct.
• Add a debug_assert_eq!(new_cells.len(), boundary_facets.len()) at the top; the if i >= new_cells.len() { break; } is defensive but can hide upstream mistakes.
• Consider guarding the external back-link update with a debug assertion that we actually found a neighbor slot in the external cell that pointed to facet_handle.cell_key(); silently skipping if none is found can also mask structural inconsistencies.

These are non-functional refactors but will make this fairly intricate logic safer to evolve.

---

337-365: extend_hull composes helpers correctly; clarify behavior when no facets are visible.

The hull-extension path correctly:

• finds visible boundary facets with find_visible_boundary_facets,
• returns a descriptive CavityFilling error if none are visible,
• then reuses fill_cavity and wire_cavity_neighbors to integrate the new vertex.

Given this is a separate path from the conflict-region BFS, it would be useful to:

• Document that extend_hull is currently intended for strictly exterior vertices (and what “exterior” means in terms of Kernel::orientation), and
• Consider whether a more specific error variant than CavityFilling for “no visible boundary facets” would help callers distinguish geometry issues from actual cavity-fill internal errors.

Functionally this looks fine; the concerns are mostly about clarity of semantics for callers.

---

383-458: Orientation-based visibility test is reasonable; consider tie-handling and reuse.

find_visible_boundary_facets builds boundary facets via tds.boundary_facets(), pulls their vertex coordinates, and classifies a facet as visible when the orientation sign flips upon replacing the opposite vertex with the query point. That’s a standard and sensible criterion.

Two small notes:

• Facets where either orientation is zero (degenerate or exactly coplanar) are treated as non‑visible. If you need “weakly visible” behavior for nearly coplanar hull facets, you may want an epsilon-based or >= 0 style test instead; right now such cases will be ignored and can cause extend_hull to report “No visible boundary facets”.
• Error mapping here into InsertionError::CavityFilling is slightly misleading (these are really visibility/boundary-analysis failures). If you keep that mapping for API simplicity, it’s worth documenting this choice so callers don’t misinterpret the source of failures.

Otherwise the implementation is straightforward and matches the documented geometric intent.

---

466-494: test_fill_cavity_2d is a good smoke test; consider extending to neighbor wiring.

The test nicely verifies that a simple 2D cavity over a single triangle produces three new cells, which sanity‑checks fill_cavity. To better guard regressions in the full insertion pipeline, you may want an additional test that:

• runs wire_cavity_neighbors on the produced new_cells and
• asserts neighbor consistency (e.g., using tds.is_valid() or a dedicated neighbor validation helper).

This would exercise both creation and wiring for a minimal cavity before integration into higher‑level insertion tests.

src/geometry/util.rs (2)

2095-2155: generate_random_triangulation implementation correctly targets DelaunayTriangulation but overloads GeometricDegeneracy.

The new implementation:

• Generates points via generate_random_points[_seeded],
• Converts them into Vertex<T, U, D> using VertexBuilder and the optional vertex_data,
• Builds a DelaunayTriangulation<FastKernel<T>, U, V, D> via with_kernel.

This is the right architectural move and uses the same kernel and error type as the rest of the construction pipeline.

Two semantic nits:

• All RandomPointGenerationError variants are wrapped into TriangulationConstructionError::GeometricDegeneracy. That’s convenient but conflates invalid parameter errors (e.g., bad bounds) with true geometric degeneracy. If you keep this mapping, consider documenting it explicitly in the function’s docs.
• For n_points < D + 1, the error will come from with_kernel as InsufficientVertices; the doc currently only mentions degeneracy generically. Adding a brief note about insufficient-vertex errors would make behavior more transparent.

Overall, the control flow and type bounds look correct.

---

4797-4912: Random triangulation tests moved to DelaunayTriangulation and appropriately marked #[ignore] for flakiness.

The random‑triangulation tests now:

• Use generate_random_triangulation returning DelaunayTriangulation<FastKernel<T>, U, V, D>,
• Validate via is_valid(), number_of_vertices, dim, etc., and
• Mark certain tests as #[ignore] with clear reasons (unseeded randomness + occasional cavity filling failures, zero‑point behavior mismatch, and high‑dimensional robustness needing investigation).

This is a pragmatic way to keep the tests available for local debugging while avoiding CI flakes until the incremental insertion path is fully hardened for those edge cases.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35ea9bb and 2301375.

📒 Files selected for processing (19)

• benches/large_scale_performance.rs (8 hunks)
• benches/microbenchmarks.rs (8 hunks)
• cspell.json (3 hunks)
• examples/memory_analysis.rs (3 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/collections.rs (5 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/facet.rs (37 hunks)
• src/core/traits/boundary_analysis.rs (6 hunks)
• src/core/triangulation.rs (1 hunks)
• src/core/vertex.rs (37 hunks)
• src/geometry/util.rs (21 hunks)
• src/lib.rs (17 hunks)
• tests/delaunay_edge_cases.rs (1 hunks)
• tests/proptest_delaunay_condition.rs (7 hunks)
• tests/proptest_duplicates.proptest-regressions (1 hunks)
• tests/proptest_duplicates.rs (3 hunks)
• tests/proptest_serialization.proptest-regressions (1 hunks)
• tests/proptest_serialization.rs (7 hunks)

✅ Files skipped from review due to trivial changes (2)

• tests/proptest_serialization.proptest-regressions
• tests/proptest_duplicates.proptest-regressions

🚧 Files skipped from review as they are similar to previous changes (3)

• src/core/collections.rs
• examples/memory_analysis.rs
• cspell.json

🧰 Additional context used

🧠 Learnings (7)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/traits/boundary_analysis.rs
• tests/delaunay_edge_cases.rs
• tests/proptest_serialization.rs
• benches/large_scale_performance.rs
• src/core/algorithms/incremental_insertion.rs
• benches/microbenchmarks.rs
• tests/proptest_duplicates.rs
• src/core/triangulation.rs
• src/geometry/util.rs
• src/core/vertex.rs
• tests/proptest_delaunay_condition.rs
• src/lib.rs
• src/core/facet.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/traits/boundary_analysis.rs
• tests/delaunay_edge_cases.rs
• tests/proptest_serialization.rs
• benches/large_scale_performance.rs
• src/core/algorithms/incremental_insertion.rs
• benches/microbenchmarks.rs
• tests/proptest_duplicates.rs
• src/core/triangulation.rs
• src/geometry/util.rs
• src/core/vertex.rs
• tests/proptest_delaunay_condition.rs
• src/lib.rs
• src/core/facet.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• src/core/traits/boundary_analysis.rs
• tests/delaunay_edge_cases.rs
• tests/proptest_serialization.rs
• benches/large_scale_performance.rs
• src/core/algorithms/incremental_insertion.rs
• benches/microbenchmarks.rs
• tests/proptest_duplicates.rs
• src/core/triangulation.rs
• src/geometry/util.rs
• src/core/vertex.rs
• tests/proptest_delaunay_condition.rs
• src/lib.rs
• src/core/facet.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait guarantees PartialOrd, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• src/core/vertex.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait includes PartialOrd through its Float supertrait bound, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• src/core/vertex.rs

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• src/lib.rs

🧬 Code graph analysis (11)

tests/delaunay_edge_cases.rs (5)

src/core/delaunay_triangulation.rs (5)

• vertices (438-440)
• new (102-104)
• number_of_vertices (321-323)
• number_of_cells (347-349)
• with_kernel (196-232)

src/core/facet.rs (5)

• vertices (496-518)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)

src/core/triangulation.rs (3)

• vertices (75-77)
• number_of_vertices (81-83)
• number_of_cells (87-89)

src/core/triangulation_data_structure.rs (3)

• vertices (817-819)
• number_of_vertices (910-912)
• number_of_cells (1050-1052)

src/core/cell.rs (3)

• vertices (593-595)
• new (389-420)
• number_of_vertices (682-684)

benches/large_scale_performance.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• vertices (438-440)
• tds (467-469)

src/core/algorithms/incremental_insertion.rs (2)

src/core/facet.rs (11)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)

src/core/boundary.rs (1)

• boundary_facets (73-81)

benches/microbenchmarks.rs (1)

src/core/delaunay_triangulation.rs (5)

• vertices (438-440)
• with_kernel (196-232)
• tds (467-469)
• is_valid (795-800)
• insert (617-706)

tests/proptest_duplicates.rs (3)

src/core/delaunay_triangulation.rs (4)

• vertices (438-440)
• new (102-104)
• tds (467-469)
• is_valid (795-800)

src/core/vertex.rs (2)

• from_points (419-424)
• is_valid (585-603)

src/core/util.rs (1)

• is_delaunay (1662-1694)

src/geometry/util.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• with_kernel (196-232)
• vertices (438-440)

src/core/vertex.rs (1)

src/geometry/point.rs (3)

• coords (100-102)
• new (115-117)
• get (127-129)

tests/proptest_delaunay_condition.rs (2)

src/core/util.rs (1)

• is_delaunay (1662-1694)

src/core/delaunay_triangulation.rs (4)

• vertices (438-440)
• new (102-104)
• tds (467-469)
• is_valid (795-800)

src/lib.rs (1)

src/core/delaunay_triangulation.rs (3)

• triangulation (509-511)
• new (102-104)
• vertices (438-440)

src/core/facet.rs (3)

src/core/delaunay_triangulation.rs (5)

• new (102-104)
• vertices (438-440)
• tds (467-469)
• with_kernel (196-232)
• cells (406-408)

src/core/triangulation.rs (2)

• vertices (75-77)
• cells (68-70)

src/core/triangulation_data_structure.rs (3)

• vertices (817-819)
• cells (750-752)
• cells (3675-3684)

src/core/delaunay_triangulation.rs (3)

src/core/algorithms/incremental_insertion.rs (3)

• extend_hull (337-365)
• fill_cavity (90-140)
• wire_cavity_neighbors (157-276)

src/core/algorithms/locate.rs (1)

• locate (193-266)

src/core/triangulation.rs (8)

• vertices (75-77)
• new_empty (58-63)
• number_of_vertices (81-83)
• number_of_cells (87-89)
• dim (93-95)
• cells (68-70)
• facets (125-127)
• boundary_facets (161-169)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: build (windows-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: Codacy Security Scan

🔇 Additional comments (47)

src/core/vertex.rs (5)

84-135: Vertex macro no‑data semantics look consistent with the new U = () convention

Pattern 1 of vertex! now hard‑codes VertexBuilder::<_, (), _> and all docs/examples use Vertex<_, (), D> for no‑data vertices. This matches the underlying Option<U> storage and the serialization behavior (data omitted when None), so the public “no‑data == ()” story is coherent.

---

143-196: Struct docs and type parameter comments correctly describe no‑data as U = ()

The Vertex<T, U, D> docs explicitly call out “use () for no data” while keeping data: Option<U> internally. That combination avoids breaking internal invariants while making the public API much clearer. No issues from a correctness or ergonomics perspective.

---

506-531: set_uuid test helper and its tests are well‑scoped

set_uuid is #[cfg(test)] + pub(crate), validates via validate_uuid, and reuses VertexValidationError::InvalidUuid through the existing From<UuidValidationError> path. The three tests (test_set_uuid_valid, test_set_uuid_nil_uuid, test_set_uuid_invalid_version) exercise success, nil, and “invalid version” cases and verify that the UUID is not mutated on error. This is a good pattern for targeted test helpers without leaking into the public API.

Also applies to: 1966-2026

---

1082-1184: Equality/hash semantics and tests are tightly aligned

PartialEq/Hash only consider coordinates, and the test_vertex_equality_and_hashing suite thoroughly checks:

• equality vs. inequality on coordinates,
• ignoring UUID and data,
• hash consistency and the Eq/Hash contract across multiple cases.

This is exactly what you want for using Vertex as a key keyed on geometry only; no issues spotted.

---

1504-1641: Validation tests for NaN/∞ and UUID edge cases give strong coverage

test_vertex_validation exercises:

• valid finite vertices across dimensions and scalar types,
• NaN and ±∞ in multiple positions/dimensions,
• nil UUID vs. invalid point precedence,
• combined invalid point + invalid UUID.

This matches the documented behavior of is_valid (point checked first, then UUID) and should catch most regressions in Coordinate/UUID validation. Implementation and tests look solid.

src/core/triangulation.rs (1)

26-47: Triangulation wrapper and facet accessors look clean and minimal

The Triangulation<K, U, V, D> struct holds kernel + tds and all the public methods (new_empty, cells, vertices, number_of_vertices, number_of_cells, dim, facets, boundary_facets) are thin, well‑typed delegations to Tds and the facet iterators. The examples using DelaunayTriangulation::new(...).triangulation().facets()/boundary_facets() are coherent with the new architecture.

Also applies to: 65-96, 97-169

src/core/delaunay_triangulation.rs (5)

196-298: Initial simplex construction via build_initial_simplex is clear and robust

with_kernel’s flow of:

• Guarding on vertices.len() < D + 1,
• Building a single D‑simplex via build_initial_simplex using insert_vertex_with_mapping + Cell::new(...) + assign_incident_cells(),
• Then incrementally inserting the remaining vertices,

is a good replacement for the old Bowyer–Watson initialization. Error mapping to TriangulationConstructionError is precise, including the extra “too many vertices” guard in build_initial_simplex. Looks correct and maintainable.

---

378-441: Accessors cleanly expose TDS and triangulation while keeping invariants explicit

The cells(), vertices(), tds(), tds_mut() (test‑only), and triangulation() accessors are straightforward:

• cells()/vertices() just delegate to tri/tds and match the new iterator types.
• tds() is read‑only and used widely in tests/benches, while tds_mut() is restricted to #[cfg(test)] + pub(crate), which is a good compromise for internal testing without exposing mutability in the public API.
• triangulation() provides the exact handle needed by higher‑level algorithms like convex hull.

No issues here.

Also applies to: 467-512

---

930-1000: validate_delaunay error mapping is precise and user‑friendly

Wrapping core::util::is_delaunay(&self.tri.tds) and translating DelaunayValidationError into TriangulationValidationError with UUIDs and keys in the messages is helpful for debugging. The fallback for InvalidCell and numeric predicate errors is also reasonable. No structural or correctness issues seen.

---

1091-1102: DelaunayCheckPolicy is fine as a forward‑looking hook

The DelaunayCheckPolicy enum is small, well‑documented, and gives you room to schedule global Delaunay validation. Even though it isn’t wired into insertion yet, it’s a reasonable public knob and doesn’t complicate the rest of the API.

---

1104-1780: Test coverage across dimensions, kernels, and construction paths is excellent

The generated tests (test_incremental_insertion!, test_build_initial_simplex!), plus the explicit suites for empty, with_kernel (Fast/Robust/f32), query methods, insert sequences, and TDS accessors, cover:

• D=2–5 initial simplices and interior insertions,
• Kernel variations (Fast/Robust, f64/f32),
• Error paths for insufficient vertices and over‑sized simplexes,
• Preservation of user vertex data in build_initial_simplex.

This breadth should make regressions in the new architecture unlikely. No issues in the test logic themselves.

src/core/facet.rs (4)

36-55: Facet docs and examples now align with the DelaunayTriangulation‑centric API

The module‑level and FacetHandle examples now use delaunay::prelude::*, construct a DelaunayTriangulation::new(&vertices), and obtain the TDS via dt.tds() before creating FacetViews. This matches the new public surface and keeps FacetView on the TDS level where it belongs. No behavioral changes, and the examples should compile under the updated prelude.

Also applies to: 213-231, 247-261

---

496-603: FacetView core methods remain lightweight and defensively implemented

FacetView::vertices, opposite_vertex, cell, and key:

• Validate cell presence and facet index,
• Surface missing vertices and TDS errors as precise FacetError variants,
• Use SmallBuffer to collect either references or vertex keys without heap churn,
• Compute facet keys via facet_key_from_vertices.

The use of SmallBuffer<&Vertex> in vertices() is safe since the buffer stores the references by value and the iterator owns them after into_iter(). No correctness or lifetime problems found.

---

712-865: Facet iterators integrate cleanly with the new TDS access patterns

AllFacetsIter::new snapshots cell_keys() and walks cell facets lazily, while BoundaryFacetsIter wraps it with a precomputed FacetToCellsMap to select hull facets. This matches how Triangulation::facets()/boundary_facets() and the boundary analysis trait use them. The dimension assertion for D <= 255 is a sensible safety guard given the u8 facet indices.

---

952-1747: Facet tests now exercise the DelaunayTriangulation/TDS integration comprehensively

All the test helpers and suites have been updated to:

• Build triangulations via DelaunayTriangulation::new(&vertices) (including typed data),
• Obtain cell_key from dt.cells(),
• Construct FacetView via FacetView::new(dt.tds(), cell_key, facet_idx).

They cover 1D–5D facets, key consistency, equality, error cases, boundary counts, and memory footprint. This strongly validates the FacetView API in the new architecture.

src/core/traits/boundary_analysis.rs (1)

20-41: BoundaryAnalysis trait examples correctly reflect the new Delaunay entry point

The trait’s doctests now:

• Use delaunay::prelude::*,
• Construct a DelaunayTriangulation::new(&vertices) and then call dt.tds() to get the Tds implementing BoundaryAnalysis,
• Show both direct number_of_boundary_facets() usage and the higher‑level dt.boundary_facets().count() pattern.

This keeps the trait semantics the same while making the documentation consistent with the rest of the refactor. No code changes or issues in the trait itself.

Also applies to: 65-81, 106-124, 153-219

tests/proptest_duplicates.rs (1)

100-126: LGTM!

The migration to DelaunayTriangulation API is correct. The dual validation (is_valid() + is_delaunay()) provides comprehensive coverage, and the prop_assume!(false); unreachable!(); pattern is idiomatic for skipping degenerate cases in proptest.

tests/delaunay_edge_cases.rs (3)

20-64: LGTM!

The test_regression_config! macro is well-designed with appropriate panic messages for debugging, and the optional ignore variant handles known problematic cases cleanly.

---

167-213: LGTM!

The minimum vertex tests correctly verify that exactly D+1 vertices produce a single cell across 2D, 3D, and 4D. These are important regression tests for simplex construction.

---

373-394: LGTM!

Good test for degenerate input handling. The test correctly verifies that collinear points in 2D fail with FailedToAddVertex error, which is the expected behavior since no valid triangulation exists.

benches/large_scale_performance.rs (1)

185-190: LGTM!

The construction benchmark correctly measures DelaunayTriangulation::new() and returns the result through black_box to prevent optimization. The migration from Tds is consistent with the new API.

tests/proptest_serialization.rs (1)

50-77: LGTM!

The JSON roundtrip test correctly serializes and deserializes DelaunayTriangulation objects, verifying that vertex count, cell count, and dimension are preserved. The use of type inference (_) for the kernel type works since FastKernel is the default.

tests/proptest_delaunay_condition.rs (2)

100-109: LGTM!

The migration to is_delaunay(dt.tds()) is appropriate. This function performs comprehensive Delaunay validation (structural invariants + empty circumsphere check for all cells), replacing the previous per-cell insphere checks with a unified validation path.

---

125-241: LGTM!

The insertion-order invariance test is appropriately ignored with a clear reference to issue #120. The test logic is sound: it acknowledges that Delaunay triangulations are not unique for degenerate/co-circular inputs, and the tolerance-based cell count comparison is a reasonable relaxation. The enhanced general position filter (co-circularity check) is thorough.

benches/microbenchmarks.rs (5)

21-87: Bench migration to DelaunayTriangulation::with_kernel looks consistent.

The dimensional benchmarks now:

• Generate seeded random points,
• Wrap them in vertex! and
• Measure DelaunayTriangulation::<RobustKernel<f64>, (), (), D>::with_kernel(...).unwrap().

This aligns with the new kernel‑centric API and keeps inputs deterministic via get_benchmark_seed. Using BatchSize::LargeInput is appropriate given per‑iteration reconstruction cost. No functional issues spotted.

---

95-123: Memory usage benchmarks correctly switched to dt.tds() counts.

benchmark_memory_usage_*d now constructs a DelaunayTriangulation and derives vertex/cell counts via dt.tds(). That’s the right way to keep the bench focused on the public triangulation type while still probing the underlying TDS. The black_box((..., ...)) tuple is a good way to prevent DCE.

---

135-203: Validation benches correctly exercise is_valid, mapping, and cell checks.

The validation benchmarks now:

• Build a DelaunayTriangulation with with_kernel,
• Time dt.is_valid().unwrap() directly, and
• Benchmark validate_vertex_mappings / validate_cell_mappings via the same dt.

This matches the validation API defined on DelaunayTriangulation and ensures you’re measuring the intended structural checks. The reuse of a single dt instance for component benchmarks avoids construction overhead skewing results.

---

213-283: Incremental construction benches correctly switched from tds.add to dt.insert.

The incremental benchmarks now:

• Build an initial simplex via DelaunayTriangulation::with_kernel(...),
• Benchmark dt.insert(additional_vertex) for single insert, and
• Benchmark multiple dt.insert(vertex) calls for small batches.

This aligns with the new incremental insertion API and still uses deterministic point sets via the shared seed. The use of BatchSize::SmallInput and black_box(dt) looks appropriate.

---

339-370: Benchmark group targets are coherent with the new naming scheme.

The criterion_group! now exposes benchmark_delaunay_triangulation_Xd, memory, validation, and incremental construction targets for 2–5D. The naming and grouping reflect the DelaunayTriangulation‑centric architecture, and there are no dangling references to removed Bowyer–Watson/TDS benches.

src/lib.rs (8)

19-38: 4D basic-usage example aligns with the new API.

The top-level example now:

• Imports via delaunay::prelude::*,
• Builds a 4D DelaunayTriangulation<_, (), (), 4> using new,
• Checks number_of_vertices, dim, and number_of_cells.

This matches the public constructor and methods on DelaunayTriangulation and illustrates the incremental insertion behavior clearly.

---

47-95: Convex hull example is consistent with ConvexHull::from_triangulation and DelaunayTriangulation.

The convex hull docs now:

• Construct 3D and 4D triangulations via DelaunayTriangulation::new,
• Pass dt.triangulation() into ConvexHull::from_triangulation,
• Use is_point_outside and find_visible_facets with the same triangulation view.

This is coherent with the new Triangulation layer and should help users adopt the new API shape without touching the underlying TDS.

---

99-140: Invariants and validation docs correctly point to DelaunayTriangulation helpers.

The “Triangulation Invariants” and “Validation helpers” sections now refer directly to:

• DelaunayTriangulation::is_valid,
• validate_vertex_mappings, validate_cell_mappings,
• validate_no_duplicate_cells, validate_facet_sharing,
• validate_neighbors, and validate_delaunay.

This accurately reflects the validation surface and clarifies that is_valid covers structural checks, while validation_report plus ValidationOptions::check_delaunay provide deeper diagnostics.

---

293-325: “Simple API Usage” examples correctly showcase DelaunayTriangulation::new in 4D and 2D.

These examples now consistently:

• Import via the prelude,
• Create triangulations with DelaunayTriangulation::new,
• Assert number_of_vertices, dim, and number_of_cells.

They align with the new constructor signature (Vertex<f64, (), D>) and illustrate both 4D and 2D minimal setups.

---

345-377: Core/geometry module exports reflect the new incremental-insertion architecture.

The core module now:

• Exposes algorithms::{flips, incremental_insertion, locate},
• Adds delaunay_triangulation and triangulation modules, and
• Re-exports delaunay_triangulation::* alongside existing TDS components.

The geometry module introduces a public kernel module and keeps traits::coordinate as the canonical coordinate trait. This reorganized surface matches the PR goals (DelaunayTriangulation/Kernel centric) while preserving lower-level access via triangulation_data_structure.

---

445-474: Prelude exports now match the intended public surface.

The prelude:

• Re-exports delaunay_triangulation::* and core traits/data types,
• Re-exports geometry algorithms, kernel::*, predicates, quality, and utilities,
• Exposes the vertex macro.

This gives downstream code a coherent “just import prelude” story for both triangulation and geometry, without pulling in the internal collections module by default (which is still available via explicit imports).

---

488-507: normal_types test still correctly covers Point, Vertex, Cell, and Tds.

Even though the public surface is now DelaunayTriangulation-centric, this test keeps verifying that core types (Point, Vertex, Cell, Tds) satisfy the expected auto traits (Send + Sync + Unpin). Keeping Tds in this check is useful to guard against accidental changes in the underlying storage type.

---

539-561: Prelude quality-exports test properly exercises quality functions with DelaunayTriangulation.

The test now:

• Constructs a 2D DelaunayTriangulation via the prelude,
• Grabs a cell key from dt.cells(),
• Calls radius_ratio and normalized_volume with dt.triangulation().

This is a good end-to-end check that both prelude exports and the triangulation/quality APIs are wired up correctly.

src/geometry/util.rs (10)

13-19: Imports updated correctly for the DelaunayTriangulation-based helpers.

Bringing in:

• core::delaunay_triangulation::DelaunayTriangulation,
• core::triangulation_data_structure::TriangulationConstructionError, and
• geometry::kernel::FastKernel

is exactly what generate_random_triangulation needs after the API shift away from direct Tds::new. No issues here.

---

1557-1569: surface_measure example now uses DelaunayTriangulation correctly.

The example:

• Uses delaunay::prelude::*,
• Builds a 3D DelaunayTriangulation with DelaunayTriangulation::new,
• Obtains boundary facets via dt.tds().boundary_facets()?, and
• Calls surface_measure(&boundary_facets).

This is consistent with the new public API and shows how to combine the high-level triangulation with low-level FacetView utilities.

---

3374-3381: Empty-facets surface_measure test still valid after type-parameterization.

The test now explicitly uses FacetView<'_, f64, (), (), 3> and validates that passing an empty slice yields total measure 0.0. This continues to check the neutral-element behavior of surface_measure and matches the updated FacetView and TDS generics.

---

3389-3402: test_surface_measure_single_facet’s migration to DelaunayTriangulation looks correct.

The test now:

• Constructs vertices as Vertex<f64, (), 3>,
• Builds DelaunayTriangulation<_, (), (), 3>::new(&vertices),
• Uses dt.tds().boundary_facets()? to extract boundary facets, and
• Filters for the facet opposite a given vertex before calling surface_measure.

The logic still isolates the intended right-triangle facet and asserts area 6.0. Migration away from raw Tds is done cleanly.

---

3435-3446: test_surface_measure_consistency_with_facet_measure correctly uses dt.tds().boundary_facets().

Switching to:

• Constructing a 3D DelaunayTriangulation, then
• Collecting boundary facets via dt.tds().boundary_facets(),

preserves the intent of comparing surface_measure to a manual sum of facet_measures. The updated construction path and facet acquisition are consistent with the rest of the file’s utilities.

---

3773-3841: Multi-triangulation surface-measure test correctly leverages DelaunayTriangulation.

The “multiple facets different sizes” test now builds two separate 3D triangulations via DelaunayTriangulation::new, gets their boundary facets from dt.tds(), then extracts the desired triangle facets by coordinates.

The final surface_measure call over both facets yielding 0.5 + 24.0 remains a good sanity check for additive behavior after the API change.

---

3847-3867: 2D perimeter test is correctly migrated to use DelaunayTriangulation.

test_surface_measure_2d_perimeter now:

• Builds a 2D triangulation from three points forming a 3‑4‑5 triangle,
• Pulls boundary edges from dt.tds().boundary_facets(), and
• Verifies that surface_measure returns perimeter 12.0.

That’s exactly the intended behavior for 1D facets in 2D space and validates the TDS/BoundaryAnalysis integration for 2D.

---

3873-3895: 4D boundary surface test has the corrected expected total (1.0).

The change of the expected total surface in test_surface_measure_4d_boundary from 5.0 / 6.0 to 1.0 matches the analytical breakdown given in the comment (four facets of volume 1/6 plus one of 1/3). Combined with the switch to building the triangulation via DelaunayTriangulation::new and using dt.tds().boundary_facets(), this makes the test both API- and mathematically-consistent.

---

3967-3991: High-facet-count surface-measure test now uses DelaunayTriangulation correctly.

test_surface_measure_many_facets constructs a tetrahedron via DelaunayTriangulation, collects all four boundary facets from dt.tds().boundary_facets(), and checks surface_measure for positivity/finite-ness. This keeps the stress test aligned with the new triangulation API.

---

2068-2071: Doc example for accessing the underlying TDS from a triangulation is now accurate.

The example now correctly unwraps a DelaunayTriangulation, calls .tds(), and uses number_of_vertices() on the TDS. This is a clear illustration of how to drop down to low‑level operations from the high‑level triangulation, and matches the current public API.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/core/delaunay_triangulation.rs (1)

1111-1125: Serde (de)serialization design for DelaunayTriangulation is appropriate and well‑documented

• The Serialize impl delegating directly to self.tri.tds.serialize(serializer) keeps the on‑wire format focused on the TDS, avoiding kernel coupling.
• The specialized Deserialize for DelaunayTriangulation<FastKernel<f64>, (), (), D> reconstructs the kernel via FastKernel::new() and is clearly documented as a convenience for the common case.
• The doc comment now explicitly explains why the specialization is limited to FastKernel<f64> and how to deserialize custom kernels (Tds → from_tds), which addresses the prior review concern.

This setup gives a clean default path while keeping more advanced kernel configurations explicit and under user control.

Also applies to: 1167-1177

🧹 Nitpick comments (11)

tests/proptest_convex_hull.rs (2)

83-86: Consistent with previous comment about full path usage.

Same pattern as line 42 - could use prelude import for brevity.

---

42-42: Use the prelude import instead of the full path.

Since delaunay::prelude::* is already imported at line 13, DelaunayTriangulation is available directly and should be used instead of delaunay::core::delaunay_triangulation::DelaunayTriangulation. This makes the code more concise and consistent with the library's documented usage patterns.

-                    if let Ok(dt) = delaunay::core::delaunay_triangulation::DelaunayTriangulation::<_, (), (), $dim>::new(&vertices) {
+                    if let Ok(dt) = DelaunayTriangulation::<_, (), (), $dim>::new(&vertices) {

benches/large_scale_performance.rs (1)

138-139: Unused _tds binding may be dead code.

The let _tds = dt.tds(); call doesn't contribute to the memory measurement since it just returns a reference to the already-constructed TDS. If this is intentional (e.g., to ensure the accessor compiles), consider adding a comment. Otherwise, it can be removed.

     let dt = DelaunayTriangulation::new(&vertices).expect("Failed to create triangulation");
-    let _tds = dt.tds();

src/core/triangulation.rs (1)

37-47: Avoid over‑constraining simple Triangulation accessors with heavy scalar bounds

This impl block puts K::Scalar: AddAssign + SubAssign + Sum + NumCast on all methods, even though only geometry/quality routines (like fix_invalid_facet_sharing via radius_ratio) actually need them. Consider splitting into:

• A lightweight impl<K, U, V, D> Triangulation<...> with only K: Kernel<D>, U: DataType, V: DataType for new_empty, cells, vertices, counts, dim, facets, boundary_facets.
• A second impl (or method‑level where) with the stronger numeric bounds just for repair/quality helpers.

That keeps the topological API usable with kernels whose scalar doesn’t implement the full numeric stack, and makes bounds easier to reason about.

Also applies to: 49-55

src/core/algorithms/incremental_insertion.rs (2)

95-170: Document or guard fill_cavity’s partial‑mutation behavior on errors

fill_cavity pushes new cells into tds as it iterates boundary facets, and returns early on:

• missing boundary cells,
• wrong vertex count, or
• cell creation/insertion failure.

In those cases, some new cells may have been inserted already, so Err(CavityFilling { .. }) does not imply the TDS is unchanged. If callers interpret errors as “no mutation”, that can be surprising.

You could either:

• Pre‑validate all boundary_facets up front (cells exist and have D+1 vertices) before creating any new cells, or
• Explicitly document that this function leaves the TDS in a partially updated state on failure and is intended to be used in contexts where the whole triangulation is discarded on error.

---

382-411: Hull‑extension helpers are correct; scalar bounds could be slightly relaxed

extend_hull + find_visible_boundary_facets:

• Correctly derive visible facets from tds.boundary_facets() via orientation sign flips (strict visibility, coplanar treated as non‑visible as documented).
• Cleanly map all failures (boundary facet fetch, orientation, cell/vertex lookup) into InsertionError::HullExtension.
• Reuse fill_cavity + wire_cavity_neighbors so hull extension shares the same cavity/wiring semantics as interior insertion.

The where clauses currently require K::Scalar: AddAssign + SubAssign + Sum, but these traits aren’t used directly in these two functions (only the kernel’s orientation and point copying are). Unless the Kernel trait itself needs those bounds, you can probably drop them from these signatures to avoid over‑constraining kernels that might not implement the full numeric stack.

Also applies to: 439-520

src/geometry/util.rs (1)

1994-2041: generate_random_triangulation correctly wraps DT construction but has a couple of polish opportunities

The new implementation:

• Generates random Point<T, D>s (seeded or unseeded) and maps any RandomPointGenerationError into TriangulationConstructionError::GeometricDegeneracy with a descriptive message.
• Builds Vertex<T, U, D>s via VertexBuilder, reusing vertex_data when provided.
• Constructs a DelaunayTriangulation<FastKernel<T>, U, V, D> using with_kernel, so downstream errors (insufficient vertices, geometric degeneracy, validation failures, etc.) surface via the existing TriangulationConstructionError variants.

Behaviorally this matches the updated docs.

Two small cleanups you might consider:

• The where clause still includes for<'a> &'a T: Div<T>, but this function no longer performs any direct scalar division; that bound can likely be dropped unless required indirectly by FastKernel<T> or with_kernel (in which case it’s better expressed on those APIs).
• The ignored test test_generate_random_triangulation_error_cases still expects the zero‑points case to produce an “empty triangulation”, while the current implementation will hit InsufficientVertices. Either adjust the test to assert the error variant or update the function semantics if you truly want to support an empty DT here, and then un‑ignore it.

Also applies to: 2111-2172

src/core/delaunay_triangulation.rs (4)

597-769: Incremental cavity‑based insertion path is sound; consider tightening error semantics for degenerate locations

The main incremental path (LocateResult::InsideCell / Outside) is well‑structured:

• You compute conflict cells, extract the cavity boundary, fill the cavity, wire neighbors, then remove old cells, and
• Run fix_invalid_facet_sharing() as a best‑effort geometric cleanup, with debug logging only on failure.
• For LocateResult::Outside, you delegate to extend_hull and update last_inserted_cell, which matches the documented hull extension support.

Two small follow‑ups to consider:

1. Degenerate cases are all folded into InsertionError::CavityFilling
   The fallback arm for OnFacet / OnEdge / OnVertex currently returns a generic InsertionError::CavityFilling with an informative message. Since the PR introduces explicit InsertionError variants for duplicate coordinates/UUIDs, you may eventually want to:

   • Map OnVertex to a more specific “duplicate” variant when the new point coincides with an existing vertex, and
   • Reserve CavityFilling for genuine cavity failures rather than location degeneracy.

2. Optional: hook in DelaunayCheckPolicy here
   The DelaunayCheckPolicy enum below is designed to schedule global validation, but it isn’t yet wired into insert. Once you’re ready, threading a policy argument or configuration into this method (or into the surrounding API) would let you periodically call validate_delaunay() / is_valid() without changing the core insertion logic.

No functional changes are required here, but clarifying the error taxonomy and planning where DelaunayCheckPolicy is consumed will keep the insertion API easier to interpret.

---

1055-1088: validate_delaunay error mapping is helpful; minor improvement opportunity for diagnostics

The mapping from DelaunayValidationError to TriangulationValidationError is generally good:

• You surface the offending cell_key and look up its UUID for DelaunayViolation.
• You preserve the underlying TriangulationValidationError when the triangulation state is bad.
• You wrap invalid‑cell and numeric‑predicate errors into higher‑level validation failures.

Two small diagnostic tweaks to consider:

• In the NumericPredicateError branch, you currently only mention cell_key and vertex_key in the message. If cell_uuid_from_key is cheap and reliable, including the cell UUID here (similar to the Delaunay violation case) would make downstream debugging and log correlation easier.
• For InvalidCell, you always use Uuid::nil() as cell_id. If you have a way to identify the problematic cell (even approximately), threading that UUID through would tighten the report.

These are purely observability improvements; the functional behavior of validate_delaunay looks correct.

---

1180-1191: DelaunayCheckPolicy is a useful hook; consider defining where it will be plumbed in

DelaunayCheckPolicy (EndOnly / EveryN) is a good abstraction for controlling global validation frequency, but right now it’s only defined here and not yet consumed in insert or elsewhere in this file.

To avoid it becoming dead API, consider:

• Either wiring it into insertion (e.g., via a configuration field on DelaunayTriangulation or an argument to higher‑level construction routines), or
• Marking it as internal/experimental until the scheduling story is finalized.

Not urgent, but clarifying its intended usage point will help future contributors understand how it’s meant to influence validation.

---

1193-1980: Tests give strong coverage; add one to lock in VertexKey stability across bootstrap

The test suite here is excellent:

• test_incremental_insertion! and test_build_initial_simplex! macros exercise both batch and bootstrap paths across 2D–5D, for Fast and Robust kernels.
• Dedicated tests cover empty(), with_empty_kernel(), with_kernel() (including f32), insufficient vertices, user data in build_initial_simplex, and insert behavior (cell counts, last_inserted_cell, etc.).
• Accessor tests verify that tds() reflects insertions and maintains invariants.

Given the bootstrap implementation currently replaces the entire TDS when reaching D+1 vertices, it would be valuable to add a focused test that checks handle stability, e.g.:

• Start from empty(),
• Insert D+1 vertices, storing each returned VertexKey,
• After bootstrap, assert that:
  • dt.tds().get_vertex_by_key(key) (or equivalent) still succeeds for each key, and
  • Each key still refers to the correct coordinates/user‑data.

Such a test would both guard against regressions in the TDS bootstrap logic (comment above) and make the intended semantics of VertexKey across the bootstrap transition crystal clear.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2301375 and f4c5d1f.

📒 Files selected for processing (27)

• .github/workflows/ci.yml (1 hunks)
• benches/large_scale_performance.rs (8 hunks)
• docs/code_organization.md (0 hunks)
• justfile (3 hunks)
• scripts/changelog_utils.py (2 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)
• src/geometry/util.rs (22 hunks)
• src/lib.rs (17 hunks)
• tests/allocation_api.rs (4 hunks)
• tests/circumsphere_debug_tools.rs (4 hunks)
• tests/coordinate_conversion_errors.rs (1 hunks)
• tests/delaunay_incremental_insertion.rs (1 hunks)
• tests/proptest_cell.rs (6 hunks)
• tests/proptest_convex_hull.rs (9 hunks)
• tests/proptest_delaunay_condition.rs (0 hunks)
• tests/proptest_delaunay_triangulation.rs (1 hunks)
• tests/proptest_duplicates.rs (0 hunks)
• tests/proptest_facet.rs (6 hunks)
• tests/proptest_geometry.rs (1 hunks)
• tests/proptest_invariants.rs (0 hunks)
• tests/proptest_point.rs (1 hunks)
• tests/proptest_predicates.rs (1 hunks)
• tests/proptest_quality.rs (0 hunks)
• tests/proptest_safe_conversions.rs (1 hunks)
• tests/proptest_serialization.proptest-regressions (1 hunks)

💤 Files with no reviewable changes (5)

• tests/proptest_duplicates.rs
• docs/code_organization.md
• tests/proptest_invariants.rs
• tests/proptest_delaunay_condition.rs
• tests/proptest_quality.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/proptest_serialization.proptest-regressions
• tests/circumsphere_debug_tools.rs

🧰 Additional context used

📓 Path-based instructions (1)

scripts/*.py

⚙️ CodeRabbit configuration file

scripts/*.py: These are Python utility scripts for changelog and commit processing.
Focus on code quality, maintainability, and adherence to Python best practices.
The scripts use ruff for comprehensive linting and formatting (pylint has been retired).

Files:

• scripts/changelog_utils.py

🧠 Learnings (8)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• tests/proptest_geometry.rs
• tests/proptest_point.rs
• tests/proptest_facet.rs
• tests/proptest_predicates.rs
• tests/proptest_safe_conversions.rs
• benches/large_scale_performance.rs
• tests/delaunay_incremental_insertion.rs
• tests/coordinate_conversion_errors.rs
• tests/allocation_api.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• tests/proptest_delaunay_triangulation.rs
• tests/proptest_convex_hull.rs
• src/geometry/util.rs
• src/lib.rs
• src/core/delaunay_triangulation.rs
• tests/proptest_cell.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• tests/proptest_geometry.rs
• tests/proptest_point.rs
• tests/proptest_facet.rs
• tests/proptest_predicates.rs
• tests/proptest_safe_conversions.rs
• benches/large_scale_performance.rs
• tests/delaunay_incremental_insertion.rs
• tests/coordinate_conversion_errors.rs
• tests/allocation_api.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• tests/proptest_delaunay_triangulation.rs
• tests/proptest_convex_hull.rs
• src/geometry/util.rs
• src/lib.rs
• src/core/delaunay_triangulation.rs
• tests/proptest_cell.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• tests/proptest_geometry.rs
• tests/proptest_point.rs
• tests/proptest_facet.rs
• tests/proptest_predicates.rs
• tests/proptest_safe_conversions.rs
• benches/large_scale_performance.rs
• tests/delaunay_incremental_insertion.rs
• tests/coordinate_conversion_errors.rs
• tests/allocation_api.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• tests/proptest_delaunay_triangulation.rs
• tests/proptest_convex_hull.rs
• src/geometry/util.rs
• src/lib.rs
• src/core/delaunay_triangulation.rs
• tests/proptest_cell.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait guarantees PartialOrd, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• tests/proptest_point.rs
• tests/proptest_safe_conversions.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait includes PartialOrd through its Float supertrait bound, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• tests/proptest_point.rs
• tests/proptest_safe_conversions.rs

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• justfile
• .github/workflows/ci.yml

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• .github/workflows/ci.yml
• src/lib.rs

🧬 Code graph analysis (9)

tests/proptest_facet.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• vertices (462-464)
• tds (491-493)

benches/large_scale_performance.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• vertices (462-464)
• tds (491-493)

tests/delaunay_incremental_insertion.rs (3)

src/core/delaunay_triangulation.rs (5)

• vertices (462-464)
• number_of_cells (371-373)
• insert (682-795)
• number_of_vertices (345-347)
• with_kernel (220-256)

src/core/triangulation.rs (3)

• vertices (75-77)
• number_of_cells (87-89)
• number_of_vertices (81-83)

src/core/triangulation_data_structure.rs (3)

• vertices (817-819)
• number_of_cells (1050-1052)
• number_of_vertices (910-912)

tests/allocation_api.rs (3)

src/core/util.rs (2)

• measure_with_result (1473-1482)
• measure_with_result (1488-1493)

src/core/delaunay_triangulation.rs (4)

• empty (132-134)
• vertices (462-464)
• new (102-104)
• number_of_vertices (345-347)

src/core/triangulation.rs (2)

• vertices (75-77)
• number_of_vertices (81-83)

src/core/triangulation.rs (4)

src/core/vertex.rs (2)

• uuid (502-504)
• dim (549-551)

src/geometry/quality.rs (1)

• radius_ratio (223-276)

src/core/delaunay_triangulation.rs (6)

• cells (430-432)
• vertices (462-464)
• dim (398-400)
• facets (563-565)
• new (102-104)
• boundary_facets (593-595)

src/core/boundary.rs (1)

• boundary_facets (73-81)

tests/proptest_convex_hull.rs (5)

src/core/delaunay_triangulation.rs (5)

• new (102-104)
• vertices (462-464)
• tds (491-493)
• triangulation (533-535)
• insert (682-795)

src/core/triangulation.rs (1)

• vertices (75-77)

src/geometry/algorithms/convex_hull.rs (1)

• from_triangulation (682-728)

src/core/vertex.rs (1)

• from_points (419-424)

src/core/util.rs (1)

• extract_hull_facet_set (1059-1081)

src/geometry/util.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (102-104)
• with_kernel (220-256)
• vertices (462-464)

src/lib.rs (4)

src/core/algorithms/locate.rs (1)

• locate (193-266)

src/core/delaunay_triangulation.rs (3)

• triangulation (533-535)
• new (102-104)
• vertices (462-464)

src/core/triangulation_data_structure.rs (1)

• vertices (817-819)

src/geometry/quality.rs (2)

• radius_ratio (223-276)
• normalized_volume (324-397)

tests/proptest_cell.rs (3)

src/core/delaunay_triangulation.rs (4)

• new (102-104)
• vertices (462-464)
• cells (430-432)
• is_valid (884-889)

src/core/cell.rs (4)

• new (389-420)
• vertices (593-595)
• cell (3224-3226)
• is_valid (984-1024)

src/core/triangulation.rs (2)

• vertices (75-77)
• cells (68-70)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Codacy Security Scan
• GitHub Check: build (windows-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: Code Coverage

🔇 Additional comments (72)

scripts/changelog_utils.py (1)

792-824: Keyword argument usage here is correct and improves clarity

Switching _handle_existing_tag, _create_tag_with_message, and _show_success_message calls to use explicit keyword arguments (force_recreate=..., is_truncated=...) matches the callee signatures and keeps behavior identical while making the intent clearer and safer against future parameter reordering. No additional changes needed.

tests/proptest_geometry.rs (1)

12-12: LGTM! Consolidation through prelude aligns with PR objectives.

The migration to delaunay::prelude::* successfully centralizes imports for Point and geometric utilities (circumcenter, circumradius, hypot, inradius, simplex_volume, squared_norm). This change is part of the broader API consolidation documented in the PR objectives and maintains test functionality with all 721 library tests passing.

justfile (3)

99-101: CI recipe now correctly runs integration tests as part of the simulation

Adding test-integration to ci aligns the target with the comment (“all Rust tests”) and with the usual expectation that integration + proptests run in CI. Looks good; just ensure .github/workflows/ci.yml is wired to use just ci (or an equivalent set of steps) so behavior stays in sync.

---

133-136: Coverage base args look coherent; consider whether you want doc-test coverage too

The new _coverage_base_args setup (workspace --lib --tests, bench/example file excludes, test timeout, verbose, implicit-test-threads) is internally consistent and should be friendly to CI environments.

If you care about doc-test coverage, you may want to add tarpaulin’s --doc or similar here; if not, this is fine as-is.

---

163-163: Help text for just ci now matches the actual recipe

The updated help string (“CI simulation (linting + all Rust tests + bench compile)”) accurately reflects the ci: lint test test-integration bench-compile dependencies.

.github/workflows/ci.yml (1)

107-107: LGTM!

Adding cargo test --tests --verbose ensures Windows CI runs integration tests from the tests/ directory, aligning coverage with Linux/macOS platforms that run these via just ci.

tests/proptest_safe_conversions.rs (1)

17-18: LGTM!

Import consolidation to use the prelude aligns with the PR's migration pattern. The test logic remains unchanged.

tests/coordinate_conversion_errors.rs (1)

6-6: LGTM!

Import consolidation to use the prelude. The comprehensive error handling tests remain unchanged and well-structured.

tests/proptest_predicates.rs (1)

9-9: LGTM!

Import consolidation to use the prelude. The property-based geometric predicate tests remain well-designed with good dimensional coverage.

tests/proptest_facet.rs (6)

11-11: LGTM!

Import consolidation to use the prelude aligns with the PR-wide migration.

---

41-42: LGTM!

Correctly migrated from Tds::new to DelaunayTriangulation::new with dt.tds() accessor pattern. The turbofish syntax properly specifies the dimension while inferring kernel, user data types.

---

71-72: LGTM!

Consistent application of the DelaunayTriangulation::new + dt.tds() pattern.

---

103-104: LGTM!

Consistent migration to wrapper-based API access.

---

128-129: LGTM!

Consistent migration pattern applied.

---

171-172: LGTM!

Facet multiplicity test correctly uses the new wrapper-based API.

tests/proptest_point.rs (1)

11-11: LGTM!

Import consolidation to use the prelude. The comprehensive property-based tests for Point operations remain well-structured with good dimensional coverage.

tests/proptest_convex_hull.rs (8)

13-13: LGTM!

Import consolidation to use the prelude.

---

45-49: LGTM!

Correctly migrated to use dt.tds() for boundary facet count and dt.triangulation() for ConvexHull::from_triangulation(), matching the updated API signatures.

---

60-61: LGTM!

Correctly updated to use is_valid_for_triangulation with updated error message terminology.

---

130-148: LGTM!

Staleness detection test correctly uses the new API pattern with dt.triangulation() for hull operations and dt.tds() for boundary facet counts.

---

152-168: LGTM!

Correctly validates hull staleness detection after triangulation modification via dt.insert().

---

185-187: LGTM!

Hull vertices subset test correctly migrated to new API.

---

235-236: LGTM!

Minimal simplex hull test correctly uses new API.

---

257-279: LGTM!

Hull reconstruction consistency test correctly uses dt.triangulation() for both hull creation and facet extraction, ensuring proper validation of reconstruction determinism.

tests/delaunay_incremental_insertion.rs (10)

1-10: LGTM! Comprehensive incremental insertion test coverage.

This new test file provides thorough coverage of the DelaunayTriangulation incremental insertion API across multiple dimensions (2D-5D), kernel variants, and point distributions. The macro-generated approach ensures consistent test patterns while reducing duplication.

---

17-39: Well-structured macro for dimension-agnostic single-point insertion tests.

The macro correctly generates tests verifying:

• Initial vertex count matches input
• Initial cell count is 1 (single simplex)
• After insertion, vertex count increments by 1
• Cell count matches expected value based on insertion topology

---

41-78: Good test case coverage across dimensions.

The expected cell counts (3 for 2D, 4 for 3D, 5 for 4D, 6 for 5D) are correct for inserting a point inside a D-simplex, which creates D+1 new simplices.

---

81-108: LGTM!

The 5-point insertion macro properly validates incremental growth with vertex count assertions after each insertion.

---

175-199: LGTM! Kernel comparison test validates API parity.

Verifying that FastKernel and RobustKernel produce the same vertex count for identical input ensures kernel implementations are functionally equivalent for well-conditioned inputs.

---

201-221: LGTM!

Tests incremental insertion with RobustKernel to ensure the robust geometry predicates work correctly through the insertion pipeline.

---

227-268: Good coverage of challenging point distributions.

Testing clustered points (line 228) and grid patterns (line 249) exercises the insertion algorithm with configurations that can be numerically challenging due to near-degeneracies.

---

274-312: LGTM! Batch vs incremental equivalence test.

Verifying that batch construction and incremental insertion produce the same vertex count validates that both construction paths are functionally equivalent.

---

318-357: LGTM!

Edge case tests cover centroid insertion and minimal simplex scenarios, which are important boundary conditions for the insertion algorithm.

---

363-380: LGTM! f32 coordinate support verification.

Testing with f32 coordinates ensures the generic implementation works correctly with different floating-point precisions.

tests/proptest_cell.rs (3)

12-12: LGTM! Import updated to use the prelude.

The import change from specific module paths to delaunay::prelude::* aligns with the PR's goal of consolidating the API surface and is consistent with how other test files have been updated.

---

42-48: LGTM! Clean migration to DelaunayTriangulation API.

The test correctly uses DelaunayTriangulation::<FastKernel<f64>, (), (), $dim>::new(&vertices) and dt.cells() for iteration. The type parameters are in the correct order (Kernel, VertexData, CellData, Dimension), and the pattern is consistently applied across all dimension-specific tests.

---

107-115: Validation pattern correctly uses the new API.

The test properly chains dt.is_valid().is_ok() before iterating cells, which matches the DelaunayTriangulation::is_valid() signature returning Result<(), TriangulationValidationError>.

tests/proptest_delaunay_triangulation.rs (8)

1-26: Excellent documentation for the test module.

The module-level documentation clearly explains the architectural context (CGAL-inspired layering), the invariants being tested, and the status of each test category. This makes it easy to understand what's being validated and why certain tests are ignored.

---

60-80: Deduplication helper is correct for test purposes.

The O(n²) comparison using to_bits() for exact float equality is appropriate for the small test inputs (max 16 vertices). The labeled loop with continue 'outer is a clean pattern for the nested iteration.

---

105-134: Well-structured incremental insertion validity tests.

The macro correctly generates tests across dimensions 2-5 with appropriate minimum vertex counts (D+1 for initial simplex). Using DelaunayTriangulation::<_, (), (), $dim> with type inference for the kernel is clean, and the vertex! macro usage for creating additional vertices is consistent with the library's API.

---

140-180: Duplicate coordinate rejection tests are well-designed.

The test correctly retrieves an existing vertex from the triangulation (rather than using input vertices that might have been skipped), ensuring accurate duplicate detection testing. The #[ignore] status with documented edge cases is appropriate.

---

226-233: Correct access pattern to underlying TDS for Delaunay validation.

Using dt.tds() to access the underlying triangulation data structure for is_delaunay() validation is the correct pattern per the new API design where DelaunayTriangulation wraps Triangulation which contains Tds.

---

296-317: Co-circularity check has acceptable complexity for test inputs.

The O(n³) loop checking all triangle combinations against all other points is acceptable for the 6-10 point range used in this test (max 120 triangles × 7 points = 840 checks). This thorough general-position filtering helps avoid false negatives from non-unique Delaunay triangulations.

---

352-363: Reasonable tolerance for cell count comparison.

The ±20% or ±2 cell tolerance (whichever is larger) appropriately handles the fact that different valid Delaunay triangulations can have slightly different cell counts for nearly-degenerate inputs.

---

402-407: Near-duplicate jitter is appropriately larger than duplicate threshold.

The 1e-7 jitter is intentionally larger than the library's 1e-10 duplicate detection epsilon, ensuring these "near-duplicates" are distinct points that should be inserted (not rejected as duplicates). This tests the boundary between exact-duplicate rejection and valid near-neighbor insertion.

benches/large_scale_performance.rs (4)

11-11: Documentation correctly updated to reflect new algorithm.

The comment change from "Bowyer-Watson algorithm" to "incremental insertion algorithm" accurately reflects the architectural change in this PR.

---

82-82: Import updated to use the new module path.

The import change from triangulation_data_structure::Tds to delaunay_triangulation::DelaunayTriangulation aligns with the API consolidation.

---

185-190: Construction benchmark correctly updated.

The measured operation properly constructs the DelaunayTriangulation and returns it through black_box to prevent dead code elimination. The setup phase correctly generates vertices without measuring that overhead.

---

246-253: Validation benchmark correctly accesses TDS through wrapper.

The pattern of creating DelaunayTriangulation then accessing dt.tds() for iteration and validation is consistent with the new API design. The throughput measurement correctly uses tds.number_of_cells().

src/lib.rs (8)

20-38: Documentation example correctly demonstrates new API.

The 4D triangulation example properly shows:

• Using delaunay::prelude::* for imports
• DelaunayTriangulation::<_, (), (), 4>::new(&vertices) construction
• Type inference for kernel parameter
• Comments explaining incremental insertion behavior

---

62-78: Convex hull example correctly uses dt.triangulation() accessor.

The pattern of calling dt.triangulation() to pass the underlying Triangulation to geometric operations like ConvexHull::from_triangulation() and hull.is_point_outside() is correct per the new architecture.

---

100-127: Validation documentation is comprehensive and accurate.

The invariants section properly documents:

• All validation helper methods with correct paths
• The distinction between is_valid() (structural) and validation_report() (comprehensive)
• The Delaunay property validation via validate_delaunay()

The table format makes it easy to find the right method for each invariant.

---

166-170: Duplicate detection documentation now correctly references actual error variants.

The documentation properly describes InsertionError::DuplicateCoordinates and InsertionError::DuplicateUuid with correct module paths, addressing the concern from previous reviews about the non-existent InvalidVertex variant.

---

184-204: Excellent documentation of the incremental insertion algorithm.

The five-step algorithm description (locate → identify conflicts → remove → fill cavity → wire neighbors) clearly explains the cavity-based insertion approach. This level of detail helps users understand the library's behavior.

---

361-401: Module structure correctly reflects architectural changes.

The new algorithm modules (flips, incremental_insertion, locate) and triangulation wrappers (delaunay_triangulation, triangulation) are properly declared and re-exported, aligning with the CGAL-inspired layered architecture.

---

450-477: Prelude exports enable the new API surface.

The prelude correctly exports delaunay_triangulation::* and kernel::*, making DelaunayTriangulation, FastKernel, and RobustKernel easily accessible. This supports the concise use delaunay::prelude::* pattern used throughout examples and tests.

---

552-563: Quality function tests correctly use dt.triangulation() accessor.

The test properly demonstrates that quality functions like radius_ratio and normalized_volume operate on Triangulation references, accessed via dt.triangulation().

tests/allocation_api.rs (4)

7-9: Imports correctly updated for new API.

The conditional import of measure and the unconditional import from delaunay::prelude::* align with the test's needs.

---

84-89: Helper correctly uses DelaunayTriangulation::empty() for zero-vertex case.

The function properly creates an empty triangulation without attempting to call new() with an empty vertex slice, which would fail. The fully qualified kernel type is verbose but explicit.

---

96-106: Helper correctly creates 3D triangulation with tetrahedron vertices.

The 4 vertices form a valid 3D simplex (tetrahedron), and the function properly uses DelaunayTriangulation::new(&vertices) with appropriate error handling.

---

170-198: Test functions correctly updated to use new API.

The tests properly:

• Use dt variable naming (consistent with other test files)
• Call dt.number_of_vertices() for assertions
• Update labels from "TDS" to "triangulation" terminology

src/core/triangulation.rs (3)

97-169: Facet and boundary iteration APIs look consistent with TDS behavior

facets() cleanly exposes AllFacetsIter over the underlying Tds, and boundary_facets() wraps tds.build_facet_to_cells_map() with a panic‑on‑corruption policy that matches the docs (“this indicates a bug in the library”). The signatures and examples align with the surrounding architecture; no changes needed.

---

175-195: Best‑effort facet‑sharing repair is now accurately documented

The fix_invalid_facet_sharing docstring explicitly describes a capped, best‑effort loop that may:

• return Ok(total_removed) even if some violations remain, and
• only surface errors when building the facet map fails.

This matches the implementation, which logs internal repair failures in debug builds but does not propagate them. This resolves the earlier doc/behavior mismatch that implied stronger guarantees.

---

196-389: Facet‑sharing repair loop and quality‑based cell culling look internally consistent

The repair loop:

• Early‑exits when validate_facet_sharing() passes.
• Identifies over‑shared facets, filters true facet carriers via vertex‑set subset checks, and then:
  • prefers radius_ratio where available (with safe K::Scalar → f64 conversion),
  • falls back to UUID‑based tie‑breaking when needed.
• Accumulates cells to remove via a set, batches remove_cells_by_keys, then runs duplicate cleanup and localized neighbor/incident repair under a hard iteration cap.

Given the best‑effort contract and MAX_FIX_FACET_ITERATIONS safety cap, this is a reasonable, self‑contained topology repair and matches the stated behavior.

src/core/algorithms/incremental_insertion.rs (3)

28-81: InsertionError surface is well‑factored for incremental insertion flows

The enum cleanly composes locate/conflict/construction errors with higher‑level phases (cavity filling, neighbor wiring, hull extension) and explicit duplicate‑coordinate/UUID cases, with informative Display messages. This should be expressive enough for DelaunayTriangulation callers without overcomplicating the error model.

---

195-321: Facet‑hash–based neighbor wiring for cavity/hull cells looks sound

wire_cavity_neighbors:

• Indexes all facets of new_cells via sorted vertex key sets and a 64‑bit hash.
• Adds matching facets from existing non‑conflict cells (including hull extension cases) but caps each facet at 2 incident cells to avoid non‑manifold topology.
• Then wires mutual neighbors for all 2‑cell facets and rejects any facet attached to >2 cells.

Given the capped cardinality and the use of sorted vertex sets before hashing, this is a reasonable and efficient neighbor‑rebuild strategy for both interior cavities and hull extension. The explicit non‑manifold guard also provides a clear failure mode for unexpected topologies.

---

528-632: Macro‑generated cavity tests give good dimensional coverage

The test_fill_cavity! macro builds a simplex in D=2..5, constructs boundary facets, calls fill_cavity and wire_cavity_neighbors, and then checks TDS validity and cell vertex counts. This is a nice smoke‑test that the cavity construction + local wiring behave consistently across dimensions.

src/geometry/util.rs (4)

13-19: Surface‑measure example and imports are aligned with the DT‑centric API

Switching the docs to:

• use delaunay::prelude::*; and
• constructing a DelaunayTriangulation::new(&vertices) then calling .tds().boundary_facets()

matches the new public surface where callers go through the DT wrapper rather than raw Tds. The added crate‑internal import of DelaunayTriangulation also keeps tests and utilities consistent with this pattern.

Also applies to: 1557-1569

---

3391-3463: Tests updated to use DelaunayTriangulation over raw TDS are consistent and focused

The updated tests for surface_measure and related geometry now:

• Build Vec<Vertex<_, (), D>>,
• Construct a DelaunayTriangulation<_, (), (), D>::new(&vertices)?,
• Pull boundary facets via dt.tds().boundary_facets()?, and
• Feed those FacetViews into surface_measure.

This matches the intended “DT wrapper + tds() accessor” architecture and keeps tests close to real user code. The conversions and expected areas/perimeters stay correct; no issues spotted.

Also applies to: 3782-3857, 3870-3902, 3923-3933, 3985-3994

---

4814-4821: Random‑triangulation tests exercise new API paths appropriately

The generate_random_triangulation tests now:

• Assert reasonable vertex counts/dimensions and is_valid() on returned DelaunayTriangulations for several (T, U, D) combinations.
• Check reproducibility across seeds and dimensions.
• Verify behavior with various vertex data types ((), integers, fixed [char; 8]).

Given the underlying construction has moved to the incremental cavity‑based path, these are good smoke tests for the new integration.

Also applies to: 4883-4885, 4927-4963, 4965-5031

---

5444-5499: Error type Display implementations are clear and user‑friendly

The Display formatting for ValueConversionError, RandomPointGenerationError, and CircumcenterError variants produces concrete, parameterized messages (including offending values and type names) that should greatly aid debugging without leaking internal details. No changes suggested.

src/core/delaunay_triangulation.rs (1)

70-135: Constructors and empty/with_empty_kernel bootstrap semantics look consistent with insert and tests

new, empty, and with_empty_kernel now all funnel into Triangulation::new_empty(kernel) + the unified insert path, and the docs’ promise about bootstrapping on the (D+1)‑th vertex matches the implementation (insert short‑circuits for < D+1 and calls build_initial_simplex at == D+1). The test matrix in this file (test_bootstrap_from_empty_*, test_empty_supports_incremental_insertion, and test_bootstrap_with_custom_kernel) exercises both the FastKernel and RobustKernel paths across dimensions, so the constructor surface looks solid.

Also applies to: 145-181

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +2.28% (target: -1.00%)	✅ 56.08%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (14c81a0)	5982	3788	63.32%
Head commit (3991a08)	3951 (-2031)	2592 (-1196)	65.60% (+2.28%)

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 (#132)	1512	848	56.08%

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%

See your quality gate settings    Change summary preferences

[codecov]

Codecov Report

❌ Patch coverage is 56.32184% with 494 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.58%. Comparing base (14c81a0) to head (3991a08).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/core/triangulation.rs	42.10%	297 Missing ⚠️
src/core/algorithms/incremental_insertion.rs	61.99%	122 Missing ⚠️
src/core/delaunay_triangulation.rs	51.06%	46 Missing ⚠️
src/core/algorithms/locate.rs	86.33%	19 Missing ⚠️
src/geometry/util.rs	71.42%	4 Missing ⚠️
src/core/util.rs	50.00%	2 Missing ⚠️
src/geometry/kernel.rs	93.33%	2 Missing ⚠️
src/geometry/quality.rs	86.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   63.32%   65.58%   +2.25%     
==========================================
  Files          20       21       +1     
  Lines        5980     3949    -2031     
==========================================
- Hits         3787     2590    -1197     
+ Misses       2193     1359     -834     

Flag	Coverage Δ
unittests	65.58% <56.32%> (+2.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Cargo.toml (1)

4-4: Verify uuid version 1.19.0—it does not exist on crates.io.

The edition = "2024" is valid (stabilized February 20, 2025) and compatible with rust-version = "1.91.0". However, line 36 specifies uuid = "1.19.0", but this version does not exist on crates.io—the latest 1.x release is 1.18.1. This will cause a build failure. Update to an existing version like 1.18.1 or verify the correct version number. The criterion 0.8.0 dependency is valid.

♻️ Duplicate comments (1)

src/lib.rs (1)

165-169: Documentation now correctly references actual error variants.

The duplicate vertex detection section now accurately references InsertionError::DuplicateCoordinates and InsertionError::DuplicateUuid, which matches the current implementation in core::algorithms::incremental_insertion.

🧹 Nitpick comments (10)

docs/property_testing_summary.md (1)

24-26: Align module/coverage wording across the summary

The new Tds / Triangulation / Delaunay sections and layered stats read well and match the current architecture, but a couple of small inconsistencies crept in:

• Line 24 still says “Three new property test modules were created” even though this doc now describes additional modules (Tds, Triangulation, DelaunayTriangulation layers, etc.).
• The body emphasizes 2D–5D coverage, while the conclusion still talks about “2D–4D triangulations”.

Consider updating the intro and conclusion wording to reflect the expanded module set and 2D–5D coverage so the document stays self-consistent.

Also applies to: 76-128, 191-211, 248-249

tests/README.md (1)

280-291: Update README references to removed Tds::new / Bowyer–Watson APIs

These sections still describe construction and validation in terms of Tds::new and Tds::bowyer_watson_with_diagnostics, and direct Delaunay validation on a standalone TDS:

• Proptest duplicates section (lines 288–291).
• regression_delaunay_known_configs.rs section (lines 697–708).

Given this PR removes Tds::new() and the legacy Bowyer–Watson algorithms in favor of a DelaunayTriangulation-based and incremental insertion pipeline, the README should be updated to reflect the new construction and validation flow (e.g., via DelaunayTriangulation::new / the incremental insertion module, and whatever the new Delaunay-validation entrypoint is) so test docs don’t point at APIs that no longer exist.

Also applies to: 695-708

src/geometry/util.rs (1)

1989-2181: Random triangulation helper refactor is sound; API/semantics are coherent

• The new return type Result<DelaunayTriangulation<FastKernel<T>, U, V, D>, TriangulationConstructionError> matches how DelaunayTriangulation::with_kernel is constructed and how consumers are now expected to work.
• The n_points == 0 fast path returning DelaunayTriangulation::with_empty_kernel(FastKernel::new()) is consistent with the tests and with dim() == -1 semantics for empty triangulations.
• Mapping RandomPointGenerationError into TriangulationConstructionError::GeometricDegeneracy is a bit coarse but is clearly documented in the error section and keeps the API’s error surface narrow.

No functional issues here; the implementation and docs/tests are in sync.

If you later decide you want finer‑grained error reporting, consider introducing a dedicated construction error variant (e.g., RandomSource) instead of overloading GeometricDegeneracy, but this is not required for correctness.

src/core/delaunay_triangulation.rs (1)

527-668: Incremental insert path fixes prior TDS‑replacement bug; degenerate errors are conservative

• For num_vertices < D + 1 you delegate entirely to self.tri.insert(...), letting the core handle bootstrap and initial simplex creation. This removes the old pattern where DelaunayTriangulation rebuilt the TDS and returned keys from the pre‑replacement structure, which previously risked VertexKey invalidation.
• For num_vertices >= D + 1 you compute a LocateResult, derive a conflict region for InsideCell, and pass this (or None for Outside) into Triangulation::insert, so insertion and key allocation remain centralized in the core.
• The conservative handling of OnFacet/OnEdge/OnVertex by returning a CavityFilling error with a clear TODO is acceptable for now and matches the function docs.

When you come back to refine the error taxonomy, mapping OnVertex to a dedicated InsertionError::DuplicateCoordinates and splitting facet/edge degeneracies into their own variants will make downstream error handling much clearer.

tests/proptest_delaunay_triangulation.rs (2)

207-215: Consider using prop_assert! for Delaunay validation.

The current code uses .unwrap() which will panic on failure. Using prop_assert! would provide better error messages with shrunk counterexamples.

                     // Verify the triangulation satisfies the Delaunay property
-                    is_delaunay(dt.tds()).unwrap();
+                    prop_assert!(
+                        is_delaunay(dt.tds()).is_ok(),
+                        "{}D triangulation should satisfy Delaunay property: {:?}",
+                        $dim,
+                        is_delaunay(dt.tds()).err()
+                    );

---

382-391: Consider using prop_assert! for consistency with other tests.

Similar to the circumsphere tests, using prop_assert! would provide better error messages.

                     // Structural and Delaunay validity for kept subset
-                    prop_assert!(dt.is_valid().is_ok());
-                    prop_assert!(is_delaunay(dt.tds()).is_ok());
+                    prop_assert!(
+                        dt.is_valid().is_ok(),
+                        "{}D triangulation should be structurally valid: {:?}",
+                        $dim,
+                        dt.is_valid().err()
+                    );
+                    prop_assert!(
+                        is_delaunay(dt.tds()).is_ok(),
+                        "{}D triangulation should satisfy Delaunay property: {:?}",
+                        $dim,
+                        is_delaunay(dt.tds()).err()
+                    );

benches/assign_neighbors_performance.rs (3)

185-198: Variable name dt is misleading for a cloned TDS.

The variable dt typically denotes a DelaunayTriangulation, but here it holds a cloned Tds. This contrasts with the macro-generated benchmarks where tds is used consistently for the cloned TDS. Consider renaming for clarity.

                    || {
                        let points = generate_grid_points_3d(grid_size);
                        let vertices: Vec<_> = points.iter().map(|p| vertex!(*p)).collect();
-                        let mut dt =
+                        let mut tds =
                            DelaunayTriangulation::<FastKernel<f64>, (), (), 3>::new(&vertices)
                                .unwrap()
                                .tds()
                                .clone();

                        // Clear existing neighbors to benchmark the assignment process
-                        dt.clear_all_neighbors();
-                        dt
+                        tds.clear_all_neighbors();
+                        tds
                    },
-                    |mut dt| {
-                        dt.assign_neighbors().unwrap();
-                        black_box(dt);
+                    |mut tds| {
+                        tds.assign_neighbors().unwrap();
+                        black_box(tds);
                    },

---

224-237: Same naming inconsistency as grid benchmark.

The spherical benchmark also uses dt for a cloned Tds. Consider renaming to tds for consistency with the macro-generated benchmarks.

---

266-278: Same naming pattern inconsistency in 2D multi-dimensional benchmark.

The 2D benchmark uses dt for the cloned TDS while 3D/4D/5D benchmarks correctly use tds. Consider using tds consistently.

src/core/algorithms/incremental_insertion.rs (1)

269-273: O(n) lookup in new_cells.contains() may become a bottleneck for large cavities.

For each existing cell, you iterate through all new cells to check membership. With k new cells and m existing cells, this becomes O(m * k). Consider building a FastHashSet<CellKey> from new_cells for O(1) lookups, similar to what's done for conflict_set.

+    // Build new cells set for O(1) lookup
+    let new_cells_set: FastHashSet<CellKey> = new_cells.iter().copied().collect();
+
     // Collect existing cell keys first to avoid borrow issues
     let existing_cell_keys: Vec<CellKey> = tds
         .cells()
         .map(|(key, _)| key)
-        .filter(|&key| !new_cells.contains(&key) && !conflict_set.contains(&key))
+        .filter(|&key| !new_cells_set.contains(&key) && !conflict_set.contains(&key))
         .collect();

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4c5d1f and 9b58653.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (20)

• Cargo.toml (1 hunks)
• benches/assign_neighbors_performance.rs (11 hunks)
• benches/large_scale_performance.rs (8 hunks)
• cspell.json (3 hunks)
• docs/code_organization.md (5 hunks)
• docs/property_testing_summary.md (2 hunks)
• scripts/benchmark_utils.py (4 hunks)
• scripts/tests/test_benchmark_utils.py (1 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/algorithms/locate.rs (1 hunks)
• src/core/boundary.rs (23 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/traits/boundary_analysis.rs (7 hunks)
• src/core/triangulation.rs (1 hunks)
• src/geometry/util.rs (22 hunks)
• src/lib.rs (17 hunks)
• tests/README.md (1 hunks)
• tests/delaunay_incremental_insertion.rs (1 hunks)
• tests/proptest_convex_hull.rs (9 hunks)
• tests/proptest_delaunay_triangulation.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/delaunay_incremental_insertion.rs
• src/core/traits/boundary_analysis.rs

🧰 Additional context used

📓 Path-based instructions (1)

scripts/*.py

⚙️ CodeRabbit configuration file

scripts/*.py: These are Python utility scripts for changelog and commit processing.
Focus on code quality, maintainability, and adherence to Python best practices.
The scripts use ruff for comprehensive linting and formatting (pylint has been retired).

Files:

• scripts/benchmark_utils.py

🧠 Learnings (9)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• benches/large_scale_performance.rs
• tests/proptest_delaunay_triangulation.rs
• tests/proptest_convex_hull.rs
• src/core/algorithms/incremental_insertion.rs
• docs/property_testing_summary.md
• tests/README.md
• benches/assign_neighbors_performance.rs
• src/core/boundary.rs
• src/core/algorithms/locate.rs
• src/core/triangulation.rs
• src/geometry/util.rs
• src/lib.rs
• src/core/delaunay_triangulation.rs
• docs/code_organization.md

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• benches/large_scale_performance.rs
• tests/proptest_delaunay_triangulation.rs
• tests/proptest_convex_hull.rs
• src/core/algorithms/incremental_insertion.rs
• docs/property_testing_summary.md
• tests/README.md
• src/core/boundary.rs
• src/core/algorithms/locate.rs
• src/core/triangulation.rs
• src/geometry/util.rs
• src/lib.rs
• src/core/delaunay_triangulation.rs
• docs/code_organization.md

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• benches/large_scale_performance.rs
• tests/proptest_delaunay_triangulation.rs
• tests/proptest_convex_hull.rs
• src/core/algorithms/incremental_insertion.rs
• docs/property_testing_summary.md
• tests/README.md
• src/core/boundary.rs
• src/core/algorithms/locate.rs
• src/core/triangulation.rs
• src/geometry/util.rs
• src/lib.rs
• src/core/delaunay_triangulation.rs
• docs/code_organization.md

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait includes PartialOrd through its Float supertrait bound, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• src/core/boundary.rs

📚 Learning: 2025-08-20T21:55:04.552Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 50
File: src/core/algorithms/robust_bowyer_watson.rs:191-195
Timestamp: 2025-08-20T21:55:04.552Z
Learning: CoordinateScalar trait guarantees PartialOrd, making direct comparisons between CoordinateScalar values valid without needing conversion to f64.

Applied to files:

• src/core/boundary.rs

📚 Learning: 2025-09-02T20:32:05.985Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 60
File: cspell.json:103-103
Timestamp: 2025-09-02T20:32:05.985Z
Learning: In cspell.json for the delaunay project, the word "itional" is intentionally added to the dictionary because it comes from a regex pattern, not a typo.

Applied to files:

• cspell.json

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• src/lib.rs
• docs/code_organization.md

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• docs/code_organization.md

🧬 Code graph analysis (9)

benches/large_scale_performance.rs (1)

src/core/delaunay_triangulation.rs (3)

• new (97-99)
• vertices (392-394)
• tds (421-423)

tests/proptest_delaunay_triangulation.rs (4)

src/core/delaunay_triangulation.rs (5)

• new (97-99)
• vertices (392-394)
• insert (612-668)
• tds (421-423)
• number_of_vertices (275-277)

src/core/triangulation.rs (3)

• vertices (138-140)
• insert (455-554)
• number_of_vertices (159-161)

src/core/triangulation_data_structure.rs (2)

• vertices (796-798)
• number_of_vertices (889-891)

src/core/vertex.rs (2)

• from_points (419-424)
• point (475-477)

src/core/algorithms/incremental_insertion.rs (5)

src/core/facet.rs (12)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• key (580-602)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)

src/core/cell.rs (6)

• cell (3224-3226)
• uuid (710-712)
• new (389-420)
• neighbors (561-563)
• vertices (593-595)
• map (323-323)

src/core/vertex.rs (9)

• point (475-477)
• from (681-683)
• from (694-696)
• from (707-709)
• from (772-774)
• uuid (502-504)
• None (1895-1895)
• map (293-293)
• map (303-303)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/triangulation.rs (2)

• boundary_facets (273-281)
• vertices (138-140)

scripts/tests/test_benchmark_utils.py (1)

scripts/benchmark_utils.py (1)

• _write_error_file (1811-1823)

src/core/boundary.rs (3)

src/core/delaunay_triangulation.rs (3)

• new (97-99)
• boundary_facets (523-525)
• tds (421-423)

src/core/triangulation.rs (1)

• boundary_facets (273-281)

src/core/traits/boundary_analysis.rs (2)

• boundary_facets (79-81)
• is_boundary_facet_with_map (177-181)

src/core/algorithms/locate.rs (2)

src/core/triangulation_data_structure.rs (3)

• vertices (796-798)
• contains_cell (834-836)
• cell_keys (814-816)

src/core/cell.rs (3)

• cell (3224-3226)
• neighbors (561-563)
• vertices (593-595)

src/core/triangulation.rs (4)

src/geometry/quality.rs (1)

• radius_ratio (223-276)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/algorithms/incremental_insertion.rs (3)

• extend_hull (397-425)
• fill_cavity (112-185)
• wire_cavity_neighbors (210-336)

src/core/algorithms/locate.rs (1)

• locate (183-256)

src/geometry/util.rs (1)

src/core/delaunay_triangulation.rs (6)

• new (97-99)
• with_empty_kernel (170-175)
• with_kernel (215-251)
• vertices (392-394)
• facets (493-495)
• boundary_facets (523-525)

src/lib.rs (3)

src/core/algorithms/locate.rs (1)

• locate (183-256)

src/core/delaunay_triangulation.rs (3)

• triangulation (463-465)
• new (97-99)
• vertices (392-394)

src/core/triangulation_data_structure.rs (1)

• vertices (796-798)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Security Scan
• GitHub Check: Codacy Static Code Analysis
• GitHub Check: build (macos-latest)
• GitHub Check: performance-regression
• GitHub Check: Code Coverage
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (windows-latest)

🔇 Additional comments (49)

cspell.json (1)

51-52: Vocabulary additions are well-aligned with the PR's geometric focus.

The new spell-check dictionary entries (Pion, Teillaud, bistellar, retriangulate/retriangulated/retriangulates) are all appropriate domain terminology for a computational geometry project undergoing triangulation algorithm refactoring. The retriangulation verb forms properly extend the existing "retriangulation" noun entry.

Also applies to: 146-146, 314-316

scripts/benchmark_utils.py (1)

1439-1441: Structured error handling in compare_with_baseline looks solid

Using _write_error_file for missing baseline, missing results, timeouts, and generic exceptions gives CI a consistent, human-readable failure artifact, and the helper is defensive about its own failures (logs but doesn’t re-raise). Call sites pass only str | Path as documented, and directory creation is safe. No issues from a correctness or robustness standpoint.

Also applies to: 1461-1466, 1477-1486, 1811-1823

tests/README.md (1)

60-84: New layered property-testing sections are clear and consistent

The added descriptions for proptest_tds.rs, proptest_triangulation.rs, and proptest_delaunay_triangulation.rs do a good job of mapping tests onto the Tds / Triangulation / DelaunayTriangulation layers, with accurate run commands and dimension ranges. This aligns well with the new architecture and the separate summary in docs/property_testing_summary.md.

Also applies to: 85-109, 112-138

scripts/tests/test_benchmark_utils.py (1)

576-632: Good coverage of _write_error_file behavior

These tests nicely pin down the new helper’s contract: they assert the exact error header/body format, confirm parent directory creation, and verify that write failures are handled non-fatally (no exception propagated, no file created). This should keep the CI-facing error output stable across refactors.

docs/code_organization.md (1)

38-171: Directory tree and architecture docs now align with actual layout

The updated tree and architecture overview correctly reflect the current module/test structure (including geometry/traits/coordinate.rs and new algorithms/{flips,incremental_insertion,locate}.rs), and avoid the stale trait paths called out previously in WARP/CONTRIBUTING. No changes needed here.
Based on learnings, this matches the current repository structure.

src/core/algorithms/locate.rs (1)

103-598: Facet-walk locate + conflict/cavity routines look consistent and well-tested

The overall design of locate, is_point_outside_facet, find_conflict_region, and extract_cavity_boundary matches the documented invariants (facet index ↔ opposite vertex) and the incremental-insertion pipeline. Use of SmallBuffer, FastHashSet/CellSecondaryMap, and the 2D–5D test coverage give good confidence in correctness and robustness for both fast and robust kernels.

src/core/triangulation.rs (1)

782-997: Triangulation repair and simplex-construction helpers are consistent and well-documented

The fix_invalid_facet_sharing implementation plus its docs now clearly describe the best‑effort nature of the repair, matching the actual behavior (bounded iterations, quality/UUID‑based selection, and non-fatal topology-repair failures). Combined with build_initial_simplex and the accompanying 2D–5D/user-data tests, the geometric/topological utilities in this module look cohesive and appropriate for the new Triangulation layer.

src/geometry/util.rs (2)

13-25: DelaunayTriangulation integration in util module looks consistent

The new imports and the surface_measure doc/example using DelaunayTriangulation::new(&vertices) and dt.tds().boundary_facets() line up with the new public API and underlying tds() accessor semantics. This keeps geometry utilities aligned with the wrapper‑centric design while still exposing the low‑level TDS where needed.

Also applies to: 1557-1572

---

4823-4907: Random triangulation tests cover empty/invalid cases well

The updated tests around generate_random_triangulation_* correctly exercise:

• invalid bounds mapping to an error, and
• n_points == 0 returning a structurally empty but valid triangulation (number_of_vertices() == 0, dim() == -1).

These directly validate the new constructor behavior and the early‑return path.

src/core/delaunay_triangulation.rs (6)

52-131: Wrapper struct and FastKernel specialization are well‑factored

The DelaunayTriangulation<K, U, V, D> wrapper around Triangulation with a cached last_inserted_cell plus the FastKernel<f64> specialization (new/empty) give a clean separation between the ergonomic public API and the generic core. The docs for new and empty now accurately describe behavior and align with the tests that exercise incremental insertion from the empty state.

---

140-252: with_empty_kernel/with_kernel constructors correctly encode bootstrap rules

• with_empty_kernel now clearly documents its “advanced usage” role and delegates to Triangulation::new_empty(kernel), matching how empty triangulations are used in tests and examples.
• with_kernel guards against vertices.len() < D + 1 with a structured InsufficientVertices error and then builds the initial simplex via Triangulation::build_initial_simplex, followed by incremental insertion of remaining vertices.

This eliminates the need for DelaunayTriangulation itself to reconstruct the TDS, which is an important architectural improvement over the previous design.

---

253-525: Accessors for cells/vertices/TDS/triangulation and boundary facets are coherent

The forwarding methods (number_of_vertices, number_of_cells, dim, cells, vertices, tds, triangulation, facets, boundary_facets) are thin, correctly typed wrappers over the underlying Triangulation/Tds APIs, and their examples match the intended usage pattern (DelaunayTriangulation as the entry point, tds() when low‑level access is needed). No issues here.

---

670-995: Removal, validation helpers, and Delaunay checking are well‑layered

• remove_vertex explicitly delegates to Triangulation::remove_vertex and documents that fan retriangulation may temporarily violate Delaunay, which is honest about current behavior.
• Structural validation helpers (is_valid, validate_*, validation_report) are thin pass‑throughs to Tds and correctly constrain on K::Scalar: CoordinateScalar where required.
• validate_delaunay cleanly adapts DelaunayValidationError into TriangulationValidationError, enriching messages with cell UUIDs where available.

These utilities provide a solid diagnostic surface without duplicating core logic.

---

997-1098: Serialization and validation policy: practical and clearly documented

• The generic Serialize impl serializing only the Tds and the specialized Deserialize for DelaunayTriangulation<FastKernel<f64>, (), (), D> via from_tds align with the “kernel is stateless” design and the docs explain the intended pattern for custom kernels.
• DelaunayCheckPolicy is documented as experimental and unused, which is fine; its shape (EndOnly vs EveryN) is a sensible foundation for future insertion‑time validation controls.

No issues; behavior and docs are aligned.

---

1106-1701: Test suite robustly exercises bootstrap, incremental insertion, and key validity

• The test_incremental_insertion! macro and generated tests across 2D–5D thoroughly cover:
  • batch vs bootstrap construction equivalence,
  • automatic creation of the initial simplex at D+1 vertices, and
  • subsequent cavity‑based insertion.
• test_insert_single_interior_point_2d explicitly verifies that the VertexKey returned from insert can be looked up in tds, which guards against the prior bootstrap key‑invalidation issue.
• Tests for empty(), with_empty_kernel(), and custom kernels (RobustKernel) confirm that incremental insertion from the empty state now works as documented.
• The various tds()/internal‑TDS accessor tests ensure that the exposed views remain consistent after mutations and that structural invariants still hold.

Together, these tests give strong confidence in the new architecture and address the concerns raised in the earlier review about key stability and empty‑constructor semantics.

src/core/boundary.rs (5)

19-24: LGTM! Simplified trait bounds are appropriate.

The removal of AddAssign<T>, SubAssign<T>, and Sum bounds is correct—these were likely inherited from prior implementations but aren't required by the BoundaryAnalysis trait methods, which only need CoordinateScalar + NumCast for facet key computation and iteration.

---

56-69: Doctests correctly migrated to the new wrapper API.

The examples now demonstrate the intended public API pattern: construct via DelaunayTriangulation::new(), access boundary facets via dt.boundary_facets(). This aligns with the PR's goal of making DelaunayTriangulation the primary entry point.

---

102-116: Doctest correctly demonstrates dt.tds().is_boundary_facet() pattern.

The example shows proper access to BoundaryAnalysis trait methods through the tds() accessor when needed for lower-level operations.

---

144-164: Doctest demonstrates efficient batch boundary checking.

The example correctly shows the recommended pattern: build the facet map once via dt.tds().build_facet_to_cells_map(), then use is_boundary_facet_with_map() for multiple queries. This avoids O(N·F) recomputation per call.

---

222-226: Test imports updated for new API.

The test module correctly imports DelaunayTriangulation and uses TriangulationValidationError for error handling.

tests/proptest_convex_hull.rs (4)

12-14: LGTM! Clean import using prelude.

Using delaunay::prelude::* provides access to all necessary types including DelaunayTriangulation, ConvexHull, Vertex, Point, and related utilities.

---

42-72: Property test correctly migrated to wrapper API.

The test properly uses:

• DelaunayTriangulation::new() for construction
• dt.tds().number_of_boundary_facets() for TDS-level operations
• ConvexHull::from_triangulation(dt.triangulation()) for hull construction
• hull.is_valid_for_triangulation(dt.triangulation()) for validation

This demonstrates the correct layered API access pattern.

---

130-175: Staleness detection test correctly validates hull invalidation.

The test properly verifies that:

1. Hull is valid for the original triangulation
2. After dt.insert(), the original hull becomes stale
3. A new hull can be constructed from the modified triangulation

The use of dt.insert() for incremental insertion aligns with the new API.

---

275-291: Facet set extraction uses correct API.

The test correctly uses extract_hull_facet_set(&hull, dt.triangulation()) for comparing facet topology between reconstructed hulls. The Jaccard similarity check with threshold 1.0 ensures exact equality.

src/lib.rs (6)

20-41: Excellent documentation update for the new API.

The basic usage example clearly demonstrates:

• 4D triangulation construction via DelaunayTriangulation::new()
• The incremental insertion model (D+1 vertices for initial simplex, then incremental)
• Key insight about cavity-based insertion

This sets the right expectations for users of the library.

---

183-228: New incremental insertion algorithm section is well-documented.

The documentation clearly explains:

1. Initial simplex construction with D+1 vertices
2. Incremental insertion steps (locate → conflict region → cavity → fill → wire)
3. Error handling semantics
4. The relationship to Delaunay validation

This provides valuable context for users understanding the algorithm.

---

248-274: Degenerate input handling example correctly demonstrates expected behavior.

The example shows that collinear points in 2D fail with TriangulationConstructionError::FailedToAddVertex, which accurately reflects the current implementation where degenerate simplices cannot be processed.

---

347-350: Deprecation notice is helpful for API migration.

The comment clarifies that this is a temporary allowance during v0.6.0 API migration, specifically for Facet -> FacetView and Tds::new()/add() -> DelaunayTriangulation::new()/insert() transitions.

---

360-378: Module structure correctly reflects new architecture.

The algorithms module now exposes:

• flips - Bistellar flip operations
• incremental_insertion - Cavity-based insertion
• locate - Point location algorithms

And the core modules include delaunay_triangulation and triangulation as the primary public types.

---

449-480: Prelude exports are well-organized.

The prelude now includes:

• DelaunayTriangulation from core
• locate, LocateResult, LocateError from algorithms
• Collection types for advanced usage
• Geometry types including the new kernel module

This provides a convenient one-import solution for common usage patterns.

tests/proptest_delaunay_triangulation.rs (5)

1-26: Well-structured property test module with clear architectural context.

The documentation clearly explains:

• The layered architecture (Tds → Triangulation → DelaunayTriangulation)
• Which invariants are tested at each layer
• Current test status (passing vs. ignored with issue references)

This makes it easy to understand what's being tested and what's pending.

---

60-80: Deduplication helper uses O(n²) algorithm - acceptable for test sizes.

The dedup_vertices_by_coords function performs pairwise comparison which is O(n²). For the test vertex counts (6-16 vertices), this is acceptable. The bitwise comparison (to_bits()) correctly handles exact duplicate detection without float comparison issues.

---

86-116: Incremental insertion validity tests cover 2D-5D.

The tests verify that:

1. Initial triangulation from DelaunayTriangulation::new() is valid
2. After insert(), the triangulation remains valid

Good coverage across dimensions with appropriate vertex count ranges.

---

121-161: Duplicate coordinate rejection tests appropriately marked as ignored.

The tests are ignored with a clear explanation that edge cases with degenerate/nearly-degenerate configurations are failing. The comment "needs investigation into duplicate detection logic" provides actionable guidance for future work.

---

231-319: Insertion-order robustness tests are well-designed.

The tests correctly:

1. Use deterministic RNG seeding for reproducibility
2. Apply deduplication and degeneracy filtering
3. Build two triangulations with different insertion orders
4. Verify both are structurally valid with matching vertex counts
5. Document that exact topology may differ (valid for co-spherical points)

The TODO comment about bistellar flips is appropriate - this is a known limitation.

benches/large_scale_performance.rs (5)

10-15: Documentation correctly updated for new algorithm.

The benchmark description now accurately refers to "incremental insertion algorithm" instead of the removed Bowyer-Watson algorithm.

---

82-88: Import correctly updated to use DelaunayTriangulation.

The benchmark now imports from delaunay::core::delaunay_triangulation::DelaunayTriangulation which is the new primary API.

---

135-160: Memory measurement correctly updated for wrapper API.

The function now:

1. Constructs via DelaunayTriangulation::new()
2. Uses black_box(&dt) to prevent the triangulation from being optimized away before memory measurement

This ensures accurate memory delta measurements for the triangulation data structure.

---

187-198: Construction benchmark correctly uses new API.

The benchmark properly constructs via DelaunayTriangulation::new(black_box(&vertices)) and returns the result through black_box(dt) to prevent dead code elimination.

---

244-256: Validation benchmark correctly accesses TDS via wrapper.

The pattern DelaunayTriangulation::new(&vertices) followed by dt.tds() is the correct way to access the underlying TDS for topology validation benchmarks.

benches/assign_neighbors_performance.rs (3)

75-89: LGTM!

The migration to DelaunayTriangulation::new() is correctly implemented. The setup properly constructs the triangulation, clones the underlying TDS, clears neighbors, and then benchmarks assign_neighbors.

---

112-115: LGTM!

Scaling benchmark correctly uses the new API pattern for both pre-computation diagnostics and the actual benchmark setup.

Also applies to: 141-142

---

247-248: LGTM on allowing too_many_lines.

The #[allow(clippy::too_many_lines)] attribute is appropriate here given the repetitive benchmark structure across dimensions. Refactoring into a macro would add complexity without significant benefit for benchmark code.

src/core/algorithms/incremental_insertion.rs (6)

26-79: LGTM! Well-structured error enum.

The InsertionError enum provides comprehensive coverage of failure modes with clear error messages. The DuplicateCoordinates and DuplicateUuid variants provide backward-compatible error reporting for duplicate insertions.

---

96-111: Excellent documentation on partial mutation semantics.

The explicit warning about non-transactional behavior is valuable. This is the right trade-off for a low-level insertion primitive, and the documentation clearly explains the caller's responsibility.

---

367-377: Facet hashing approach is correct but relies on deterministic hasher.

DefaultHasher (SipHash-1-3) is deterministic within a single process run, which is sufficient here since the hash map is ephemeral. The sorted vertex keys ensure canonical facet representation.

---

516-525: Visibility logic is correct; consider simplifying the condition.

The current condition is clear but can be simplified using sign comparison:

let is_visible = orientation_with_opposite.signum() != 0 
    && orientation_with_opposite.signum() == -orientation_with_point.signum();

Or equivalently, keeping your explicit form is fine for clarity. The conservative coplanar handling (treating zero as non-visible) is well-documented and appropriate.

---

541-593: LGTM! Thorough macro-based test coverage.

The test macro cleanly generates cavity filling tests across dimensions 2-5, validating both cell creation count and TDS integrity via is_valid(). Good use of parameterization for expected facet counts.

---

356-362: Direct field assignment to cell.neighbors is the intended design pattern for crate-internal code.

The neighbors field is declared as pub(crate), explicitly making it public within the crate. The field's documentation states: "Access via neighbors() method. Writable by TDS for neighbor assignment." This confirms direct assignment is intentional and used consistently throughout the codebase (triangulation_data_structure.rs, cell.rs, and incremental_insertion.rs). No setter method exists because the design allows direct mutation by the TDS. This is not a violation of invariants—it's the designated API for neighbor updates.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

scripts/tests/test_benchmark_utils.py (2)

364-398: Consider renaming test_compare_uses_quiet for clarity.

The test name test_compare_uses_quiet is misleading since it now verifies that --quiet is not used. A clearer name would be test_compare_without_quiet_flag or test_compare_omits_quiet_flag to match the actual assertion.

     @pytest.mark.parametrize("dev_mode", [False, True])
     @patch("benchmark_utils.run_cargo_command")
-    def test_compare_uses_quiet(self, mock_cargo, dev_mode):
+    def test_compare_omits_quiet_flag(self, mock_cargo, dev_mode):
         """Test that PerformanceComparator invokes cargo without --quiet flag (removed for better error visibility)."""

---

617-631: Remove unused readonly_dir setup.

The readonly_dir.mkdir() on line 623 creates a directory that is never actually used or made read-only. The test relies on mocking Path.open to simulate the write failure, making this setup dead code.

     def test_write_error_file_handles_write_failure(self, comparator):
         """Test that _write_error_file handles write failures gracefully."""
-        # Create a read-only directory to trigger write failure
         with tempfile.TemporaryDirectory() as temp_dir:
-            output_file = Path(temp_dir) / "readonly" / "error_results.txt"
-            readonly_dir = Path(temp_dir) / "readonly"
-            readonly_dir.mkdir()
+            output_file = Path(temp_dir) / "error_results.txt"

             # Mock Path.open to raise an exception
             with patch.object(Path, "open", side_effect=OSError("Permission denied")):
                 # Should not raise exception, just log it
                 comparator._write_error_file(output_file, "Test error", "Test details")

             # File should not exist due to write failure
             assert not output_file.exists()

src/core/algorithms/incremental_insertion.rs (1)

370-380: Consider using a deterministic hasher for facet key computation.

DefaultHasher may produce different hash values across different Rust versions or platforms due to its randomized seed. While this is fine for runtime hash maps, if facet keys are ever persisted or compared across runs, consider using a deterministic hasher like FNV (already used elsewhere in the codebase per the README's Jaccard similarity section).

 fn compute_facet_hash(sorted_vkeys: &[VertexKey]) -> u64 {
-    use std::collections::hash_map::DefaultHasher;
-    use std::hash::{Hash, Hasher};
+    use std::hash::Hash;
+    use rustc_hash::FxHasher;
 
-    let mut hasher = DefaultHasher::new();
+    let mut hasher = FxHasher::default();
     for &vkey in sorted_vkeys {
         vkey.hash(&mut hasher);
     }
-    hasher.finish()
+    std::hash::Hasher::finish(&hasher)
 }

Alternatively, if DefaultHasher is intentional for this use case (runtime-only matching within a single process), this can be left as-is.

src/core/triangulation.rs (1)

615-690: Vertex removal with fan triangulation is well-structured.

The implementation handles edge cases appropriately (vertex not found, no incident cells, empty boundary) with proper fallbacks. The order of operations correctly removes cells before the vertex itself.

Consider adding a TDS validation step after fan triangulation completes to catch potential topology issues early (similar to fix_invalid_facet_sharing calls in other paths).

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b58653 and f5b54c3.

📒 Files selected for processing (10)

• .github/workflows/benchmarks.yml (0 hunks)
• benches/assign_neighbors_performance.rs (11 hunks)
• docs/property_testing_summary.md (7 hunks)
• scripts/benchmark_utils.py (5 hunks)
• scripts/tests/test_benchmark_utils.py (3 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)
• tests/README.md (1 hunks)
• tests/delaunay_incremental_insertion.rs (1 hunks)
• tests/proptest_delaunay_triangulation.rs (1 hunks)

💤 Files with no reviewable changes (1)

• .github/workflows/benchmarks.yml

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/delaunay_incremental_insertion.rs
• scripts/benchmark_utils.py

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• tests/proptest_delaunay_triangulation.rs
• benches/assign_neighbors_performance.rs
• docs/property_testing_summary.md
• tests/README.md
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• tests/proptest_delaunay_triangulation.rs
• docs/property_testing_summary.md
• tests/README.md
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• tests/proptest_delaunay_triangulation.rs
• docs/property_testing_summary.md
• tests/README.md
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs

🧬 Code graph analysis (3)

benches/assign_neighbors_performance.rs (1)

src/core/delaunay_triangulation.rs (5)

• vertices (392-394)
• new (97-99)
• tds (421-423)
• number_of_cells (301-303)
• number_of_vertices (275-277)

src/core/algorithms/incremental_insertion.rs (5)

src/core/facet.rs (12)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• key (580-602)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)

src/core/cell.rs (7)

• cell (3224-3226)
• uuid (710-712)
• new (389-420)
• number_of_vertices (682-684)
• neighbors (561-563)
• vertices (593-595)
• map (323-323)

src/core/triangulation.rs (4)

• boundary_facets (273-281)
• number_of_vertices (159-161)
• cells (111-113)
• vertices (138-140)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

scripts/tests/test_benchmark_utils.py (1)

scripts/benchmark_utils.py (1)

• _write_error_file (1811-1823)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Code Coverage
• GitHub Check: Codacy Security Scan
• GitHub Check: performance-regression
• GitHub Check: build (macos-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (windows-latest)
• GitHub Check: Codacy Static Code Analysis

🔇 Additional comments (23)

scripts/tests/test_benchmark_utils.py (1)

576-615: LGTM!

These new tests provide good coverage for the _write_error_file method, verifying error content formatting, parent directory creation, and correlation with the implementation in benchmark_utils.py. The assertions correctly validate the expected error file structure.

tests/README.md (1)

60-137: Documentation accurately reflects the new architectural layers.

The updated property-based testing section correctly documents the three-layer architecture (Tds, Triangulation, DelaunayTriangulation) and the dimensional coverage (2D-5D). The status notes for ignored tests are clear and appropriately reference ongoing investigations.

src/core/algorithms/incremental_insertion.rs (4)

96-111: Partial mutation documentation is appropriately explicit.

The documentation clearly states the non-transactional behavior and provides guidance for callers who need rollback semantics. This is a reasonable design choice for performance.

---

210-339: Neighbor wiring algorithm is well-designed with appropriate safeguards.

The implementation correctly handles both interior cavity filling and hull extension scenarios. The non-manifold topology detection (lines 325-334) provides good defensive error handling.

---

519-536: Conservative coplanar handling is appropriate but may need future enhancement.

The documentation correctly notes that coplanar cases (orientation == 0) are treated as non-visible to avoid numerical instability. This may cause "no visible facets" errors for points nearly on the hull surface. The TODO-like comment about epsilon-based thresholds provides clear direction for future enhancement if needed.

---

538-648: Comprehensive dimension-generic tests for cavity filling.

The macro-generated tests provide excellent coverage across 2D-5D, validating both structural correctness (cell vertex counts) and TDS consistency (neighbor relationships).

src/core/triangulation.rs (4)

469-504: Stale VertexKey issue has been properly addressed.

The implementation now correctly captures the vertex UUID before TDS operations (line 470) and remaps the vertex key after rebuilding the TDS (lines 493-499). This ensures the returned VertexKey is always valid for the final TDS state.

---

793-812: Documentation accurately describes best-effort behavior.

The updated doc comments explicitly state this is a "best-effort repair mechanism" and clearly note that some internal failures are logged but don't cause error returns. This addresses the previous concern about doc/behavior mismatch.

---

727-788: Fan triangulation correctly avoids degenerate cells.

The implementation properly skips boundary facets that already contain the apex vertex (lines 757-760), preventing creation of cells with duplicate vertices. The error case when no cells are created is appropriately handled.

---

1011-1193: Comprehensive test coverage for initial simplex construction.

The tests cover all dimensions (2D-5D), error cases (insufficient/excess vertices), and user data preservation. The macro-based approach ensures consistent testing across dimensions.

tests/proptest_delaunay_triangulation.rs (5)

60-80: Bitwise deduplication is correct for exact duplicate detection.

The implementation uses to_bits() comparison to avoid float comparison lint issues while correctly detecting exact coordinate duplicates. The 'outer labeled loop pattern is appropriate here.

---

86-116: Incremental insertion validity tests are well-structured.

The tests correctly use prop_assume to skip cases where initial construction fails, focusing on validating that insertion maintains validity for successfully constructed triangulations.

---

167-231: Ignored tests are appropriately documented with investigation notes.

The ignored tests have clear status notes explaining the failures (Delaunay property violations, edge case failures) and indicate they need investigation. This is appropriate for tracking known issues during development.

---

237-325: Insertion-order robustness tests are well-designed with appropriate expectations.

The tests correctly acknowledge that Delaunay triangulation is not unique for degenerate/co-spherical point sets, focusing on validity invariants rather than exact topology comparison. The deterministic seed (0x00DE_C0DE) ensures reproducibility.

---

344-419: Duplicate cloud integration tests provide realistic messy-input scenarios.

The strategy generating exact duplicates and near-duplicates (1e-7 jitter) is a good test for the construction pipeline's robustness. The ignored status is consistent with other Delaunay property tests pending investigation.

docs/property_testing_summary.md (2)

76-127: Documentation accurately reflects the three-layer test architecture.

The updated documentation correctly describes the test organization by architectural layer (Tds, Triangulation, DelaunayTriangulation) with accurate test counts and status notes for ignored tests.

---

191-210: Test statistics provide clear summary by architectural layer.

The test count breakdown by layer (Tds: 32, Triangulation: 36, DelaunayTriangulation: 17) and execution time estimates are helpful for understanding the test suite scope.

benches/assign_neighbors_performance.rs (6)

5-9: LGTM!

Documentation clearly notes the migration, and the FastKernel import is correctly added to support the new DelaunayTriangulation API pattern.

---

75-89: LGTM!

The setup correctly creates a DelaunayTriangulation, clones the underlying Tds for mutation, and clears neighbors before measurement. The iter_with_setup pattern properly excludes triangulation construction from the benchmark timing.

---

112-115: LGTM!

Metrics are correctly accessed via DelaunayTriangulation methods (number_of_cells(), number_of_vertices()) for pre-benchmark analysis, consistent with the new API pattern.

---

185-189: LGTM!

The chained .new(&vertices).unwrap().tds().clone() pattern is appropriate here since the DelaunayTriangulation reference isn't needed after obtaining the Tds clone.

---

247-248: Lint suppression is appropriate.

The #[allow(clippy::too_many_lines)] is justified here—the function explicitly benchmarks 4 dimensions (2D–5D) with clear separation. Extracting these into separate functions would reduce benchmark cohesion without meaningful benefit.

---

266-270: LGTM!

All per-dimension benchmark blocks consistently apply the new DelaunayTriangulation::new().unwrap().tds().clone() pattern. The explicit dimension-specific type parameters (2, 3, 4, 5) are correctly specified.

Also applies to: 294-298, 322-326, 350-354

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

scripts/benchmark_utils.py (1)

1453-1514: Comparison errors are now logged to a file, but summary still reports “no regressions”

The new _write_error_file helper and its use in compare_with_baseline are a good improvement:

• Baseline missing → error file with "Baseline file not found".
• No current results → error file with "No benchmark results found".
• Timeout → error file with "Benchmark execution timeout".
• CalledProcessError / generic exceptions → error file with "Benchmark execution error" and details.

This guarantees benches/compare_results.txt exists with useful diagnostics and is well-covered by the new tests.

However, BenchmarkRegressionHelper.generate_summary currently treats any existing compare_results.txt that does not contain the string "REGRESSION" as a clean run:

if "REGRESSION" in content:
    print("Result: ⚠️ Performance regressions detected")
    ...
else:
    print("Result: ✅ No significant performance regressions")

With the new behavior, all error cases now produce a comparison file that contains ❌ Error: ... but no "REGRESSION". That means the summary will state “✅ No significant performance regressions” even when the comparison never ran successfully (e.g., timeout or cargo failure). The CLI exits with a non‑zero code in those cases, but the textual summary becomes misleading.

Consider explicitly recognizing error files in generate_summary and reporting a failure result instead. For example:

-            if results_file.exists():
-                with results_file.open("r", encoding="utf-8") as f:
-                    content = f.read()
-                    if "REGRESSION" in content:
-                        print("Result: ⚠️ Performance regressions detected")
-                        os.environ["BENCHMARK_REGRESSION_DETECTED"] = "true"
-                        BenchmarkRegressionHelper.write_github_env_vars({"BENCHMARK_REGRESSION_DETECTED": "true"})
-                        print("   Exported BENCHMARK_REGRESSION_DETECTED=true for downstream CI steps")
-                    else:
-                        print("Result: ✅ No significant performance regressions")
+            if results_file.exists():
+                with results_file.open("r", encoding="utf-8") as f:
+                    content = f.read()
+                    if "❌ Error:" in content:
+                        print("Result: ❌ Benchmark comparison failed (see benches/compare_results.txt for details)")
+                    elif "REGRESSION" in content:
+                        print("Result: ⚠️ Performance regressions detected")
+                        os.environ["BENCHMARK_REGRESSION_DETECTED"] = "true"
+                        BenchmarkRegressionHelper.write_github_env_vars({"BENCHMARK_REGRESSION_DETECTED": "true"})
+                        print("   Exported BENCHMARK_REGRESSION_DETECTED=true for downstream CI steps")
+                    else:
+                        print("Result: ✅ No significant performance regressions")

This keeps the new error file behavior while ensuring the high-level summary reflects that the comparison failed instead of implying a clean run.

Also applies to: 1839-1851, 2331-2377

🧹 Nitpick comments (3)

src/core/triangulation.rs (1)

1071-1263: Clarify return semantics when repair iterations exhaust without fully resolving violations.

The implementation may return Ok(total_removed) even if validate_facet_sharing() still fails after MAX_FIX_FACET_ITERATIONS (lines 1085-1260). The docs note this is "best-effort" (line 1052-1054) and mention that some internal failures are logged but don't cause errors (line 1067-1069), but they don't explicitly state the method can return Ok(...) while facet sharing violations remain unresolved.

Consider either:

• Adding a final validate_facet_sharing() check after the loop and returning an error if violations persist, or
• Amending the doc comment to explicitly state: "May return Ok(n) even if some facet sharing violations remain after reaching the iteration limit or when repair stalls."

This would eliminate ambiguity for callers who need to know whether Ok(...) guarantees a valid triangulation.

src/core/collections.rs (2)

323-347: Use FacetIndex alias instead of raw u8 in FacetIssuesMap

The new FacetIssuesMap looks good and matches the localized facet validation use case, but you’re using a bare u8 for the facet index even though FacetIndex is already defined as a dedicated alias.

Switching to FacetIndex would tighten the API and keep facet index semantics consistent with the rest of the module.

-/// - **Value**: `SmallBuffer<(CellKey, u8), 4>` - handles up to 4 over-sharing cells on stack
+/// - **Value**: `SmallBuffer<(CellKey, FacetIndex), 4>` - handles up to 4 over-sharing cells on stack
@@
-pub type FacetIssuesMap = FastHashMap<u64, SmallBuffer<(CellKey, u8), 4>>;
+pub type FacetIssuesMap = FastHashMap<u64, SmallBuffer<(CellKey, FacetIndex), 4>>;

---

1261-1264: Tests cover basic construction of secondary maps; consider minimal roundtrip

Instantiating CellSecondaryMap and VertexSecondaryMap in tests ensures the aliases are wired correctly. If you want a bit more safety, you could later add a tiny roundtrip (insert/get using real keys) to guard against accidental key-type mismatches, but it’s not strictly necessary for this PR.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5b54c3 and 35558fd.

📒 Files selected for processing (8)

• Cargo.toml (2 hunks)
• justfile (5 hunks)
• scripts/benchmark_utils.py (6 hunks)
• scripts/tests/test_benchmark_utils.py (3 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/collections.rs (6 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• justfile
• Cargo.toml

🧰 Additional context used

📓 Path-based instructions (1)

scripts/*.py

⚙️ CodeRabbit configuration file

scripts/*.py: These are Python utility scripts for changelog and commit processing.
Focus on code quality, maintainability, and adherence to Python best practices.
The scripts use ruff for comprehensive linting and formatting (pylint has been retired).

Files:

• scripts/benchmark_utils.py

🧠 Learnings (5)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• src/core/algorithms/incremental_insertion.rs
• src/core/delaunay_triangulation.rs
• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/algorithms/incremental_insertion.rs
• src/core/delaunay_triangulation.rs
• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/algorithms/incremental_insertion.rs
• src/core/delaunay_triangulation.rs
• src/core/triangulation.rs

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• scripts/benchmark_utils.py

🧬 Code graph analysis (4)

src/core/algorithms/incremental_insertion.rs (4)

src/core/algorithms/locate.rs (1)

• locate (183-256)

src/core/triangulation.rs (4)

• boundary_facets (274-282)
• number_of_vertices (160-162)
• cells (112-114)
• vertices (139-141)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

src/core/boundary.rs (1)

• boundary_facets (71-79)

scripts/benchmark_utils.py (1)

scripts/subprocess_utils.py (1)

• run_cargo_command (109-137)

src/core/delaunay_triangulation.rs (3)

src/geometry/util.rs (13)

• core (1809-1809)
• std (251-251)
• std (291-291)
• std (512-512)
• std (520-520)
• std (989-989)
• std (1020-1020)
• std (1060-1060)
• std (1263-1263)
• std (1304-1304)
• std (2411-2411)
• std (2458-2458)
• std (5190-5190)

src/core/algorithms/locate.rs (1)

• locate (183-256)

src/core/triangulation.rs (7)

• vertices (139-141)
• build_initial_simplex (335-379)
• number_of_vertices (160-162)
• number_of_cells (181-183)
• dim (208-210)
• cells (112-114)
• insert (456-586)

scripts/tests/test_benchmark_utils.py (1)

scripts/benchmark_utils.py (1)

• _write_error_file (1839-1851)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Code Coverage
• GitHub Check: performance-regression
• GitHub Check: Codacy Security Scan
• GitHub Check: build (windows-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: build (ubuntu-latest)

🔇 Additional comments (8)

src/core/collections.rs (5)

105-107: SparseSecondaryMap import matches new auxiliary map aliases

Importing SparseSecondaryMap here is appropriate for the new CellSecondaryMap and VertexSecondaryMap type aliases; no issues spotted.

---

747-757: UUID→vertex-key doctest correctly updated to new triangulation API

The example now using delaunay::prelude::*, DelaunayTriangulation::new, and dt.tds() aligns with the new public API and keeps reverse lookup guidance accurate; looks good.

---

778-787: UUID→cell-key doctest matches new DelaunayTriangulation entry point

Similarly, this doctest now reflects the unified DelaunayTriangulation::new + tds() access pattern, which should help keep docs and real usage in sync.

---

935-979: CellSecondaryMap alias and docs are a solid fit for cell-scoped aux data

Defining CellSecondaryMap<V> = SparseSecondaryMap<CellKey, V> with the accompanying docs and example (in_conflict: CellSecondaryMap<bool>) cleanly codifies how to track per-cell algorithm state without another hashmap; API shape and usage look good.

---

980-1020: VertexSecondaryMap mirrors cell variant and completes aux-data story

The VertexSecondaryMap<V> = SparseSecondaryMap<VertexKey, V> alias plus the processing-order example nicely mirror the cell-side API and give a clear pattern for vertex-scoped scratch data; no issues from a design or correctness standpoint.

scripts/tests/test_benchmark_utils.py (2)

364-398: Cargo invocation test correctly enforces removal of --quiet

The parametrized test_compare_omits_quiet_flag cleanly asserts that run_cargo_command is invoked without --quiet and that DEV_MODE_BENCH_ARGS are present only in dev mode, matching the updated PerformanceComparator.compare_with_baseline implementation. The use of capture_output=True in the assertions also keeps behavior aligned with the production code.

---

576-629: Error-file tests comprehensively cover content and failure behavior

The new _write_error_file tests validate:

• Happy path content (header, title, details, and guidance text).
• Handling of both Path and string details.
• Parent directory auto-creation.
• Graceful handling of Path.open failures without leaking exceptions.

These are well-structured, filesystem-scoped tests that mirror the implementation in PerformanceComparator._write_error_file and give good confidence in CI-facing error reporting behavior.

scripts/benchmark_utils.py (1)

1320-1388: BaselineGenerator cargo error handling and verbosity look solid

Removing --quiet from the cargo bench invocations and adding explicit subprocess.CalledProcessError handling (printing exit code plus captured stdout/stderr and logging the exception) makes baseline generation failures much easier to debug without changing success semantics. Timeout handling and the overall boolean contract of generate_baseline remain intact.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

scripts/slurm_storage_comparison.sh (1)

306-312: cargo clean between phases wipes the first phase’s Criterion baselines

Because both phases share the same CARGO_TARGET_DIR, calling cargo clean before DenseSlotMap benchmarks deletes the SlotMap criterion/ data that was just produced. That means:

• critcmp slotmap denseslotmap (and the report hints) won’t actually see both baselines.
• The archived criterion directory and baselines in the tarball only contain the second phase’s results.

To preserve the comparison:

• Either drop the second cargo clean, or
• Use separate CARGO_TARGET_DIRs per phase (and aggregate both into the archive), or
• Copy the first run’s criterion/ somewhere under artifacts/ before cleaning.

Right now the core “compare SlotMap vs DenseSlotMap” promise is only partially met.

Also applies to: 349-378, 393-401, 408-425

🧹 Nitpick comments (3)

scripts/benchmark_utils.py (1)

1453-1514: Structured comparison error output is a good improvement; consider richer timeout details

The new _write_error_file helper plus its use in compare_with_baseline gives CI a consistent compare_results.txt with a clear ❌ Error: header, and the CalledProcessError handler now prints both stderr and stdout, which is very helpful.

One minor DX improvement: for the timeout path you currently record only "Timeout after {bench_timeout} seconds" in the error file. Including the exception message as well would preserve more context (e.g., command args) in the artifact, similar to the generic Exception and CalledProcessError paths.

For example:

-        except subprocess.TimeoutExpired as e:
+        except subprocess.TimeoutExpired as e:
@@
-            self._write_error_file(output_file, "Benchmark execution timeout", f"Timeout after {bench_timeout} seconds")
+            self._write_error_file(
+                output_file,
+                "Benchmark execution timeout",
+                f"{e} (timeout after {bench_timeout} seconds)",
+            )

This keeps console logs and artifacts aligned without changing control flow.

Also applies to: 1839-1851

scripts/slurm_storage_comparison.sh (1)

208-215: Consider validating timeout availability up front

The script relies on timeout "$PHASE_TIMEOUT" ... in both phases, but unlike cargo/uv it isn’t checked. On systems without GNU coreutils’ timeout, the job will fail late with a less obvious “command not found” after doing setup work.

You could mirror the other dependency checks near the top of execution mode:

if ! command -v timeout &>/dev/null; then
  echo "❌ Error: timeout not found (GNU coreutils). Please install it on this node." >&2
  exit 1
fi

This makes failures deterministic and easier to debug.

Also applies to: 286-293, 322-329

src/core/triangulation.rs (1)

470-505: Stale-key fix in the D+1 branch is good; consider filtering the returned hint after repairs

Capturing the vertex UUID before insertion and re-deriving v_key from the rebuilt TDS in the num_vertices == D + 1 branch fixes the earlier stale-VertexKey issue. The overall insert flow (bootstrap → initial simplex → locate + cavity/hull + local facet repair) looks coherent.

One subtle edge case: both interior and exterior paths:

let new_cells = /* ... */;
if let Some(issues) = self.detect_local_facet_issues(&new_cells)? {
    let removed = self.repair_local_facet_issues(&issues)?;
}
Ok((v_key, new_cells.first().copied()))

return a hint derived from new_cells even though repair_local_facet_issues may have removed some of those cells. If that first key was removed, later locate calls that reuse the hint may see an invalid cell key.

It’s probably only a performance/regional-robustness concern (depending on how locate handles a dead hint), but you could tighten it by filtering against the current TDS:

let hint_cell = new_cells
    .iter()
    .copied()
    .find(|ck| self.tds.contains_cell(*ck));
Ok((v_key, hint_cell))

(and apply the same pattern in both branches).

Also applies to: 539-575

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35558fd and 25ef314.

📒 Files selected for processing (8)

• cspell.json (4 hunks)
• justfile (5 hunks)
• scripts/benchmark_utils.py (7 hunks)
• scripts/slurm_storage_comparison.sh (5 hunks)
• scripts/tests/test_benchmark_utils.py (4 hunks)
• src/core/collections.rs (6 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• scripts/tests/test_benchmark_utils.py
• justfile
• src/core/delaunay_triangulation.rs
• cspell.json

🧰 Additional context used

📓 Path-based instructions (1)

scripts/*.py

⚙️ CodeRabbit configuration file

scripts/*.py: These are Python utility scripts for changelog and commit processing.
Focus on code quality, maintainability, and adherence to Python best practices.
The scripts use ruff for comprehensive linting and formatting (pylint has been retired).

Files:

• scripts/benchmark_utils.py

🧠 Learnings (5)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/triangulation.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• src/core/triangulation.rs

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• scripts/benchmark_utils.py

🧬 Code graph analysis (2)

src/core/triangulation.rs (2)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

scripts/benchmark_utils.py (1)

scripts/subprocess_utils.py (1)

• run_cargo_command (109-137)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Code Coverage
• GitHub Check: build (windows-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: Codacy Security Scan

🔇 Additional comments (7)

scripts/benchmark_utils.py (2)

1343-1388: Baseline generation cargo error handling is solid

Special‑casing TimeoutExpired and CalledProcessError while logging and returning False makes the CLI behavior predictable and surfaces cargo failures clearly, especially with capture_output=True for richer diagnostics. This looks good as‑is.

---

2357-2359: Regression summary correctly distinguishes infrastructure errors from regressions

Updating the summary to check for \"❌ Error:\" before \"REGRESSION\" aligns it with the new error‑file format and cleanly separates benchmark/infra failures from genuine performance regressions in CI output.

src/core/collections.rs (1)

323-347: Facet issue map and secondary-slotmap aliases are well-aligned with new usage

The FacetIssuesMap layout matches how detect_local_facet_issues builds and consumes over‑shared facet data, and the CellSecondaryMap / VertexSecondaryMap aliases give a clear, type-safe way to hang auxiliary state off CellKey / VertexKey. The instantiation tests ensure these types stay wired correctly without adding behavioral surface area. No changes needed.

Also applies to: 935-1020, 1261-1263

src/core/triangulation.rs (4)

61-283: Core Triangulation accessors and facet iterators cleanly wrap Tds

new_empty, the cell/vertex iterators, counters, and dim() all delegate directly to Tds, and the facets() / boundary_facets() APIs provide a clear, iterator-based surface while keeping the facet map internal. The expect in boundary_facets() matches the docs’ “this indicates a bug in the library” stance. This layer looks solid and minimal.

---

296-379: Initial simplex construction is consistent and well-guarded

build_initial_simplex correctly enforces “exactly D+1 vertices”, rebuilds a fresh Tds, inserts vertices with UUID mapping, constructs a single cell, and assigns incident cells before returning. The error wrapping via TriangulationConstructionError makes failures explicit and the tests across 2D–5D plus edge cases exercise the main paths. No changes needed here.

---

867-930: Localized facet-issue detection matches the intended O(k·D) behavior

detect_local_facet_issues builds a facet → (CellKey, FacetIndex) map only over the provided cells, hashes facets from sorted vertex keys, and promotes entries with len() > 2 into a separate FacetIssuesMap. The explicit u8 facet-index check ensures FacetIndex stays valid for the configured dimension range. This is exactly the localized diagnostic you want on top of the global validator.

---

1050-1087: Deprecation and documentation of fix_invalid_facet_sharing align with its best-effort nature

Marking fix_invalid_facet_sharing as deprecated and explicitly documenting it as a global O(N·D) best-effort repair (with an iteration cap and no guarantee of fully resolving violations) matches how the implementation downgrades internal failures to warnings. The guidance to prefer detect_local_facet_issues + repair_local_facet_issues for incremental operations is clear. No further changes needed here.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (6)

scripts/tests/test_benchmark_utils.py (1)

576-628: New _write_error_file tests give solid coverage of success and failure modes

The four tests around _write_error_file (baseline-not-found, benchmark error, parent-dir creation, and write failure) thoroughly exercise the helper, including the failure path where Path.open is patched to raise OSError. Assertions check for the key strings that downstream logic depends on (❌ Error: …, Details: …, and the explanatory footer), which should keep CI summaries robust if the template changes slightly.

One minor consideration: patching Path.open at the class level affects all Path opens in the context; it’s safe here but if you expand tests later, you may want to narrow the patch to output_file.open via patch.object(output_file, "open", ...) to avoid accidental interference.

scripts/benchmark_utils.py (1)

1343-1355: Baseline generation: cargo invocation and error handling look correct but don’t emit structured error files

Removing --quiet and always passing capture_output=True for run_cargo_command in generate_baseline is consistent with the intent to improve diagnostics, and the new subprocess.CalledProcessError branch that dumps stderr/stdout plus a logged exception is reasonable. The function still returns False on timeouts, cargo failures, or generic exceptions without touching comparison result files, which matches its role (producing baselines rather than comparison output).

If you later want symmetry with compare_with_baseline, consider optionally emitting a small error artifact alongside baseline generation (e.g., baseline-artifact/baseline_error.txt), but that’s not required for the current CI flow.

Also applies to: 1368-1388

src/core/algorithms/incremental_insertion.rs (1)

583-689: Clarify visibility semantics and error mapping in find_visible_boundary_facets

The visibility test treats coplanar facets (orientation == 0) as non‑visible and maps all failures (boundary lookup, vertex lookup, orientation errors) into InsertionError::HullExtension. That’s conservative but may surprise users debugging outer‑hull issues.

Consider tightening the docs here to explicitly state:

• Coplanar facets are intentionally excluded from hull extension; nearly coplanar queries may yield “No visible boundary facets found …”.
• Any structural/lookup/orientation failure during boundary processing will surface as HullExtension { message }.

No code change required, but a short “# Panics / # Errors” clarification in the doc block will make debugging hull‑extension failures easier.

src/core/triangulation.rs (2)

456-680: insert logic is sound but long; clean up unused iteration and consider splitting flows

The unified insert handles:

• Bootstrap (< D+1 vertices) and initial simplex (== D+1) via build_initial_simplex and UUID remapping.
• Interior insertion using fill_cavity + wire_cavity_neighbors + conflict removal + localized facet repair + repair_neighbor_pointers + assign_incident_cells.
• Hull extension via extend_hull plus the same localized repair pipeline.

Two small points:

1. The for iteration in 0..MAX_REPAIR_ITERATIONS loops (interior and hull paths) only use iteration in #[cfg(debug_assertions)] logging, which triggers an unused‑variable lint in non‑debug builds. You can keep the logging but satisfy the lint with:

-                for iteration in 0..MAX_REPAIR_ITERATIONS {
+                for iteration in 0..MAX_REPAIR_ITERATIONS {
+                    let _ = iteration; // used only in debug logging

2. At 160+ lines, this method is hard to scan. Consider (later, not blocking) extracting the interior and hull‑extension repair loops into private helpers like repair_after_interior_insertion and repair_after_hull_extension to reduce cognitive load and keep the main control flow clearer.

---

1060-1125: repair_local_facet_issues docs still suggest topology rebuild

The implementation selects cells to remove based on radius_ratio (with UUID fallback) and then just calls remove_cells_by_keys, returning Ok(removed_count). It does not rebuild neighbors or incidence here; that’s left to callers.

The doc comment currently says:

“Returns error if topology rebuild fails after cell removal.”

but no such rebuild or error path exists in this function anymore; topology repair happens at the call sites (e.g., via repair_neighbor_pointers and assign_incident_cells).

Recommend softening the docs to reflect actual behavior, e.g.:

• Clarify that this is a localized removal helper and does not rebuild neighbors/incidence.
• Note that it only returns Err(...) if quality computation or TDS access fails, not for “topology rebuild failures”.

No code change required, just doc alignment.

src/core/delaunay_triangulation.rs (1)

992-1035: validate_delaunay error mapping is helpful; optional clippy clean‑up

validate_delaunay wraps core::util::is_delaunay and enriches errors with cell UUIDs where possible, which is very helpful for debugging. The use of format! with captured variables (e.g., {cell_uuid}, {cell_key:?}, {source}) already follows modern idioms.

If clippy is still complaining about “variables can be used directly in the format! string” here, you can ignore it or, if desired, factor repeated strings into helper functions—there’s no functional issue.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25ef314 and 00a9f51.

📒 Files selected for processing (9)

• Cargo.toml (1 hunks)
• justfile (3 hunks)
• scripts/benchmark_utils.py (7 hunks)
• scripts/slurm_storage_comparison.sh (9 hunks)
• scripts/tests/test_benchmark_utils.py (5 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)
• tests/delaunay_edge_cases.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• Cargo.toml

🧰 Additional context used

📓 Path-based instructions (1)

scripts/*.py

⚙️ CodeRabbit configuration file

scripts/*.py: These are Python utility scripts for changelog and commit processing.
Focus on code quality, maintainability, and adherence to Python best practices.
The scripts use ruff for comprehensive linting and formatting (pylint has been retired).

Files:

• scripts/benchmark_utils.py

🧠 Learnings (5)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• tests/delaunay_edge_cases.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• tests/delaunay_edge_cases.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• tests/delaunay_edge_cases.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• justfile
• scripts/benchmark_utils.py

🧬 Code graph analysis (3)

scripts/tests/test_benchmark_utils.py (2)

scripts/benchmark_utils.py (4)

• _write_error_file (1839-1851)
• BenchmarkRegressionHelper (1995-2378)
• generate_summary (118-157)
• generate_summary (2331-2378)

scripts/tests/conftest.py (1)

• temp_chdir (23-51)

src/core/algorithms/incremental_insertion.rs (6)

src/core/facet.rs (12)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• key (580-602)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)

src/core/cell.rs (6)

• cell (3224-3226)
• uuid (710-712)
• new (389-420)
• number_of_vertices (682-684)
• vertices (593-595)
• map (323-323)

src/core/triangulation.rs (4)

• boundary_facets (274-282)
• number_of_vertices (160-162)
• cells (112-114)
• vertices (139-141)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/triangulation_data_structure.rs (5)

• number_of_vertices (889-891)
• cells (729-731)
• cells (3186-3195)
• vertices (796-798)
• get_cell (824-826)

scripts/benchmark_utils.py (1)

scripts/subprocess_utils.py (1)

• run_cargo_command (109-137)

🪛 GitHub Actions: Performance Regression Testing

src/core/algorithms/incremental_insertion.rs

[error] 324-324: Cargo bench failed with exit code 101. Error: unused variable: facet_key.

🪛 GitHub Check: build (macos-latest)

src/core/algorithms/incremental_insertion.rs

[failure] 532-532:
variables can be used directly in the format! string

---

[failure] 417-417:
docs for function which may panic missing # Panics section

src/core/triangulation.rs

[failure] 620-620:
adding items after statements is confusing, since items exist from the start of the scope

---

[failure] 541-541:
adding items after statements is confusing, since items exist from the start of the scope

---

[failure] 456-456:
this function has too many lines (164/100)

🪛 GitHub Check: build (ubuntu-latest)

src/core/algorithms/incremental_insertion.rs

[failure] 532-532:
variables can be used directly in the format! string

---

[failure] 417-417:
docs for function which may panic missing # Panics section

src/core/triangulation.rs

[failure] 620-620:
adding items after statements is confusing, since items exist from the start of the scope

---

[failure] 541-541:
adding items after statements is confusing, since items exist from the start of the scope

---

[failure] 456-456:
this function has too many lines (164/100)

🪛 GitHub Check: performance-regression

src/core/algorithms/incremental_insertion.rs

[failure] 431-431:
variable total_repaired is assigned to, but never used

---

[failure] 324-324:
unused variable: facet_key

src/core/triangulation.rs

[failure] 614-614:
unused variable: iteration

---

[failure] 543-543:
unused variable: iteration

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Code Coverage
• GitHub Check: Codacy Security Scan
• GitHub Check: build (windows-latest)

🔇 Additional comments (26)

justfile (3)

133-136: Coverage configuration simplified and clarified.

The migration to --workspace --lib --tests provides cleaner coverage targeting. The explicit exclusion of storage_backend_compatibility with its accompanying explanation (Phase 4 evaluation tests with ignored status) ensures future maintainers understand the rationale.

---

163-163: Help text accurately reflects CI pipeline expansion.

The documentation correctly describes the enhanced CI target that now includes integration tests.

---

99-101: CI target comment is misleading regarding workflow alignment.

The justfile comment claims the CI target "matches .github/workflows/ci.yml", but the workflow file does not actually run bench-compile. On Linux/macOS, the workflow delegates to just ci, while on Windows it only runs cargo build, cargo test --lib, cargo test --doc, and cargo test --tests—excluding the benchmark compilation step. Update the comment to accurately describe what the justfile target does rather than claiming equivalence with the workflow.

Likely an incorrect or invalid review comment.

scripts/slurm_storage_comparison.sh (7)

40-54: Past issue is fixed: --large and --time are now independent.

The argument parsing correctly separates concerns: --large only sets LARGE_SCALE (line 49), and --time=* explicitly sets TIME_LIMIT (line 53). The help text (lines 66–67) clearly documents this. The example from the past review now works as expected: --time=7-00:00:00 --large will use a 7-day limit, not override to 14 days.

---

184-188: Good defensive verification for timeout command.

The new check for GNU coreutils' timeout (lines 184–188) is necessary since the script relies on it for per-phase timeout enforcement (lines 293, 342). This prevents silent failures if timeout is missing on the execution node.

---

293-293: Per-phase timeout enforcement and exit handling look correct.

Both phases (SlotMap at line 293, DenseSlotMap at line 342) properly use timeout to enforce per-phase limits, save distinct baselines (--save-baseline slotmap vs. --save-baseline denseslotmap), and differentiate timeout (exit 124) from other failures (lines 297–305, 346–354). The feature flag dense-slotmap is correctly applied only to Phase 2.

Also applies to: 342-342

---

313-325: Baseline backup/restore mechanism is well-designed.

The strategy to preserve both backends' Criterion baselines is clever:

1. After Phase 1, back up SlotMap results to artifacts/slotmap-baselines-${SLURM_JOB_ID} (lines 314–320).
2. After cargo clean wipes Criterion, Phase 2 runs and populates criterion/ with DenseSlotMap results.
3. On restore (lines 431–442), copy SlotMap backup back using cp -rn (no-clobber) to preserve DenseSlotMap data.

This ensures both backends' baselines end up in the final criterion/ directory for comparison. The -n flag (line 438) prevents overwriting Phase 2 results—good defensive coding.

Verify that the final criterion/ directory after archival (line 447) contains both slotmap and denseslotmap subdirectories by checking a completed job's artifacts.

Also applies to: 431-442

---

362-423: Reporting and archival flow is sound, with good error tolerance.

The reporting section generates a Markdown report (lines 374–409) with job metadata and comparison instructions, and attempts critcmp for detailed output (lines 415–422) with graceful fallback. Archival correctly restores SlotMap baselines, copies merged Criterion results, includes the report, and creates a tarball (lines 426–463). Soft error handling (warnings rather than exits) allows the script to complete even if some steps fail.

Also applies to: 425-463

---

102-129: Job submission is well-guarded but relies on sbatch output parsing.

The submission logic verifies sbatch is available (line 102) and extracts the job ID using regex (line 119): grep -oP '\d+$'. While this should work for typical Slurm output ("Submitted batch job 12345"), it assumes the output format is stable. The non-empty check on JOB_ID (line 121) provides a safety net, but consider adding explicit validation or using --parsable flag if available to make parsing more robust across Slurm versions.

Verify that sbatch ... | grep -oP '\d+$' reliably extracts the job ID on your Slurm version(s). If needed, test with sbatch --parsable for machine-readable output.

---

215-254: Review dynamic per-phase timeout calculation—concerns are valid but already mitigated.

The timeout logic (lines 215–254) correctly parses Slurm time formats and handles edge cases, though three design points merit awareness:

1. Integer division truncation: PHASE_TIMEOUT_HOURS=$((AVAILABLE_HOURS / 2)) discards remainders (e.g., 71h available → 34h per phase, losing 1h). This is acceptable and expected for integer arithmetic, but means jobs won't always use their full allocation.

2. Very short job time limits: Jobs with <2h total allocate only 1h per phase after the 2h buffer, which may prevent phase completion. The code already detects this at line 248–250 and warns the user: "Time limit too short ($TIME_LIMIT_STR), phases may not complete". This warning is adequate.

3. scontrol availability: Line 217 relies on scontrol show job $SLURM_JOB_ID. The fallback to "UNLIMITED" (336h per phase, line 219) is conservative and safe. Behavior is well-defined across common Slurm configurations.

No changes needed. The code is robust for typical use cases (default 3-day limit) and appropriately warns users of edge cases.

scripts/tests/test_benchmark_utils.py (3)

365-398: Cargo invocation test correctly enforces removal of --quiet and dev-mode arg usage

The updated test_compare_omits_quiet_flag accurately asserts that run_cargo_command is called without --quiet, still checks for DEV_MODE_BENCH_ARGS in dev mode, and verifies capture_output=True, which matches the new compare_with_baseline behavior. This gives good regression coverage on the CLI contract without overfitting to the exact argument order.

---

1612-1647: Summary behavior on error files is correctly specified and asserted

test_generate_summary_with_error_file cleanly validates the new contract where BenchmarkRegressionHelper.generate_summary treats comparison error files (identified by ❌ Error:) as a failed comparison and not as “no regressions”. The expectations around the printed summary lines are precise and ensure that future changes won’t accidentally regress to the old “no regressions” messaging when an error occurs.

---

1749-1777: Timeout handling test for PerformanceComparator matches new structured error output

The extended test_timeout_error_handling_performance_comparator now checks that a timeout not only prints the human-readable stderr guidance but also writes a structured error file whose contents include the standard error header, the underlying command (cargo bench), and the explicit timeout value. This tightly couples the test to the new _write_error_file contract in a good way and should catch regressions in timeout reporting.

scripts/benchmark_utils.py (3)

1454-1481: Comparison path now consistently emits structured error files for all failure modes

The updated compare_with_baseline:

• Writes an error file when the baseline file is missing before doing any work.
• Runs cargo bench without --quiet in both normal and dev modes while still capturing output.
• On TimeoutExpired, CalledProcessError, or any other Exception, logs diagnostics, prints human-readable errors, and writes a structured error file via _write_error_file.

This ensures that benches/compare_results.txt always exists (or is updated) on failure, which is exactly what the new tests and BenchmarkRegressionHelper.generate_summary depend on. The separation between timeout-specific messaging and generic “Benchmark execution error” is also clear and useful.

Also applies to: 1491-1514

---

1839-1851: _write_error_file helper is simple and robust for CI consumption

The new _write_error_file helper writes a minimal, structured “Comparison Results” document with a clear ❌ Error: header, a Details: line (accepting both str and Path), and explanatory footer text. The internal try/except that logs but swallows any exception makes sense here: if the filesystem is unwritable, we at least don’t crash the calling path, which is already logging its own failure.

If you ever need richer metadata (timestamps, commit ids) in error files, this would be the right central place to extend, but the current design is appropriate for the scope.

---

2357-2367: Regression summary now correctly distinguishes errors from regressions

The change in BenchmarkRegressionHelper.generate_summary to first check for "❌ Error:" before "REGRESSION" in compare_results.txt cleanly separates “comparison failed” from “regressions detected”. This aligns with the new _write_error_file format and prevents misleading “no significant performance regressions” messages when the comparison never actually ran to completion.

src/core/algorithms/incremental_insertion.rs (1)

691-801: Nice multi‑dimensional coverage for fill_cavity tests

The test_fill_cavity! macro and its 2D–5D instantiations give good confidence that cavity filling + neighbor wiring preserves invariants (tds.is_valid() and cell vertex counts). This is a solid regression harness around fill_cavity/wire_cavity_neighbors.

src/core/triangulation.rs (3)

335-379: build_initial_simplex contract and error mapping look good

The function enforces exactly D+1 vertices, cleanly maps wrong counts into TriangulationConstructionError::InsufficientVertices, and routes cell creation failure through FailedToCreateCell. Assigning incident cells via assign_incident_cells() before returning ensures the resulting Tds is structurally usable.

No changes needed here.

---

961-1024: Localized facet-issue detection helper is well‑structured

detect_local_facet_issues correctly restricts work to the provided cells, hashes sorted facet vertex keys, and returns Some(issues) only for over‑shared facets (>2 cells). It also gracefully skips cells that were already removed and turns facet-index overflow into a structured TriangulationValidationError.

This aligns well with the intended O(k·D) localized validation.

---

1169-1361: Deprecated global fix_invalid_facet_sharing behavior matches updated docs

The deprecated global repair now clearly documents itself as a best‑effort, iterative O(N·D) pass and:

• Uses quality/UUID selection to choose cells to remove.
• Integrates with repair_neighbor_pointers and assign_incident_cells() only once at the end when something was removed.
• Explicitly states that Ok(total_removed) is not a guarantee that all violations are gone.

This reconciles the implementation with the docstring; looks good for a deprecated API.

src/core/delaunay_triangulation.rs (5)

65-130: new / empty fast‑path constructors are consistent with bootstrap semantics

The new helper for FastKernel<f64>, (), () and the empty() constructor both correctly route through with_kernel / with_empty_kernel, and the examples reflect the actual bootstrap behavior: vertices accumulate with zero cells until the (D+1)‑th insertion triggers simplex creation.

The updated docs no longer over‑promise around incremental insertion from an empty triangulation—they match the implemented flow.

---

215-251: with_kernel initial-simplex + incremental path looks correct

with_kernel:

• Guards against vertices.len() < D + 1 with a clear InsufficientVertices error.
• Builds the initial simplex via Triangulation::build_initial_simplex(&vertices[..=D]).
• Wraps the resulting Tds with the provided kernel.
• Inserts remaining vertices by calling insert and mapping any failure into TriangulationConstructionError::FailedToAddVertex.

This is consistent with the intended “batch + incremental continuation” construction flow.

---

588-729: insert wiring to conflict-region and triangulation layer is sound, degenerate cases are clearly gated

For DelaunayTriangulation::insert:

• Bootstrap (num_vertices < D + 1) delegates fully to Triangulation::insert, which now handles initial simplex creation when the (D+1)‑th vertex is inserted (via UUID remap).
• For num_vertices >= D + 1, you:
  • Run locate on the incoming vertex before inserting it.
  • Use find_conflict_region only for LocateResult::InsideCell and pass that region down into Triangulation::insert.
  • Treat LocateResult::Outside as hull‑extension (no conflict region).
  • Explicitly reject OnFacet/OnEdge/OnVertex as unimplemented, with a clear InsertionError::CavityFilling message.

This separation of concerns (high‑level conflict computation in DelaunayTriangulation, low‑level cavity/hull work in Triangulation) looks correct. Once you decide to support degenerate locations, you can refine the TODO block into dedicated error variants without changing this control flow.

---

1166-1174: DelaunayCheckPolicy is clearly documented as experimental and unused

The DelaunayCheckPolicy enum is marked as experimental and the docs clearly state that it is not yet wired into insertion logic. That’s appropriate for an API placeholder and sets correct expectations for users.

No changes needed until you’re ready to integrate it.

---

1194-1346: Incremental-insertion tests give strong coverage across dimensions and flows

The test_incremental_insertion! macro and its 2D–5D instantiations exercise:

• Batch construction (new),
• Bootstrap from empty,
• Continuation into cavity‑based insertions, and
• Equivalence between bootstrap and batch results.

Combined with the empty() and with_kernel() tests, this is a very solid regression suite around the new insertion pipeline and bootstrap semantics.

tests/delaunay_edge_cases.rs (1)

472-493: Test assertion for collinear 2D points is correct—no code change needed.

The test correctly expects FailedToAddVertex for four collinear points in 2D. build_initial_simplex does not validate geometric non-degeneracy (per its docstring: "does not require the Delaunay property—it only uses basic topology"). The first three collinear points [(0,0), (1,0), (2,0)] successfully form a degenerate simplex cell. When the fourth point (3,0) is inserted via the normal insertion path, locate() encounters a degenerate location and returns InsertionError::CavityFilling, which is wrapped as TriangulationConstructionError::FailedToAddVertex in the with_kernel() loop. The error originates from the incremental insertion of the fourth vertex, not from build_initial_simplex.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

examples/convex_hull_3d_20_points.rs (1)

6-19: Docs still reference Bowyer–Watson and wrong example name.

The example description is now out of sync with the implementation: it no longer uses Bowyer–Watson and the command refers to convex_hull_3d_100_points while this file is convex_hull_3d_20_points. Suggest updating the docs to match the new incremental insertion backend and the actual example name.

-//! - Using the `generate_random_triangulation` utility function for convenience
-//! - Building a Delaunay triangulation using the Bowyer-Watson algorithm
+//! - Using the `generate_random_triangulation` utility function for convenience
+//! - Building a Delaunay triangulation using the incremental cavity-based insertion algorithm
@@
-//! ```bash
-//! cargo run --example convex_hull_3d_100_points
-//! ```
+//! ```bash
+//! cargo run --example convex_hull_3d_20_points
+//! ```

♻️ Duplicate comments (1)

src/core/triangulation.rs (1)

1371-1437: repair_local_facet_issues “Errors” docs over‑promise relative to implementation.

The “Errors” section says:

“Returns error if topology rebuild fails after cell removal.”

but this function only selects cells to remove and calls remove_cells_by_keys; it does not perform any neighbor/topology rebuild itself. The only errors it can currently return are from quality computation or the coordinate/validation helpers used along the way.

To avoid misleading callers, I’d adjust the docstring along these lines:

-/// # Errors
-///
-/// Returns error if topology rebuild fails after cell removal. This indicates
-/// the triangulation may be in an inconsistent state and should be validated.
+/// # Errors
+///
+/// Returns error if quality evaluation or facet bookkeeping fails while
+/// selecting cells to remove. This function itself does not rebuild neighbors;
+/// callers are responsible for repairing or validating topology after removal
+/// (e.g., via `repair_neighbor_pointers` or a validation pass).

🧹 Nitpick comments (5)

src/core/algorithms/incremental_insertion.rs (1)

608-608: Replace .unwrap() calls with error handling to avoid panics.

The repair_neighbor_pointers function uses .unwrap() on lines 608, 626, 669, and 670 when retrieving cells. While the # Panics section documents this behavior (lines 543-544), these could be replaced with proper error handling for consistency with the rest of the function, which already returns InsertionError.

-            let cell = tds.get_cell(cell_key).unwrap();
+            let cell = tds
+                .get_cell(cell_key)
+                .ok_or_else(|| InsertionError::NeighborWiring {
+                    message: format!("Cell {cell_key:?} not found during facet matching"),
+                })?;

Apply similar changes to lines 626, 669, and 670.

Also applies to: 626-626, 669-670

examples/convex_hull_3d_20_points.rs (1)

200-263: Drop the now‑unused _original_vertices argument.

test_point_containment no longer uses _original_vertices, and the caller always passes an empty slice. Simplifying the signature will reduce noise and make the example easier to follow.

-fn test_point_containment(
-    dt: &DelaunayTriangulation<FastKernel<f64>, (), (), 3>,
-    _original_vertices: &[()], // Not needed anymore since we access vertices from tds
-) {
+fn test_point_containment(
+    dt: &DelaunayTriangulation<FastKernel<f64>, (), (), 3>,
+) {
@@
-    // Test point containment
-    test_point_containment(&dt, &[]);
+    // Test point containment
+    test_point_containment(&dt);

src/core/triangulation.rs (3)

348-421: Initial simplex construction and degeneracy handling look correct (given stateless kernels).

build_initial_simplex enforces vertices.len() == D + 1, uses a kernel orientation check to reject degenerate simplices with a clear GeometricDegeneracy error message, and then builds a single‑cell Tds with proper vertex–cell mappings via assign_incident_cells(). This matches the crate‑level docs for TriangulationConstructionError::InsufficientVertices and ::GeometricDegeneracy.

One design assumption baked in here is that K::default() is an acceptable kernel instance for the orientation test. That’s consistent with the documentation that kernels are stateless and reconstructible, but if you ever introduce stateful kernels, you’ll likely want to thread an explicit kernel instance into this helper instead of relying on Default.

---

557-707: Retry classification in insert_transactional is string‑based and somewhat brittle.

The transactional insertion logic (clone TDS → try insert → rollback + perturb + retry on certain errors) is solid, but the is_retryable detection currently relies on substring checks of error messages ("Cycle detected", "Non-manifold", etc.) and to_string() on nested errors. That makes behavior dependent on wording and formatting of errors, which is fragile to future refactors.

Consider pushing this classification into the error types themselves (e.g., a helper like InsertionError::is_retryable() or finer‑grained variants for the “cycle”, “non‑manifold”, and specific conflict cases) so the retry logic stays robust even if messages change.

---

1041-1131: Verify neighbor invariants after remove_vertex when local facet repair removes additional cells.

remove_vertex correctly performs fan‑based cavity filling and wires neighbors via wire_cavity_neighbors before calling remove_cells_by_keys on the original incident cells. When detect_local_facet_issues(&new_cells) finds over‑shared facets, repair_local_facet_issues can then remove additional cells, but after that you only call assign_incident_cells() and do not run repair_neighbor_pointers or similar neighbor rebuilding.

If remove_cells_by_keys does not fully clear neighbor pointers from surviving cells, or if removing the extra cells upsets neighbor consistency, this could leave some neighbor links stale after a vertex removal that triggered repairs (even though tests pass for the “simple” cases).

Please double‑check:

• Whether Tds::remove_cells_by_keys guarantees neighbor pointers referencing removed cells are cleaned up, and
• Whether you want to mirror the insertion path by running repair_neighbor_pointers after any non‑zero removed from repair_local_facet_issues in remove_vertex, for symmetry and extra safety around topology invariants.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00a9f51 and 48d3cf7.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (13)

• Cargo.toml (1 hunks)
• benches/assign_neighbors_performance.rs (0 hunks)
• benches/microbenchmarks.rs (8 hunks)
• cspell.json (5 hunks)
• examples/convex_hull_3d_20_points.rs (18 hunks)
• examples/memory_analysis.rs (4 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/algorithms/locate.rs (1 hunks)
• src/core/delaunay_triangulation.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)
• src/geometry/kernel.rs (1 hunks)
• src/lib.rs (16 hunks)
• tests/check_perturbation_stats.rs (1 hunks)

💤 Files with no reviewable changes (1)

• benches/assign_neighbors_performance.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• examples/memory_analysis.rs
• cspell.json

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/algorithms/locate.rs
• src/geometry/kernel.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• benches/microbenchmarks.rs
• src/lib.rs
• examples/convex_hull_3d_20_points.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/algorithms/locate.rs
• src/geometry/kernel.rs
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs
• benches/microbenchmarks.rs
• src/lib.rs
• examples/convex_hull_3d_20_points.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• src/core/algorithms/locate.rs
• src/geometry/kernel.rs
• src/core/algorithms/incremental_insertion.rs
• tests/check_perturbation_stats.rs
• src/core/triangulation.rs
• benches/microbenchmarks.rs
• src/lib.rs
• examples/convex_hull_3d_20_points.rs
• src/core/delaunay_triangulation.rs

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• Cargo.toml
• src/lib.rs

🧬 Code graph analysis (8)

src/geometry/kernel.rs (4)

src/geometry/util.rs (1)

• core (1809-1809)

src/geometry/predicates.rs (2)

• insphere (349-424)
• simplex_orientation (111-159)

src/geometry/robust_predicates.rs (5)

• robust_insphere (149-192)
• robust_orientation (303-337)
• general_triangulation (736-746)
• num_traits (1418-1418)
• high_precision (762-773)

src/core/delaunay_triangulation.rs (1)

• new (96-98)

src/core/algorithms/incremental_insertion.rs (3)

src/core/triangulation.rs (2)

• boundary_facets (277-285)
• cells (115-117)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

tests/check_perturbation_stats.rs (4)

src/core/delaunay_triangulation.rs (3)

• empty (126-128)
• is_valid (786-791)
• number_of_vertices (275-277)

src/core/triangulation_data_structure.rs (4)

• empty (2273-2282)
• default (206-208)
• is_valid (2838-2852)
• number_of_vertices (894-896)

src/core/vertex.rs (1)

• point (475-477)

src/core/triangulation.rs (1)

• number_of_vertices (163-165)

src/core/triangulation.rs (1)

src/core/boundary.rs (1)

• boundary_facets (71-79)

benches/microbenchmarks.rs (1)

src/core/delaunay_triangulation.rs (6)

• new (96-98)
• vertices (392-394)
• with_kernel (214-251)
• tds (421-423)
• is_valid (786-791)
• insert (673-690)

src/lib.rs (4)

src/core/algorithms/locate.rs (1)

• locate (204-277)

src/core/delaunay_triangulation.rs (6)

• triangulation (463-465)
• new (96-98)
• vertices (392-394)
• number_of_vertices (275-277)
• number_of_cells (301-303)
• tds (421-423)

src/geometry/kernel.rs (5)

• new (204-208)
• new (314-319)
• orientation (90-93)
• orientation (217-227)
• orientation (363-373)

src/core/triangulation_data_structure.rs (4)

• vertices (801-803)
• number_of_vertices (894-896)
• number_of_cells (1034-1036)
• get_cell (829-831)

examples/convex_hull_3d_20_points.rs (3)

src/geometry/util.rs (16)

• generate_random_triangulation (2115-2181)
• generate_random_triangulation (4828-4828)
• generate_random_triangulation (4845-4845)
• generate_random_triangulation (4862-4862)
• generate_random_triangulation (4866-4866)
• generate_random_triangulation (4895-4895)
• generate_random_triangulation (4905-4905)
• generate_random_triangulation (4916-4916)
• generate_random_triangulation (4920-4920)
• generate_random_triangulation (4946-4946)
• generate_random_triangulation (4953-4953)
• generate_random_triangulation (4960-4960)
• generate_random_triangulation (4967-4967)
• generate_random_triangulation (4980-4980)
• generate_random_triangulation (5008-5008)
• generate_random_triangulation (5026-5026)

src/core/delaunay_triangulation.rs (2)

• tds (421-423)
• facets (554-556)

src/geometry/algorithms/convex_hull.rs (2)

• from_triangulation (682-728)
• facets (410-412)

src/core/delaunay_triangulation.rs (6)

src/geometry/util.rs (1)

• core (1809-1809)

src/core/cell.rs (6)

• cell (3224-3226)
• new (389-420)
• vertices (593-595)
• is_valid (984-1024)
• serialize (247-260)
• deserialize (274-359)

src/core/util.rs (3)

• geometry (1677-1677)
• geometry (1755-1755)
• is_delaunay (1662-1694)

src/core/triangulation.rs (6)

• vertices (142-144)
• new_empty (85-90)
• build_initial_simplex (348-421)
• cells (115-117)
• facets (242-244)
• insert (498-511)

src/core/triangulation_data_structure.rs (9)

• vertices (801-803)
• empty (2273-2282)
• cells (734-736)
• cells (3191-3200)
• is_valid (2838-2852)
• validate_facet_sharing (2740-2759)
• validate_neighbors (2967-3046)
• serialize (3183-3207)
• deserialize (3217-3344)

src/core/vertex.rs (6)

• empty (375-385)
• is_valid (585-603)
• serialize (212-225)
• serialize (778-783)
• deserialize (241-337)
• deserialize (787-801)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Codacy Security Scan
• GitHub Check: performance-regression
• GitHub Check: Code Coverage
• GitHub Check: build (macos-latest)
• GitHub Check: build (windows-latest)
• GitHub Check: build (ubuntu-latest)

🔇 Additional comments (33)

Cargo.toml (4)

34-34: Approve slotmap upgrade to 1.1.0.

This is a safe minor version bump. The serde feature is appropriately maintained for serialization support.

---

36-36: Approve uuid upgrade to 1.19.0.

This is a safe minor version bump. Both v4 and serde features are maintained, so no API changes are expected.

---

39-39: Verify criterion 0.8.0 compatibility with benchmark suite.

Criterion 0.8.0 is available on crates.io, and the PR claims all benchmarks compile successfully. This is a minor version bump from 0.7, which should be backward compatible. However, please verify that none of the 6 benchmarks (circumsphere_containment, triangulation_creation, microbenchmarks, ci_performance_suite, profiling_suite, large_scale_performance) rely on deprecated or changed APIs from the criterion 0.7 → 0.8 transition.

---

51-73: Benchmark removal aligns with PR consolidation goals.

The removal of the "assign_neighbors_performance" benchmark entry (noted in PR summary) is consistent with the code consolidation and simplification objectives. The remaining 6 benchmarks cover the key performance scenarios for the refactored triangulation APIs.

src/core/algorithms/incremental_insertion.rs (4)

382-383: Good use of #[allow(unused_variables)] to suppress the warning.

The annotation on line 382 properly addresses the facet_key variable that's only used in debug builds, which was flagged in a previous review. This is a clean solution.

---

134-248: Solid cavity filling implementation with good error handling.

The fill_cavity function properly:

• Documents partial mutation semantics (lines 118-133)
• Validates boundary cell dimensionality (lines 197-207)
• Uses appropriate error types throughout
• Includes debug assertions for duplicate detection

---

731-759: extend_hull correctly orchestrates hull extension workflow.

The function properly:

• Finds visible boundary facets using kernel predicates
• Fills the cavity with new cells
• Wires neighbors without removing conflict cells (appropriate for hull extension)
• Returns meaningful errors for edge cases

---

492-520: mirror_facet_index function is actively used in neighbor wiring logic.

The function is called at line 673 within the neighbor-wiring process to find the mirror facet index after hash-based candidate matching. It's an essential part of the algorithm, not dead code. The approach combines hash matching (to find candidate neighbors) with direct vertex comparison (via mirror_facet_index) to establish correct bidirectional cell relationships.

Likely an incorrect or invalid review comment.

src/core/algorithms/locate.rs (4)

204-277: Well-implemented facet-walking point location algorithm.

The locate function:

• Properly handles empty triangulations (lines 218-221)
• Uses cycle detection with visited set (lines 232-240)
• Has safety limit for degenerate cases (line 216)
• Returns appropriate LocateResult variants

The algorithm correctly implements the facet-walking approach documented in the references.

---

491-496: Let chains syntax is correctly used here.

This is valid stable Rust (let chains stabilized in 1.64.0), not the unstable guard patterns. The syntax if let Some(x) = opt && condition compiles correctly on Rust 1.91.0.

---

570-701: Robust cavity boundary extraction with degeneracy detection.

The extract_cavity_boundary function includes:

• Facet deduplication via hash set (lines 586-642)
• Ridge fan detection for non-manifold topology (lines 650-698)
• Meaningful error variants for geometric degeneracies

The ridge fan threshold of 3 (line 691) correctly identifies non-manifold boundaries where more than 2 facets share a ridge.

---

279-296: Critical invariant dependency is well-documented.

The is_point_outside_facet function clearly documents the topological invariant it depends on (lines 285-296), including references to where the invariant is enforced and validated. This is excellent defensive documentation.

tests/check_perturbation_stats.rs (1)

7-44: Good test structure for validating perturbation statistics.

The test properly:

• Uses a seeded random generator for reproducibility
• Tracks multiple statistics categories
• Handles errors gracefully by counting as skipped
• Validates final triangulation state

benches/microbenchmarks.rs (3)

18-19: Benchmarks correctly migrated to new DelaunayTriangulation API.

The benchmarks now use:

• DelaunayTriangulation::<RobustKernel<f64>, (), (), $dim>::with_kernel(...) for construction
• dt.insert(...) for incremental insertion
• dt.is_valid(), dt.validate_vertex_mappings(), etc. for validation

This aligns with the PR's API consolidation objectives.

Also applies to: 72-72

---

111-112: Accessing TDS counts via dt.tds() is appropriate for memory benchmarks.

The pattern dt.tds().number_of_vertices() correctly accesses the underlying TDS to get vertex/cell counts, which is the intended accessor pattern per the PR summary.

---

340-343: Benchmark target names updated consistently.

All benchmark function names have been updated from bowyer_watson_triangulation_* to benchmark_delaunay_triangulation_*, maintaining consistency with the renamed functions.

src/geometry/kernel.rs (5)

46-147: Well-designed Kernel trait following CGAL architecture.

The trait properly:

• Requires Clone + Default for ergonomic usage
• Associates a Scalar type with CoordinateScalar bound
• Provides orientation and in_sphere predicates with consistent return semantics (-1, 0, +1)
• Has comprehensive documentation with examples

---

211-241: Predicate result mapping is consistent between kernels.

Both FastKernel and RobustKernel correctly map:

• Orientation::NEGATIVE → -1, DEGENERATE → 0, POSITIVE → 1
• InSphere::OUTSIDE → -1, BOUNDARY → 0, INSIDE → 1

This ensures interchangeability between kernels.

Also applies to: 357-387

---

678-693: Good test for near-degenerate cases with robust kernel.

The test_orientation_nearly_collinear_2d_robust test properly exercises the robust kernel's behavior with nearly-collinear points (1e-10 deviation), validating that it returns a valid orientation value.

---

814-818: Default trait test ensures kernel compatibility with validation code.

Testing that both kernels implement Default is important since the Kernel trait requires it, and this ensures both can be used interchangeably in generic contexts.

---

357-360: Note the additional num_traits::Zero bound on RobustKernel.

The RobustKernel implementation requires T: CoordinateScalar + Sum + num_traits::Zero (line 359), while FastKernel only requires T: CoordinateScalar + Sum (line 213). This difference is correct and reflects the actual requirements of the underlying robust_insphere function, which requires the num_traits::Zero trait. Users switching between kernels should be aware that RobustKernel has this additional constraint on the scalar type.

examples/convex_hull_3d_20_points.rs (1)

49-485: DelaunayTriangulation migration in the example looks consistent and idiomatic.

The switch from raw Tds to DelaunayTriangulation (using dt.triangulation() for ConvexHull and dt.tds() for low-level stats/iteration) is clean and consistent across analysis, containment tests, visibility, and performance sections. The example exercises the new API surface without bypassing invariants, and hull extraction/validation are wired correctly.

src/lib.rs (3)

19-95: Updated crate‑level examples line up with the new Delaunay‑centric API.

The basic 4D triangulation and convex hull examples correctly use delaunay::prelude::*, DelaunayTriangulation::<_, (), (), N>::new, and ConvexHull::from_triangulation(dt.triangulation()), matching the new public surface and type aliases. These doctests should remain stable as the Delaunay wrapper evolves.

---

97-229: Invariants and incremental‑insertion documentation now match the implementation.

The descriptions of structural invariants, helper methods (is_valid, validation_report, validate_delaunay, per‑invariant validators), incremental cavity‑based insertion, degeneracy handling, and duplicate detection are consistent with the new DelaunayTriangulation/Triangulation + incremental_insertion pipeline. This resolves the earlier Tds/Bowyer–Watson doc drift and makes the guarantees much clearer.

---

448-751: Prelude and tests give a solid, coherent public surface.

Re‑exporting DelaunayTriangulation, Triangulation, kernels, convex hull, predicates, quality metrics, collections, and locate/InsertionError via prelude, plus the new tests that exercise these exports (quality, kernels, core types, point location, convex hull), provides a well‑rounded, easy‑to‑use API while keeping internals (like collections) under explicit import. The wiring and coverage here look good.

src/core/triangulation.rs (2)

37-285: Triangulation wrapper and basic accessors are clean and minimal.

The Triangulation<K, U, V, D> struct and its basic accessors (new_empty, cells, vertices, counts, dim, facets, boundary_facets) are a thin, well‑bounded layer over Tds and match how the rest of the crate expects to consume topology. The boundary‑facet iterator correctly relies on build_facet_to_cells_map() and panics only on structural corruption, which is appropriate for an internal‑facing helper.

---

1674-2067: Triangulation tests across dimensions exercise the new helpers well.

The macro‑generated tests for build_initial_simplex, degeneracy rejection, and localized facet validation/repair from 2D through 5D give strong coverage of the new Triangulation API and facet‑sharing invariants. They validate vertex/cell counts, incident‑cell assignments, and manifold facet sharing, which should catch most regressions in these core helpers.

src/core/delaunay_triangulation.rs (6)

64-251: Constructors and bootstrap/with‑kernel flows are consistent and well‑documented.

The specialized new/empty for FastKernel<f64> and generic with_empty_kernel/with_kernel correctly delegate to Triangulation::new_empty and Triangulation::build_initial_simplex + incremental insertion. The docs now match the behavior: you can truly start from empty() / with_empty_kernel() and let the (D+1)th insertion bootstrap the initial simplex, while with_kernel handles batch construction plus cavity‑based continuation. Tests for both fast and robust kernels confirm this.

---

253-331: Accessors over Triangulation and Tds provide a clean, layered API.

number_of_vertices, number_of_cells, dim, cells, vertices, tds, triangulation, and triangulation_mut cleanly separate combinatorial access (Tds) from kernel‑aware operations (Triangulation). The safety notes on triangulation_mut are thorough and point users to insert / remove_vertex as safer alternatives.

---

588-753: Insertion and removal delegation to Triangulation looks correct.

insert fully delegates to Triangulation::insert, caching the returned cell hint in last_inserted_cell, and the docs correctly describe interior (cavity) and exterior (hull extension) behavior, with degenerate OnFacet/OnEdge/OnVertex locations still reported as errors. remove_vertex cleanly reuses Triangulation::remove_vertex, with docs explicitly calling out that Delaunay may be temporarily violated until future flip‑based refinement is added. This aligns with the new triangulation layer.

---

899-997: Delaunay validation wrapper and error mapping are well thought‑out.

validate_delaunay delegates to core::util::is_delaunay and maps DelaunayValidationError variants into TriangulationValidationError with helpful UUID context for cells/vertices where possible. This keeps a single structural error type for callers while still distinguishing Delaunay violations, structural issues, and numeric predicate failures.

---

1034-1135: Serialization specialization and validation policy scaffolding are reasonable.

Serializing only the underlying Tds and reconstructing a stateless kernel on deserialize is a pragmatic choice, and the FastKernel specialization is clearly documented as the “common case” while pointing advanced users to from_tds for custom kernels. The DelaunayCheckPolicy enum is explicitly marked experimental and unused, which makes its intent clear without surprising current users.

---

1137-1738: DelaunayTriangulation tests give strong coverage of the new public API.

The test_incremental_insertion! macro (2D–5D), the empty()/with_empty_kernel() tests, with_kernel tests (Fast/Robust/f32), and various insert/tds accessor/validation tests together exercise both batch and bootstrap construction paths, incremental insertion, and the read‑only/mutable accessors. This should catch most regressions in the high‑level API and confirms the intended semantics described in the docs.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/core/algorithms/incremental_insertion.rs (1)

103-142: Make is_retryable rely on structured error variants instead of message substrings

InsertionError::is_retryable currently detects non-manifold wiring via NeighborWiring { message } and message.contains("Non-manifold"), which is brittle to future wording changes. Since you already have a TODO here, consider introducing a dedicated structured variant (e.g., NeighborWiring::NonManifoldTopology { … }) and matching on that instead of parsing the message string. This keeps retry semantics robust as error messages evolve.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48d3cf7 and 16df375.

📒 Files selected for processing (4)

• examples/convex_hull_3d_20_points.rs (20 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)
• tests/check_perturbation_stats.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• tests/check_perturbation_stats.rs
• src/core/triangulation.rs
• examples/convex_hull_3d_20_points.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/triangulation.rs
• examples/convex_hull_3d_20_points.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/triangulation.rs
• examples/convex_hull_3d_20_points.rs
• src/core/algorithms/incremental_insertion.rs

🧬 Code graph analysis (2)

tests/check_perturbation_stats.rs (4)

src/core/delaunay_triangulation.rs (3)

• empty (126-128)
• is_valid (786-791)
• number_of_vertices (275-277)

src/core/triangulation_data_structure.rs (4)

• empty (2273-2282)
• default (206-208)
• is_valid (2838-2852)
• number_of_vertices (894-896)

src/core/vertex.rs (1)

• point (475-477)

src/core/triangulation.rs (1)

• number_of_vertices (163-165)

src/core/algorithms/incremental_insertion.rs (5)

src/core/algorithms/locate.rs (1)

• locate (204-277)

src/core/triangulation.rs (4)

• boundary_facets (277-285)
• number_of_vertices (163-165)
• cells (115-117)
• vertices (142-144)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/triangulation_data_structure.rs (7)

• default (206-208)
• number_of_vertices (894-896)
• cells (734-736)
• cells (3191-3200)
• vertices (801-803)
• cell_keys (819-821)
• get_cell (829-831)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: build (macos-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (windows-latest)
• GitHub Check: Codacy Security Scan
• GitHub Check: Code Coverage
• GitHub Check: performance-regression

🔇 Additional comments (2)

tests/check_perturbation_stats.rs (1)

52-63: Division-by-zero guard and casting are now safe and well-scoped

The successful guard cleanly prevents division by zero, and using NumCast for the small usize totals in this test is perfectly fine. The average is only computed when there is at least one successful insertion, with a clear fallback message otherwise.

examples/convex_hull_3d_20_points.rs (1)

49-482: Example migration to DelaunayTriangulation looks consistent and correct

The example now cleanly uses DelaunayTriangulation as the main handle, delegating topology to dt.tds() and hull/visibility/containment operations to dt.triangulation(). Facet and hull analyses, timing loops, and memory estimates all align with the new API surface with no observable correctness issues.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

docs/numerical_robustness_guide.md (4)

313-334: Obsolete code example: References removed Bowyer-Watson algorithm pattern.

Lines 313–334 demonstrate robust_insphere usage within a find_bad_cells() method pattern that is specific to the legacy Bowyer-Watson implementation. Per PR #132, the entire bowyer_watson.rs, robust_bowyer_watson.rs, and trait-based InsertionAlgorithm patterns are removed. This code example will not work after consolidation.

Replace with an example using the new consolidated DelaunayTriangulation::insert() API.

---

400-435: Critical: Extensive code examples use removed APIs (RobustBoyerWatson, Tds::new(), trait-based insertion).

Lines 400–435, 465–506, and 519–567 contain extensive code examples (~120 lines total) demonstrating usage of:

• Tds::new() (being removed)
• RobustBoyerWatson type and its factory methods (entire type removed)
• insert_vertex() pattern within trait-based algorithms (removed with consolidation)
• RobustBoyerWatson::new(), ::with_config(), ::for_degenerate_cases() (all removed)

These examples will not compile after PR #132 merges. All code examples that demonstrate insertion patterns must be rewritten to use the new DelaunayTriangulation::insert() API.

Also applies to: 465-506, 519-567

---

870-887: Critical: Migration strategy contradicts PR #132's actual consolidation and removal of RobustBoyerWatson.

Lines 870–887 recommend that users adopt RobustBoyerWatson::for_degenerate_cases() as an immediate strategy and evaluate robust algorithms as a medium-term approach. However, PR #132 removes RobustBoyerWatson entirely as part of the consolidation. This migration strategy is obsolete and misleading—users following this advice will face code breakage when this PR merges.

The section must be rewritten to describe the actual API consolidation into DelaunayTriangulation and how error handling patterns have evolved.

---

63-76: Critical: Documentation extensively references non-existent modules and traits throughout the file.

Section 5 (lines 71–76) references src/core/algorithms/robust_bowyer_watson.rs, and code examples at lines 402+, 495+, and 512+ import RobustBoyerWatson and InsertionAlgorithm—neither of which exist in the current codebase. The documented migration strategy (lines 862–909) promotes RobustBoyerWatson::for_degenerate_cases() as a solution, but this API does not exist.

Additionally, code examples reference Tds::new() and the trait InsertionAlgorithm, both of which are not present in src/core/. These examples cannot compile against the current implementation. The documentation describes aspirational or obsolete features as "Implemented." Section 5 and all related code examples must be removed or completely rewritten to reflect the actual architecture (incremental insertion via DelaunayTriangulation).

♻️ Duplicate comments (1)

src/core/triangulation.rs (1)

1366-1432: repair_local_facet_issues docs still over-promise compared to implementation.

The docstring (lines 1350-1354) states "Returns error if quality evaluation or facet bookkeeping fails while selecting cells to remove", but the implementation never returns an error:

• Lines 1380-1393: radius_ratio failures are silently filtered with .filter_map
• Line 1429: Always returns Ok(removed_count)

This is the same issue flagged in past reviews (marked as "✅ Addressed in commit 00a9f51"), but the mismatch persists. Either:

1. Propagate errors from quality evaluation:

                 .filter_map(|&cell_key| {
                     let quality_result = radius_ratio(self, cell_key);
                     let uuid = self.tds.get_cell(cell_key)?.uuid();

-                    quality_result.ok().and_then(|ratio| {
+                    Some((cell_key, quality_result, uuid))
+                })
+                .collect::<Result<Vec<_>, _>>()?;
+            
+            let mut cell_qualities: Vec<(CellKey, f64, Uuid)> = raw_qualities
+                .into_iter()
+                .filter_map(|(cell_key, quality_result, uuid)| {
+                    quality_result.ok().and_then(|ratio| {
                         safe_scalar_to_f64(ratio)
                             .ok()
                             .filter(|r| r.is_finite())
                             .map(|r| (cell_key, r, uuid))
                     })
                 })

2. Update the docs to match the best-effort behavior:

-    /// # Errors
-    ///
-    /// Returns error if quality evaluation or facet bookkeeping fails while
-    /// selecting cells to remove. This function itself does not rebuild neighbors;
+    /// This function uses quality metrics to select cells to remove. Quality
+    /// evaluation failures are handled gracefully (affected cells are excluded
+    /// from quality-based selection and fall back to UUID ordering).
+    ///
+    /// This function does not rebuild neighbors;

🧹 Nitpick comments (10)

src/core/triangulation.rs (4)

713-715: Document or reconsider capturing inserted_uuid and point before TDS mutation.

Lines 713-715 capture inserted_uuid and point before inserting the vertex into the TDS. While this is necessary for the initial-simplex rebuild case (lines 739-744), it's unclear why point needs to be captured early since it's not modified by insert_vertex_with_mapping.

Consider adding a comment explaining why these captures happen before insertion, or removing the point capture if it's not strictly necessary for correctness.

     ) -> Result<((VertexKey, Option<CellKey>), usize), InsertionError>
     where
         K::Scalar: CoordinateScalar,
     {
         use crate::core::algorithms::incremental_insertion::{
             extend_hull, fill_cavity, wire_cavity_neighbors,
         };
         use crate::core::algorithms::locate::{
             LocateResult, extract_cavity_boundary, find_conflict_region, locate,
         };
 
-        // Capture the inserted vertex's UUID and point before any TDS operations
+        // Capture UUID before insertion for TDS rebuild case (line 739-744)
+        // Point is captured for convenience (not strictly needed before insertion)
         let inserted_uuid = vertex.uuid();
         let point = *vertex.point();

---

797-818: Clarify iterative repair loop termination conditions.

The repair loop (lines 797-818) iterates up to 10 times but has unclear termination logic:

• Line 801: detect_local_facet_issues returns Option<FacetIssuesMap>
• Line 814-817: Loop breaks if issues is None
• But there's no check for whether repair made progress

If repair_local_facet_issues removes 0 cells and issues persist, the loop will continue unnecessarily. Consider adding an early-exit check:

             if let Some(issues) = self.detect_local_facet_issues(&remaining_cells)? {
                 #[cfg(debug_assertions)]
                 eprintln!(
                     "Repair iteration {}: {} over-shared facets detected, removing cells...",
                     iteration + 1,
                     issues.len()
                 );
 
                 let removed = self.repair_local_facet_issues(&issues)?;
+                
+                if removed == 0 {
+                    #[cfg(debug_assertions)]
+                    eprintln!("No cells removed in iteration {} - stopping repair", iteration + 1);
+                    break;
+                }
+                
                 total_removed += removed;
 
                 #[cfg(debug_assertions)]
                 eprintln!("Removed {removed} cells (total: {total_removed})");
             } else {

---

875-880: Validate that post-repair locate test is meaningful.

Lines 875-880 attempt to locate the just-inserted point after repair to detect cycles. However:

1. The point was just inserted successfully, so it's guaranteed to be inside the triangulation
2. The locate test uses Some(test_cell) as a hint, which may not exercise the full walking logic
3. If repair created cycles, the test might not catch them if the hint cell is already the containing cell

Consider either:

• Adding a comment explaining this is a basic sanity check, not comprehensive cycle detection
• Using a different point (e.g., a boundary vertex) to force actual walking
• Or removing the test if it's not adding meaningful validation

                 // CRITICAL: Validate that locate still works after repair
-                // If repair created cycles, we'll catch it here
+                // Basic sanity check that locate doesn't crash. Note: This uses a hint
+                // so it may not exercise full walking logic. Comprehensive cycle detection
+                // happens in validate_no_neighbor_cycles() in debug builds.
                 if let Some(test_cell) = self.tds.cells().next().map(|(k, _)| k) {
                     // Try to locate the point we just inserted
                     let _ = locate(&self.tds, &self.kernel, &point, Some(test_cell))?;
                 }

---

1265-1328: detect_local_facet_issues could return early on first over-shared facet for faster failure.

The current implementation (lines 1315-1327) collects all over-shared facets before returning. For callers that just need to know "are there issues?", this does unnecessary work.

Consider adding an optional early_return: bool parameter:

 pub fn detect_local_facet_issues(
     &self,
     cells: &[CellKey],
+    early_return: bool,
 ) -> Result<Option<FacetIssuesMap>, TriangulationValidationError>

Then exit immediately on first detection:

     for (facet_hash, cell_facet_pairs) in facet_to_cells {
         if cell_facet_pairs.len() > 2 {
+            if early_return {
+                // Return immediately on first over-shared facet
+                let mut early_map = FacetIssuesMap::default();
+                early_map.insert(facet_hash, cell_facet_pairs);
+                return Ok(Some(early_map));
+            }
             over_shared.insert(facet_hash, cell_facet_pairs);
         }
     }

This optimization is optional but could speed up repair loops that check "any issues?" repeatedly.

src/core/algorithms/incremental_insertion.rs (6)

200-239: Debug-only duplicate facet detection is thorough but expensive.

Lines 200-239 implement comprehensive duplicate facet detection, but only in debug builds. This is good for catching bugs during development, but consider:

1. The detection runs on every fill_cavity call, which happens for every inserted vertex
2. Building the seen_facets map is O(k·D) where k = boundary facets
3. The detection only prints warnings—it doesn't prevent the creation of duplicate cells

Consider adding a note in the function docs warning that duplicate boundary facets will create overlapping cells (which will be caught/repaired later by topology validation), rather than implying this check prevents the issue:

 /// # Partial Mutation on Error
 ///
 /// **IMPORTANT**: If this function returns an error, the TDS may be left in a
 /// partially updated state with some new cells already inserted. This function
 /// does NOT rollback cells created before the error occurred.
+///
+/// **Note (Debug Builds)**: In debug builds, this function checks for duplicate
+/// boundary facets and logs warnings if found. Duplicate facets will create
+/// overlapping cells, which will be detected and repaired by subsequent topology
+/// validation passes (see `detect_local_facet_issues` / `repair_local_facet_issues`).

---

292-301: Validate 1:1 correspondence assertion is redundant given the algorithm.

Lines 292-301 assert that boundary_facets.len() == new_cells.len(). However:

1. The loop at lines 243-290 iterates exactly once per boundary facet
2. Each iteration pushes exactly one cell to new_cells (line 289)
3. There are no continue or early-exit branches

Therefore, this assertion can never fail (barring memory corruption). Consider either:

1. Removing the check as it's guaranteed by construction
2. Moving it to debug-only (debug_assert_eq!) if you want the safety check
3. Adding a comment explaining it's a defensive check

-    // Validate we created one cell per boundary facet (1:1 correspondence)
-    if boundary_facets.len() != new_cells.len() {
-        return Err(InsertionError::CavityFilling {
-            message: format!(
-                "Created {} cells for {} boundary facets (should be 1:1)",
-                new_cells.len(),
-                boundary_facets.len()
-            ),
-        });
-    }
+    // Defensive check: 1:1 correspondence is guaranteed by construction
+    // (one iteration per boundary facet, one cell push per iteration)
+    debug_assert_eq!(
+        boundary_facets.len(),
+        new_cells.len(),
+        "Created {} cells for {} boundary facets (should be 1:1)",
+        new_cells.len(),
+        boundary_facets.len()
+    );

---

418-433: Boundary facet filtering logic in wire_cavity_neighbors is subtle—add explanatory comment.

Lines 418-433 implement a critical heuristic: only add existing cells to the facet map if they match a boundary facet of the new cells (len == 1). The comment at lines 420-423 explains this prevents non-manifold topology, but the logic is subtle.

Consider expanding the comment to explain the full rationale:

             // Only add if this facet matches one from new cells
             if let Some(existing_facet_cells) = facet_map.get_mut(&facet_key) {
-                // Only add to boundary facets (len == 1)
-                // Internal facets of new cells (len >= 2) should not be wired to existing cells
-                // This prevents creating non-manifold topology where a facet is shared by
-                // multiple new cells AND an existing cell
+                // CRITICAL: Only wire to boundary facets (len == 1)
+                //
+                // Why: If len >= 2, the facet is already shared by multiple new cells (internal).
+                // Adding an existing cell would create a non-manifold configuration where the facet
+                // is shared by 3+ cells (>2 new cells + existing cell).
+                //
+                // By only wiring to boundary facets (len == 1), we ensure:
+                // - Boundary facets get their external neighbor (existing cell)
+                // - Internal facets remain paired between new cells only
+                // - Manifold property is preserved (each facet shared by ≤2 cells)
                 if existing_facet_cells.len() == 1 {

---

662-675: Redundant cell retrieval in repair_neighbor_pointers—refactor for clarity.

Line 664 retrieves cell from the TDS, but this cell reference is only used to extract vertex keys (lines 669-674). The cell was already retrieved at line 634, but that reference is no longer in scope.

Consider restructuring to avoid the redundant lookup:

         // For each facet that needs repair, find its neighbor by facet matching
         for facet_idx in facets_to_repair {
             // Build facet vertex keys (all except facet_idx)
-            let cell = tds
-                .get_cell(cell_key)
-                .ok_or_else(|| InsertionError::NeighborWiring {
-                    message: format!("Cell {cell_key:?} not found during facet matching"),
-                })?;
-            let mut facet_vkeys = SmallBuffer::<VertexKey, MAX_PRACTICAL_DIMENSION_SIZE>::new();
-            for (i, &vkey) in cell.vertices().iter().enumerate() {
+            let cell_vertices = tds
+                .get_cell_vertices(cell_key)
+                .map_err(|e| InsertionError::NeighborWiring {
+                    message: format!("Cell {cell_key:?} not found during facet matching: {e}"),
+                })?;
+            let mut facet_vkeys = SmallBuffer::<VertexKey, MAX_PRACTICAL_DIMENSION_SIZE>::new();
+            for (i, &vkey) in cell_vertices.iter().enumerate() {
                 if i != facet_idx {
                     facet_vkeys.push(vkey);
                 }
             }

This uses the existing get_cell_vertices API, which likely has similar performance but expresses intent more clearly.

---

783-784: Debug-only cycle validation runs unconditionally—document performance impact.

Line 784 calls validate_no_neighbor_cycles(tds)? in debug builds. This is a BFS traversal (lines 799-849) that:

• Samples up to 10 cells
• Walks up to 10,000 steps per cell
• Uses a HashSet to track visited cells

For large triangulations (10⁶+ cells), even sampling 10 cells could be expensive. Consider adding a note in the repair_neighbor_pointers docs:

 /// # Algorithm
 /// 1. For each cell, check each neighbor pointer
 /// 2. If neighbor is None or points to non-existent cell, mark for repair
 /// 3. Build facet hash for that specific facet
 /// 4. Scan all other cells to find the one sharing that facet
 /// 5. Wire mutual neighbors only for the broken facets
+///
+/// # Debug Validation
+/// In debug builds, this function performs additional cycle detection via BFS
+/// (see `validate_no_neighbor_cycles`). This adds overhead but helps catch
+/// neighbor graph corruption early during development.

---

990-997: Visibility criterion comment is excellent—consider extracting to module-level doc.

Lines 990-997 provide a detailed explanation of the visibility criterion and the conservative coplanar handling. This is valuable context that would benefit other readers of the module.

Consider moving this explanation to the module-level docs (top of file) or to a dedicated section in the docstring for extend_hull, so it's discoverable without reading the implementation:

 //! Incremental Delaunay insertion using cavity-based algorithm.
 //!
 //! This module implements efficient incremental insertion following CGAL's approach:
 //! 1. Locate the cell containing the new point (facet walking)
 //! 2. Find conflict region (BFS with in_sphere tests)
 //! 3. Extract cavity boundary facets
 //! 4. Remove conflict cells
 //! 5. Fill cavity (create new cells connecting boundary to new vertex)
 //! 6. Wire neighbors locally (no global assign_neighbors call)
+//!
+//! ## Hull Extension and Visibility
+//!
+//! When inserting a vertex outside the current convex hull, the algorithm finds
+//! *visible* boundary facets using orientation tests:
+//! - A facet is **strictly visible** if the new point and the opposite vertex
+//!   have opposite orientations relative to the facet's supporting hyperplane.
+//! - **Coplanar cases** (orientation == 0) are conservatively treated as non-visible
+//!   to avoid numerical instability. This may cause "no visible facets" errors for
+//!   points nearly on the hull surface.
+//! - For "weakly visible" behavior, a threshold-based approach would be needed
+//!   (not currently implemented).

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16df375 and 67a8949.

📒 Files selected for processing (4)

• docs/code_organization.md (6 hunks)
• docs/numerical_robustness_guide.md (3 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• docs/numerical_robustness_guide.md
• docs/code_organization.md
• src/core/triangulation.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• docs/numerical_robustness_guide.md
• docs/code_organization.md
• src/core/triangulation.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• docs/code_organization.md
• src/core/triangulation.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• docs/code_organization.md

📚 Learning: 2025-09-02T02:40:54.439Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 58
File: scripts/tests/test_subprocess_utils.py:92-101
Timestamp: 2025-09-02T02:40:54.439Z
Learning: In Rust projects, cargo is expected to be available in development and CI environments, so conditional skipping of cargo-dependent tests is typically unnecessary.

Applied to files:

• docs/code_organization.md

🧬 Code graph analysis (2)

src/core/triangulation.rs (8)

src/core/vertex.rs (7)

• uuid (502-504)
• point (475-477)
• hash (736-740)
• from (681-683)
• from (694-696)
• from (707-709)
• from (772-774)

src/core/algorithms/incremental_insertion.rs (3)

• repair_neighbor_pointers (607-787)
• fill_cavity (190-304)
• wire_cavity_neighbors (329-478)

src/geometry/quality.rs (1)

• radius_ratio (223-276)

src/core/triangulation_data_structure.rs (2)

• cells (734-736)
• cells (3191-3200)

src/geometry/kernel.rs (5)

• new (204-208)
• new (314-319)
• orientation (90-93)
• orientation (217-227)
• orientation (363-373)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/algorithms/locate.rs (2)

• locate (204-277)
• extract_cavity_boundary (570-701)

src/core/algorithms/incremental_insertion.rs (4)

src/core/algorithms/locate.rs (1)

• locate (204-277)

src/core/facet.rs (15)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• key (580-602)
• facet_vertices (1091-1115)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)
• next (772-807)
• next (852-864)

src/core/cell.rs (7)

• cell (3224-3226)
• hash (1456-1472)
• new (389-420)
• number_of_vertices (682-684)
• neighbors (561-563)
• vertices (593-595)
• map (323-323)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: build (windows-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: performance-regression
• GitHub Check: build (ubuntu-latest)
• GitHub Check: Code Coverage
• GitHub Check: Codacy Security Scan

🔇 Additional comments (5)

docs/numerical_robustness_guide.md (1)

142-240: All APIs and patterns described in the "Error Handling and Retry Logic" section (lines 142–240) are correctly implemented in the consolidated architecture and match the documented behavior.

Verified components:

• InsertionError enum with all error variants (NonManifoldTopology, DuplicateCoordinates, DuplicateUuid, TopologyValidation)
• is_retryable() method correctly classifies retryable vs. non-retryable errors
• InsertionStatistics struct with attempts, used_perturbation, cells_removed_during_repair, and success fields
• insert_with_statistics() and insert_transactional() methods in Triangulation
• Progressive perturbation schedule (1e-4, 1e-3, 1e-2, 2e-2, 5e-2) with up to 5 retry attempts
• Transactional rollback semantics via TDS cloning on failed attempts
• Error classification distinguishing geometric degeneracies (retryable via perturbation) from structural failures (non-retryable)

The documentation accurately reflects the actual implementation.

src/core/algorithms/incremental_insertion.rs (1)

118-157: LGTM! is_retryable classification is well-designed.

The is_retryable method provides clear semantics for distinguishing geometric degeneracies (retryable via perturbation) from structural failures (not retryable). The classification is thorough:

• ✅ Locate cycles → retryable (numerical degeneracy)
• ✅ Non-manifold topology → retryable (can be resolved by perturbation)
• ✅ Conflict region duplicates/ridge fans → retryable
• ✅ Duplicate UUIDs/coordinates → not retryable (structural)
• ✅ Legacy string-based check for backwards compatibility

This is a significant improvement over string-based error classification.

docs/code_organization.md (3)

278-284: Documentation accurately reflects new architecture layers.

Lines 278-284 correctly document the new layered structure:

• triangulation_data_structure.rs - Main Tds struct
• delaunay_triangulation.rs - DelaunayTriangulation (top layer)
• triangulation.rs - Generic Triangulation layer with kernel

This aligns with the code changes in the PR. The architecture overview is clear and accurate.

---

363-367: Documentation claims verified as accurate.

Lines 363-367 correctly document the InsertionError implementation. The NonManifoldTopology variant exists with the documented fields (facet_hash and cell_count), the is_retryable() method properly handles it as retryable, and the implementation uses structured error matching via enum patterns (line 139) rather than string parsing. No changes needed.

---

118-124: Stale references to removed algorithm files exist in active documentation and code comments.

The directory tree in lines 118-124 is accurate (only flips.rs, incremental_insertion.rs, locate.rs exist), but multiple stale references to removed files persist:

• src/core/triangulation_data_structure.rs:63 - Comment references bowyer_watson::find_bad_cells()
• tests/README.md - Test documentation references convex_hull_bowyer_watson_integration.rs and integration_robust_bowyer_watson.rs
• tests/COVERAGE.md - Coverage data lists removed files: bowyer_watson.rs, robust_bowyer_watson.rs
• docs/OPTIMIZATION_ROADMAP.md - References removed robust_bowyer_watson.rs and bowyer_watson.rs

Update these references to reflect the new algorithm structure or remove outdated sections.

⛔ Skipped due to learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/core/triangulation.rs (1)

801-843: Repair loop logic is sound but could benefit from early success check

The repair iteration logic (lines 804-843) properly:

• Limits iterations to 10 for safety
• Detects stalled repair (removed == 0) and returns error
• Accumulates total removed cells
• Breaks when no more issues detected (line 841)

However, there's a minor optimization opportunity: after removing cells in the repair loop, you could check if facet sharing is now valid before continuing to the next iteration. This would save unnecessary detect_local_facet_issues calls.

Optional optimization:

                 total_removed += removed;

                 #[cfg(debug_assertions)]
                 eprintln!("Removed {removed} cells (total: {total_removed})");
+
+                // Early exit if repair succeeded
+                if self.tds.validate_facet_sharing().is_ok() {
+                    break;
+                }
             } else {

This is a minor performance optimization and not critical for correctness.

src/core/algorithms/incremental_insertion.rs (2)

1015-1036: Strict visibility criterion is conservative but may reject valid coplanar cases

The current implementation treats coplanar cases (orientation == 0) as non-visible (lines 1018-1031). This is documented as a conservative approach to avoid numerical instability, but it has consequences:

Current behavior:

• Only facets with strictly opposite orientations are considered visible
• Points nearly on the hull surface may trigger "No visible boundary facets" errors
• The TODO comment (lines 1024-1029) acknowledges this limitation

Trade-off:

• ✅ Conservative: Avoids creating degenerate cells from numerical instability
• ❌ Restrictive: May reject valid insertions for points very close to the hull

Recommendation: This is acceptable for now given the progressive perturbation retry logic in Triangulation::insert_transactional (which will perturb coplanar points away from the hull surface). However, the TODO should be tracked for future improvement.

Consider creating an issue to track the weakly-visible enhancement mentioned in the TODO.

Would you like me to create an issue to track the "weakly visible" threshold-based approach mentioned in the TODO comment (lines 1024-1029)?

---

1041-1151: Good test coverage for cavity filling across multiple dimensions

The test suite provides solid coverage:

1. Dimension-parametric tests (lines 1047-1150): Uses macro to generate tests for 2D-5D, ensuring algorithm works uniformly.

2. Comprehensive validation (lines 1068-1095): Each test verifies:

   • Correct number of cells created (one per boundary facet)
   • Neighbor wiring succeeds without errors
   • TDS remains valid after operations
   • All new cells have correct vertex count (D+1)

3. Integration testing: Tests fill_cavity and wire_cavity_neighbors together, which is appropriate since they must work as a unit.

Consider adding tests for error cases (e.g., invalid facet indices, missing cells) to verify error handling paths.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67a8949 and 2a19dcf.

📒 Files selected for processing (3)

• docs/numerical_robustness_guide.md (10 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• docs/numerical_robustness_guide.md
• src/core/triangulation.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• docs/numerical_robustness_guide.md
• src/core/triangulation.rs
• src/core/algorithms/incremental_insertion.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• docs/numerical_robustness_guide.md
• src/core/triangulation.rs
• src/core/algorithms/incremental_insertion.rs

🧬 Code graph analysis (1)

src/core/algorithms/incremental_insertion.rs (3)

src/core/algorithms/locate.rs (1)

• locate (204-277)

src/core/facet.rs (14)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• key (580-602)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)
• next (772-807)
• next (852-864)

src/core/boundary.rs (1)

• boundary_facets (71-79)

🪛 LanguageTool

docs/numerical_robustness_guide.md

[style] ~793-~793: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... Points have extreme coordinate values (very large or very small) - Input data comes from ...

(EN_WEAK_ADJECTIVE)

---

[style] ~793-~793: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...xtreme coordinate values (very large or very small) - Input data comes from measurements w...

(EN_WEAK_ADJECTIVE)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Code Coverage
• GitHub Check: performance-regression
• GitHub Check: build (ubuntu-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: build (windows-latest)
• GitHub Check: Codacy Security Scan
• GitHub Check: Codacy Static Code Analysis

🔇 Additional comments (12)

src/core/triangulation.rs (5)

353-426: LGTM: build_initial_simplex correctly validates degeneracy and constructs initial simplex

The implementation properly:

• Validates input count (exactly D+1 vertices)
• Uses kernel orientation test to detect degenerate simplices (collinear/coplanar)
• Constructs the TDS with proper incident cell assignment
• Provides clear error messages for all failure cases

The design note about K::default() (line 307-308) is a good forward-looking comment about stateful kernels.

---

1068-1169: LGTM: remove_vertex correctly implements fan triangulation with topology repair

The implementation properly:

• Finds all cells containing the vertex (lines 1084-1094)
• Handles edge cases (no cells, empty boundary) with early returns
• Uses fan triangulation to fill the cavity (lines 1114-1126)
• Wires neighbors before removing old cells (line 1128)
• Applies localized facet validation and repair (lines 1140-1160)
• Conditionally repairs neighbors only when cells were removed (lines 1152-1159)
• Rebuilds incident cells before final vertex removal (lines 1163-1166)

The algorithm maintains topological consistency throughout the operation.

---

1406-1489: LGTM: repair_local_facet_issues correctly implements quality-based cell selection

The repair logic properly:

• Computes quality metrics for all involved cells (lines 1419-1450)
• Propagates errors from quality evaluation (lines 1430-1449)
• Validates that quality ratios are finite (lines 1441-1449)
• Uses quality-based selection when available (lines 1452-1466)
• Falls back to UUID ordering when quality computation fails (lines 1467-1481)
• Keeps the 2 best cells per over-shared facet (manifold invariant)
• Defers neighbor rebuilding to the caller (lines 1483-1488)

The error handling ensures that failures in quality computation are surfaced to the caller rather than silently ignored.

---

1726-2191: Excellent test coverage with dimension-parametric test generation

The test suite provides comprehensive coverage:

1. Dimension-parametric tests (lines 1735-1834): The macro-based approach generates tests across 2D-5D, ensuring the algorithm works uniformly across dimensions.

2. Error handling tests (lines 1836-1958): Validates proper error messages for insufficient vertices, excess vertices, collinear points (2D), and coplanar points (3D).

3. Facet validation tests (lines 1960-2119): Verifies the manifold topology invariant (≤2 cells per facet) across all dimensions with comprehensive edge cases.

4. Topology consistency test (lines 2121-2190): Validates neighbor pointer symmetry and consistency after vertex removal with repair operations.

The test organization is clean and follows Rust conventions. The use of macros to generate dimension-specific tests is a good pattern for this library.

---

577-632: UUID changes on retry attempts are rolled back entirely; UUID preservation is not necessary for transactional correctness.

Each failed insertion attempt is completely rolled back via tds = tds_snapshot, which restores the entire TDS state including all UUID mappings. Because only successful insertions persist, the fact that each retry generates a new UUID is inconsequential—the UUID changes only exist within failed attempts that are discarded. Additionally, implementing the proposed fix is not possible: VertexBuilder has #[builder(setter(skip))] on the uuid field, which intentionally prevents a uuid() method from being generated on the builder. UUID generation is by design automatic and unique for each vertex instance to prevent accidental UUID reuse.

Likely an incorrect or invalid review comment.

src/core/algorithms/incremental_insertion.rs (4)

54-169: LGTM: InsertionError enum provides comprehensive error classification with retry semantics

The error design is well-structured:

1. Comprehensive variants (lines 54-128): Covers all failure modes including conflict region, location, construction, cavity operations, topology validation, and duplicate detection.

2. Retryable classification (lines 130-169): The is_retryable() method correctly identifies geometric degeneracies that can be resolved via coordinate perturbation (cycles, non-manifold topology, topology validation failures) versus structural failures that won't be fixed by perturbation (duplicate UUIDs, duplicate coordinates).

3. Integration with thiserror: Proper use of #[from] attributes for automatic conversions from underlying error types.

The error handling design enables the transactional retry logic in Triangulation::insert_transactional.

---

189-201: Partial mutation behavior is well-documented and acceptable for transactional caller

The documentation clearly states that fill_cavity may leave the TDS partially updated on error (lines 189-201). This design is acceptable because:

1. Higher-level rollback: The caller (Triangulation::insert_transactional) handles rollback by cloning the entire TDS before calling this function (line 635 in triangulation.rs).

2. Performance rationale: Avoiding double validation at this level improves performance while maintaining correctness through the transactional wrapper.

3. Clear contract: The documentation explicitly warns callers that recovery is their responsibility if they need transactional semantics.

This is a good example of layered error handling where low-level functions optimize for performance while high-level functions provide transactional guarantees.

---

434-454: Boundary facet check is critical for manifold preservation - excellent documentation

The conditional wiring to boundary facets only (lines 436-453) is a crucial correctness condition that's well-documented:

1. Problem: If we wired to facets shared by 2+ new cells (internal facets), we'd create non-manifold topology (>2 cells per facet).

2. Solution: Only wire when existing_facet_cells.len() == 1, ensuring:

   • Boundary facets get their external neighbor (existing cell)
   • Internal facets remain paired between new cells only
   • Manifold property preserved (each facet shared by ≤2 cells)

3. Documentation: The CRITICAL comment (lines 436-446) clearly explains the reasoning.

This is a subtle but essential correctness condition that's easy to overlook. The detailed comment ensures future maintainers understand the constraint.

---

634-814: LGTM: repair_neighbor_pointers implements surgical neighbor repair with proper error handling

The implementation correctly:

1. Preserves existing neighbors (lines 670-686): Only repairs facets with None or invalid neighbor pointers, leaving correct relationships intact.

2. Avoids double-wiring (lines 652-657, 746-757): Tracks wired pairs to ensure each facet pair is wired exactly once.

3. Uses mirror facet logic (lines 759-778): Correctly finds the corresponding facet index in the neighbor cell using mirror_facet_index.

4. Propagates errors (lines 661-665, 691-695, 713-717, 760-769): All cell lookups use ok_or_else to return descriptive errors instead of panicking.

5. Debug validation (lines 809-813): In debug builds, validates that no cycles were introduced via BFS traversal.

The surgical repair approach is more efficient than rebuilding all neighbor relationships and is appropriate for post-repair scenarios.

docs/numerical_robustness_guide.md (3)

142-240: Documentation accurately reflects the new error handling and retry logic

The "Error Handling and Retry Logic" section correctly documents:

1. Error classification (lines 144-171): Code example matches the InsertionError enum variants from incremental_insertion.rs.

2. Transactional insertion (lines 173-212): Example correctly shows insert_with_statistics returning both result and InsertionStatistics.

3. Perturbation schedule (lines 194-212): The documented schedule (1e-4, 1e-3, 1e-2, 2e-2, 5e-2) matches the implementation in triangulation.rs (lines 591-598).

4. Retryable errors (lines 214-240): The list of retryable vs non-retryable errors matches the is_retryable() implementation.

The documentation provides clear examples and explanations that will help users understand the error handling strategy.

---

310-328: Kernel usage examples are clear and demonstrate the intended API

The code examples correctly show:

1. FastKernel usage (lines 318-319): DelaunayTriangulation::empty() uses FastKernel by default.

2. RobustKernel usage (lines 322-327): Shows explicit kernel construction with with_empty_kernel(RobustKernel::new()).

3. Automatic predicate dispatch (line 326): Notes that kernel predicates are used automatically during insertion.

The examples provide a clear migration path for users experiencing numerical issues.

---

772-810: Performance considerations accurately reflect the trade-offs

The documentation provides practical guidance on when to use each kernel type:

1. Overhead percentages (lines 777-781): Documented with references to benchmark files and notes about variability.

2. RobustKernel use cases (lines 789-795): Clearly identifies when robust predicates are needed (insertion errors, degenerate geometry, extreme coordinates, measurement precision issues).

3. FastKernel use cases (lines 797-801): Identifies when standard predicates are sufficient (well-conditioned points, performance critical, general position).

4. Optimization strategies (lines 803-809): Includes the tiered approach (try fast, fall back to robust) which is a best practice.

The guidance will help users make informed decisions about kernel selection.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/core/triangulation.rs (1)

552-689: Transactional insertion flow and retry semantics now look correct

The updated insert_transactional logic correctly:

• snapshots tds before each attempt,
• rolls back on any error,
• distinguishes retryable vs non‑retryable errors via is_retryable,
• and returns the matched InsertionError directly instead of re‑unwrapping a moved Result.

This fixes the previous unwrap_err issue and keeps the manifold‑preserving semantics clear while encapsulating perturbation logic.

🧹 Nitpick comments (1)

src/core/triangulation.rs (1)

758-1027: Local facet-repair loops use global scans and duplicate degenerate handling

Two small things in try_insert_impl are worth tightening:

1. Global vs local facet checks in insertion repair

   Both the interior (InsideCell) and hull (Outside) branches build remaining_cells from all cells and call detect_local_facet_issues(&remaining_cells) on every iteration. This makes the repair loop effectively O(N·D) per iteration, even though you already track new_cells and (for interiors) know the conflict region. For large meshes this can dominate insertion cost.

   Consider restricting detection to the newly affected region, e.g.:

   • interior: detect_local_facet_issues(&new_cells) plus maybe a small halo if needed,
   • hull: similarly based on new_cells,
     and only falling back to a full pass if a validator elsewhere detects global issues.

   This keeps the “localized O(k·D)” story for detect_local_facet_issues accurate in the hot insertion paths.

2. Duplicated degenerate-location error path

   Degenerate locations (OnFacet, OnEdge, OnVertex) are already handled in the (location, conflict_cells) match by returning an InsertionError::CavityFilling. The final match location { _ => ... } arm with the same error message is therefore unreachable.

   Removing or consolidating that final _ arm would simplify the control flow without changing behavior.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a19dcf and a484bbd.

📒 Files selected for processing (3)

• WARP.md (1 hunks)
• src/core/algorithms/incremental_insertion.rs (1 hunks)
• src/core/triangulation.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• WARP.md
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• WARP.md
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• WARP.md
• src/core/algorithms/incremental_insertion.rs
• src/core/triangulation.rs

📚 Learning: 2025-11-21T22:40:08.556Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 124
File: justfile:111-112
Timestamp: 2025-11-21T22:40:08.556Z
Learning: In the delaunay repository justfile, the clippy recipe runs cargo clippy twice: once without --all-features to check default code paths, and once with --all-features to check feature-gated code. This is intentional and necessary because the repository has feature-gated code behind count-allocations, dense-slotmap, bench, and test-debug features.

Applied to files:

• WARP.md

🧬 Code graph analysis (2)

src/core/algorithms/incremental_insertion.rs (3)

src/core/facet.rs (13)

• cell (561-565)
• tds (411-413)
• new (263-268)
• new (432-456)
• new (744-761)
• new (833-841)
• cell_key (289-291)
• cell_key (397-399)
• key (580-602)
• facet_vertices (1091-1115)
• facet_index (312-314)
• facet_index (404-406)
• vertices (496-518)

src/core/triangulation.rs (3)

• boundary_facets (277-285)
• cells (115-117)
• vertices (142-144)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/triangulation.rs (9)

src/core/vertex.rs (9)

• uuid (502-504)
• point (475-477)
• empty (375-385)
• from (681-683)
• from (694-696)
• from (707-709)
• from (772-774)
• map (293-293)
• map (303-303)

src/core/cell.rs (7)

• uuid (710-712)
• cell (3224-3226)
• vertices (593-595)
• number_of_vertices (682-684)
• new (389-420)
• neighbors (561-563)
• map (323-323)

src/core/algorithms/incremental_insertion.rs (4)

• repair_neighbor_pointers (634-814)
• extend_hull (896-924)
• fill_cavity (207-320)
• wire_cavity_neighbors (345-500)

src/geometry/quality.rs (1)

• radius_ratio (223-276)

src/core/triangulation_data_structure.rs (10)

• empty (2273-2282)
• cells (734-736)
• cells (3191-3200)
• vertices (801-803)
• number_of_vertices (894-896)
• number_of_cells (1034-1036)
• default (206-208)
• cell_keys (819-821)
• contains_cell (839-841)
• get_cell (829-831)

src/geometry/kernel.rs (6)

• new (204-208)
• new (314-319)
• default (352-354)
• orientation (90-93)
• orientation (217-227)
• orientation (363-373)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/traits/boundary_analysis.rs (1)

• boundary_facets (79-81)

src/core/algorithms/locate.rs (1)

• locate (204-277)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Code Coverage
• GitHub Check: build (windows-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: build (ubuntu-latest)
• GitHub Check: performance-regression
• GitHub Check: Codacy Security Scan

🔇 Additional comments (13)

WARP.md (2)

27-27: Clarify the "edit_files" tool reference.

Line 27 mandates use of an "edit_files tool" for all code changes, but this tool is not a standard or widely documented part of most development environments or code-review systems. Please clarify:

• Is this a custom internal tool? If so, provide documentation or a link to the tool.
• Is this an aspirational best practice to be adopted later?
• Should this reference a different, established tool or workflow?

---

60-62: Verify MSRV and Rust Edition versions.

The document claims MSRV 1.91.0 and Edition 2024. Please confirm these are current and match the repository's actual configuration (e.g., Cargo.toml).

Rust 1.85.0 and Rust 2024 Edition were announced on February 20, 2025, so the reference to "Edition 2024" is current and accurate as of today's date.

src/core/algorithms/incremental_insertion.rs (9)

54-169: Excellent error taxonomy and retry logic.

The InsertionError enum provides comprehensive coverage of failure modes with clear categorization. The is_retryable() method correctly identifies geometric degeneracies (cycles, non-manifold topology) that can be resolved via coordinate perturbation while rejecting structural failures (duplicate UUIDs, coordinates). The use of structured error variants with detailed context (e.g., facet_hash, cell_count in NonManifoldTopology) enables precise debugging.

---

171-320: Well-structured cavity filling with appropriate safeguards.

The implementation correctly creates one (D+1)-cell per boundary facet with the new vertex as apex. Key strengths:

• Comprehensive documentation of partial-mutation semantics (lines 186-206) sets clear expectations for callers.
• Debug-mode duplicate detection (lines 217-256) provides early warnings without blocking insertion (duplicates are caught by subsequent topology validation).
• Defensive assertion (lines 311-317) verifies the 1:1 correspondence invariant.
• Descriptive error messages aid debugging.

---

322-500: Robust neighbor wiring with manifold preservation.

The facet-matching algorithm correctly handles both interior cavity filling and hull extension. Notable design decisions:

• Lines 436-446: The critical check if existing_facet_cells.len() == 1 ensures only boundary facets (not internal facets shared by multiple new cells) are wired to existing cells, preserving the manifold property (≤2 cells per facet).
• Lines 469-494: Structured NonManifoldTopology error with detailed debug logging enables coordinate perturbation retry while providing diagnostic context.
• Comprehensive facet indexing covers all necessary cell types (new, conflict, existing).

---

502-600: Clean helper implementations with appropriate algorithms.

• set_neighbor: Straightforward neighbor pointer update with proper error propagation.
• compute_facet_hash: Uses FastHasher for deterministic hashing consistent with FastHashMap/FastHashSet.
• mirror_facet_index: CGAL-style algorithm (lines 543-600) correctly identifies the vertex in neighbor_cell not contained in the shared facet. The O(D²) complexity is acceptable given typical dimensions (2-5) and the clear algorithmic structure.

---

602-814: Efficient repair with proper error handling.

The neighbor pointer repair correctly:

• Targets only broken/None pointers, preserving valid relationships (O(k·n·D) vs O(n²·D) for full rebuild).
• Uses mirror_facet_index for correct bidirectional wiring (lines 772-795).
• Tracks wired_pairs to prevent double-wiring the same facet (lines 746-757).
• Returns InsertionError::NeighborWiring instead of panicking on missing cells (addresses past review feedback).
• Includes debug-only cycle detection (lines 809-811) for early corruption detection during development.

---

816-876: Appropriate debug-mode cycle detection.

The BFS validation provides early detection of neighbor graph corruption during development:

• Samples 10 cells to balance coverage vs overhead.
• MAX_WALK_STEPS safety limit (10,000) prevents infinite loops in corrupted graphs.
• Debug-only (#[cfg(debug_assertions)]) ensures zero runtime cost in release builds.

---

878-924: Correct hull extension workflow.

The function properly sequences the hull extension operations:

1. Find visible boundary facets via orientation tests.
2. Fill cavity with new cells.
3. Wire neighbors (passing None for conflict_cells since hull extension doesn't remove cells).

The error message on line 912 helpfully notes the coplanar case, consistent with the conservative visibility criterion documented in find_visible_boundary_facets.

---

926-1039: Conservative visibility criterion with clear documentation.

The orientation-based visibility test is correctly implemented:

• Lines 999-1013: Computes orientation with opposite vertex and with query point.
• Lines 1030-1031: Strictly requires opposite signs (excludes coplanar cases where either orientation is 0).
• Lines 1018-1029: Comprehensive comment explains the conservative approach avoids numerical instability but may fail for near-hull points. The TODO identifies threshold-based approaches as future work.

This aligns with the module-level documentation (lines 11-21) and represents a reasonable trade-off between robustness and coverage.

---

1041-1396: Excellent test coverage across dimensions and error cases.

The test suite demonstrates thorough validation:

• Macro-generated tests (lines 1050-1152, 1313-1395) cover dimensions 2-5 for both cavity filling and neighbor repair.
• Error case tests (lines 1156-1241) validate handling of invalid keys, missing cells, and empty inputs.
• Retryability tests (lines 1245-1308) verify correct classification of geometric degeneracies vs structural failures.
• Validation checks ensure TDS validity (line 1084), vertex counts (lines 1089-1097), and neighbor consistency (lines 1323-1345).

The use of DelaunayTriangulation API and consistent patterns across dimensions reflects good test design.

src/core/triangulation.rs (2)

37-286: Triangulation core struct and accessors look solid

The Triangulation wrapper, basic accessors (cells, vertices, counts, dim, facets, boundary_facets), and their doctests are consistent with the Tds API and the new DelaunayTriangulation-centric architecture. The panic-on-corruption behavior in boundary_facets matches the docs and is appropriate for an internal-invariant violation.

---

1736-2259: Test macros provide good cross-dimensional coverage

The test macros for build_initial_simplex, facet validation, basic accessors, and remove_vertex (2D–5D) give strong coverage of both construction and localized repair behavior, including degeneracy rejection and neighbor symmetry checks. This should catch most regressions in the new triangulation orchestration.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/core/triangulation.rs (1)

804-804: Consider using a named constant for repair iteration limits.

Both interior insertion (line 804) and hull extension (line 937) use hardcoded 0..10 for repair loops, while the deprecated fix_invalid_facet_sharing defines const MAX_FIX_FACET_ITERATIONS: usize = 10 (line 1542). For consistency and maintainability, consider defining a shared constant like MAX_LOCAL_REPAIR_ITERATIONS and using it in all three locations.

Apply this diff to improve consistency:

+// Maximum iterations for local topology repair after insertion/removal
+const MAX_LOCAL_REPAIR_ITERATIONS: usize = 10;
+
 impl<K, U, V, const D: usize> Triangulation<K, U, V, D>
 where
     K: Kernel<D>,
@@ -801,7 +803,7 @@
                 // 9. Iteratively repair non-manifold topology until facet sharing is valid
                 let mut total_removed = 0;
                 #[allow(unused_variables)]
-                for iteration in 0..10 {
+                for iteration in 0..MAX_LOCAL_REPAIR_ITERATIONS {
                     // Check for non-manifold issues in newly created cells (local scan)
@@ -934,7 +936,7 @@
                 // Iteratively repair non-manifold topology until facet sharing is valid
                 let mut total_removed = 0;
                 #[allow(unused_variables)]
-                for iteration in 0..10 {
+                for iteration in 0..MAX_LOCAL_REPAIR_ITERATIONS {
                     // Check for non-manifold issues in newly created hull cells (local scan)

Optionally rename MAX_FIX_FACET_ITERATIONS to match, or keep separate if the deprecated method's limit should differ.

Also applies to: 937-937

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a484bbd and 3991a08.

📒 Files selected for processing (1)

• src/core/triangulation.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.

Applied to files:

• src/core/triangulation.rs

📚 Learning: 2025-08-30T02:40:22.032Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.

Applied to files:

• src/core/triangulation.rs

📚 Learning: 2025-09-04T20:03:49.896Z

Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.

Applied to files:

• src/core/triangulation.rs

🧬 Code graph analysis (1)

src/core/triangulation.rs (5)

src/core/vertex.rs (2)

• uuid (502-504)
• point (475-477)

src/core/algorithms/incremental_insertion.rs (1)

• repair_neighbor_pointers (634-814)

src/geometry/quality.rs (1)

• radius_ratio (223-276)

src/core/boundary.rs (1)

• boundary_facets (71-79)

src/core/algorithms/locate.rs (1)

• locate (204-277)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Codacy Security Scan
• GitHub Check: performance-regression
• GitHub Check: Code Coverage
• GitHub Check: build (windows-latest)
• GitHub Check: build (macos-latest)
• GitHub Check: build (ubuntu-latest)

🔇 Additional comments (7)

src/core/triangulation.rs (7)

1-516: Excellent foundation and bootstrap logic.

The module structure is clean, following CGAL's separation of geometric kernel and combinatorial TDS. The build_initial_simplex orientation validation correctly rejects degenerate configurations, and the transactional insertion API provides a robust entry point with automatic rollback and perturbation retry.

Key highlights:

• build_initial_simplex (lines 353-426): Validates orientation to prevent degenerate simplices (collinear/coplanar vertices)
• insert_transactional delegation (lines 503-515, 539-550): Progressive perturbation schedule up to 5% provides good balance between resolving degeneracies and avoiding numerical drift
• Past review issue (error propagation in insert_transactional) has been correctly addressed

---

713-751: Confirmed fix: Stale VertexKey issue resolved.

The captured inserted_uuid (line 721) and remapping after TDS rebuild (lines 746-751) correctly addresses the past review concern. The returned VertexKey is now always valid for the final TDS state.

---

651-683: Confirmed fix: Error propagation corrected.

The pattern Err(e) => { ... return Err(e); } (lines 651-683) correctly addresses the past issue of calling result.unwrap_err() on a moved value. Error is now properly owned and returned.

---

1154-1164: Confirmed fix: remove_vertex now returns total cells removed.

The accumulation of cells_removed += removed (line 1164) correctly addresses the past review comment. Callers now receive the total count including cells removed during facet repair, matching the documented behavior (line 1057).

---

1448-1467: Confirmed fix: Quality evaluation failures now propagate as errors.

The radius_ratio and safe_scalar_to_f64 errors are now properly propagated (lines 1448-1457) rather than silently falling back. The finite check (lines 1459-1467) ensures only valid quality values are used. This aligns the behavior with the updated documentation (lines 1407-1412) and addresses the past review concern.

---

1495-1535: Deprecation provides clear migration path.

The deprecated fix_invalid_facet_sharing includes a helpful note (lines 1497-1508) directing users to the localized O(k·D) approach (detect_local_facet_issues + repair_local_facet_issues). The doc comment clearly describes best-effort semantics (lines 1510-1531), which is appropriate for a deprecated method.

---

1730-2254: Comprehensive dimension-parametric test coverage.

The macro-based tests systematically verify correctness across 2D-5D for:

• Initial simplex construction (lines 1739-1826)
• Degeneracy rejection (lines 1840-1962)
• Facet validation (lines 1964-2184)
• Vertex removal with neighbor pointer integrity (lines 2024-2108)
• Basic accessors (lines 2110-2217)

This thorough coverage across dimensions gives high confidence in the implementation's robustness.