Conversation
Updates the documentation and adds a test case to clarify the limitations and alternatives for using string-like data with vertices, as only Copy data types are supported. This includes recommendations for using numeric IDs with external lookups, character codes, and custom Copy enums. (Internal Change)
Adds extensive unit tests for the `RobustBoyerWatson` triangulation algorithm, covering various functionalities and error handling scenarios. These tests validate the algorithm's behavior under extreme conditions, including handling of degenerate cases, precision failures, and invalid input data, ensuring its robustness and reliability in diverse scenarios.
Replaces `HashSet` with `FastHashSet` in `Cell`, `Facet`, `Vertex`, and `util` modules to improve performance by using a faster hash set implementation for checking duplicate vertices. Also replaces `Vec` with `SmallBuffer` in `InsertionBuffers` for `bad_cells_buffer`, `boundary_facets_buffer`, `vertex_points_buffer`, and `visible_facets_buffer` in `insertion_algorithm` module for performance gains. Refactors code for deserialization of `Cell` and `Facet` by removing unnecessary imports and standardizing the deserialization visitor pattern.
Enhances the algorithm test suite by adding new tests and improving coverage for both the standard and robust Boyer-Watson implementations. This includes testing default implementations, insertion strategies, facet cache providers, and error handling paths to ensure more robust and reliable algorithms. Additionally, adds `repr` to the cspell dictionary and excludes coverage reports from git tracking.
Addresses various error paths and edge cases in triangulation, convex hull, and predicates modules. These changes increase the overall reliability and robustness of the library. They include handling degenerate cases, invalid input, numerical precision issues, and potential overflow scenarios. Improved testing coverage.
Updates the rust toolchain version to 1.90 across Cargo.toml, clippy.toml, and rust-toolchain.toml. Also, regenerates performance benchmark results in PERFORMANCE_RESULTS.md with updated times, reflecting the new toolchain and potentially minor performance variations. Minor adjustments to benchmarking utility commands. Internal change.
Enhances the test suite for triangulation and convex hull functionality, focusing on error handling, degenerate cases, and performance. This includes improved coverage for numeric edge cases, visibility checks, and cache management. (Internal change: test suite improvement)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRenames IncrementalBoyerWatson to IncrementalBowyerWatson, refactors insertion buffers to SmallBuffer, changes visibility/boundary checks to Result-returning APIs, broadens trait bounds, adds CoordinateScalar::mantissa_digits, adjusts geometry predicates/point deserialization, bumps MSRV to 1.90.0, and refreshes docs/bench artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant TDS as TriangulationDataStructure
participant BW as IncrementalBowyerWatson
participant IA as InsertionAlgorithm
participant BUFF as InsertionBuffers (SmallBuffer)
Client->>TDS: insert_vertex(point, data)
TDS->>BW: construct/reuse algorithm
BW->>BUFF: prepare_*_buffers()
BW->>IA: determine_insertion_strategy(...)
IA->>IA: is_facet_visible_from_vertex(...)\nreturns Result<bool, InsertionError>
alt Ok(true/false)
IA-->>BW: Ok(true/false)
BW->>BW: update bad cells / cavity / boundary
BW-->>TDS: InsertionInfo
TDS-->>Client: Ok(result)
else Err(InsertionError)
IA-->>BW: Err(...)
BW-->>TDS: Err(InsertionError)
TDS-->>Client: Err(InsertionError)
end
sequenceDiagram
autonumber
participant User
participant TDS as Triangulation
participant Util as FacetKey/Maps
participant B as BoundaryAnalysis
User->>TDS: boundary_facets()
TDS->>Util: build facet_to_cells map
Util-->>TDS: map
TDS->>B: iterate facets, validate multiplicity
alt len == 1
B-->>TDS: Ok(include facet)
else len == 0 or > 2
B-->>TDS: Err(InvalidFacetMultiplicity)
end
TDS-->>User: Result<Vec<Facet>, TriangulationValidationError>
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request focuses on improving test coverage across the codebase while making some minor optimizations and clarifications. The changes include raising the minimum Rust version to 1.90.0, adding comprehensive tests for various modules, optimizing hash collection usage, and updating documentation to be more precise about data type constraints.
- Raised minimum supported Rust version to 1.90.0 for consistency across tooling
- Added extensive test coverage for error paths, edge cases, and geometric predicates
- Optimized cell validation and various modules to use
FastHashSetinstead of standardHashSet
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/COVERAGE.md | New comprehensive test coverage report documenting current state and improvement priorities |
| src/lib.rs | Updated feature description to clarify vertex/cell data type constraints |
| src/geometry/util.rs | Added 464 lines of comprehensive error path and edge case tests |
| src/geometry/robust_predicates.rs | Added 529 lines of tests covering error handling, configuration, and edge cases |
| src/geometry/predicates.rs | Added 316 lines of tests for display traits, error handling, and method consistency |
| src/geometry/point.rs | Streamlined imports and replaced HashMap with FastHashMap in tests |
| src/geometry/algorithms/convex_hull.rs | Added 1802 lines of comprehensive error handling and edge case tests |
| src/core/vertex.rs | Updated documentation, streamlined imports, and enhanced string data usage examples |
| src/core/util.rs | Removed unused extreme coordinates utilities, optimized hash collections usage |
| src/core/triangulation_data_structure.rs | Added 580 lines of coverage tests for error paths and multi-dimensional cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| //! - d-dimensional convex hulls | ||
| //! - Generic floating-point coordinate types (supports `f32`, `f64`, and other types implementing `CoordinateScalar`) | ||
| //! - Arbitrary data types associated with vertices and cells | ||
| //! - Copy-able data types associated with vertices and cells (see [`DataType`](core::traits::DataType) for constraints) |
There was a problem hiding this comment.
Good improvement in documentation clarity. This change makes the constraints on vertex and cell data types much more explicit, helping users understand the Copy requirement.
| //! - Copy-able data types associated with vertices and cells (see [`DataType`](core::traits::DataType) for constraints) | |
| //! - Data types associated with vertices and cells must implement the [`Copy`](https://doc.rust-lang.org/std/marker/trait.Copy.html) trait (see [`DataType`](core::traits::DataType) for additional constraints) |
|
|
||
| // The sign interpretation depends on both orientation and dimension parity | ||
| // For the lifted matrix formulation, even and odd dimensions have opposite sign conventions | ||
| let dimension_is_even = D.is_multiple_of(2); |
There was a problem hiding this comment.
Excellent addition of detailed comments explaining the dimension-dependent sign interpretation. This is critical algorithmic knowledge that makes the code much more maintainable.
| let dimension_is_even = D.is_multiple_of(2); | |
| let dimension_is_even = D % 2 == 0; |
| fn vertex_string_data_usage_examples() { | ||
| // This test demonstrates what works and what doesn't work with string data in vertices. | ||
| // Note: String data has limitations due to the DataType trait requirements and lifetime complexities. |
There was a problem hiding this comment.
Very helpful educational test that clearly explains the limitations of string data in vertices and provides practical alternatives. This will save users significant debugging time.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
+ Coverage 66.54% 71.74% +5.19%
==========================================
Files 18 18
Lines 3180 3298 +118
==========================================
+ Hits 2116 2366 +250
+ Misses 1064 932 -132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
bot
added
documentation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/geometry/util.rs (1)
480-517: Detect precision loss for target type T (f32) as well, not just f64.Current check guards only f64’s 53-bit integer precision; converting to f32 can still lose precision above 2^24−1. Use the smaller of f64’s and T’s mantissa limits.
pub fn safe_usize_to_scalar<T: CoordinateScalar>( value: usize, ) -> Result<T, CoordinateConversionError> { - // Check for potential precision loss when converting usize to f64 - // f64 has 53 bits of integer precision (including the implicit bit). - // Integers up to 2^53 - 1 are exactly representable. - const MAX_PRECISE_USIZE_IN_F64: u64 = (1_u64 << 53) - 1; // 9,007,199,254,740,991 + // Guard precision for both the f64 intermediate and the target T. + // Use mantissa bits min(53 (f64), T::mantissa_digits()) to bound exact integers. + const F64_MANTISSA_BITS: u32 = 53; + let t_mantissa_bits: u32 = T::mantissa_digits() as u32; + let max_precise_bits = core::cmp::min(F64_MANTISSA_BITS, t_mantissa_bits); + let max_precise_u128: u128 = (1u128 << max_precise_bits) - 1; // Use try_from to safely convert usize to u64 for comparison - let value_u64 = + let value_u64 = u64::try_from(value).map_err(|_| CoordinateConversionError::ConversionFailed { coordinate_index: 0, coordinate_value: format!("{value}"), from_type: "usize", to_type: std::any::type_name::<T>(), })?; - if value_u64 > MAX_PRECISE_USIZE_IN_F64 { + if (value_u64 as u128) > max_precise_u128 { return Err(CoordinateConversionError::ConversionFailed { coordinate_index: 0, coordinate_value: format!("{value}"), from_type: "usize", to_type: std::any::type_name::<T>(), }); } // Safe to convert to f64 without precision loss, then convert to TWARP.md (1)
202-203: MSRV docs are stale (1.89.0 vs PR’s 1.90.0).Project Context still lists MSRV 1.89.0. Update to 1.90.0 for consistency with this PR.
- - **Language**: Rust (MSRV 1.89.0, Edition 2024) + - **Language**: Rust (MSRV 1.90.0, Edition 2024)src/core/algorithms/robust_bowyer_watson.rs (1)
1013-1065: Fix missing numeric trait bounds in fallback_visibility_heuristic (compile-time).This method uses +=, -, *, and f64::from but only constrains T: DivAssign. Add the missing bounds to prevent generic usage failures.
Apply:
- fn fallback_visibility_heuristic( + fn fallback_visibility_heuristic( &self, facet: &Facet<T, U, V, D>, vertex: &Vertex<T, U, D>, ) -> bool - where - T: DivAssign<T>, + where + T: DivAssign<T> + + AddAssign<T> + + std::ops::Sub<Output = T> + + std::ops::Mul<Output = T>, {src/core/boundary.rs (1)
101-134: Detect and fail on invalid facet multiplicities (enforce invariant).You state that 0, 3+ cell incidences indicate corruption, but the loop silently ignores those cases. Return a validation error when
cells.len()is neither 1 nor 2 to surface topology violations early.Example shape (adjust error variant to what you have available):
- for (_facet_key, cells) in facet_to_cells { - if let [(cell_id, facet_index)] = cells.as_slice() { + for (facet_key, cells) in facet_to_cells { + match cells.as_slice() { + [(cell_id, facet_index)] => { // existing boundary handling... - } + } + [_, _] => { + // shared/internal facet; skip + } + slice => { + // multiplicity must be 1 (boundary) or 2 (internal) + return Err( + TriangulationValidationError::FacetError( + FacetError::InvalidFacetMultiplicity { + facet_key, + found: slice.len(), + } + ) + ); + } + }
🧹 Nitpick comments (60)
cspell.json (1)
111-111: "conv" may be too generic and mask real typos; prefer scoping or removal.
If “conv” is short for “convolution” in benches/docs, scope it via an overrides entry for those files rather than whitelisting globally. Otherwise consider removing it.Apply this change locally in words and add a file‑scoped override:
- "conv",Add (outside this hunk) an overrides entry targeting the specific files/dirs that use “conv” (adjust patterns as needed):
{ "overrides": [ { "filename": "benches/**", "ignoreWords": ["conv"] } ] }rust-toolchain.toml (1)
3-3: Toolchain pin matches Cargo/clippy; consider adding rust-analyzer (optional).Adding "rust-analyzer" to components helps IDEs match the pinned toolchain.
components = [ - "cargo", + "cargo", "clippy", "rustfmt", "rust-docs", "rust-std", "rust-src", + "rust-analyzer", ].gitignore (1)
21-22: Anchor tarpaulin coverage path to repo root to avoid over‑matching.Without a leading slash, this could ignore similarly named nested dirs.
-coverage_report/tarpaulin-report.* +/coverage_report/tarpaulin-report.*src/geometry/util.rs (3)
892-902: Doc/behavior mismatch for 1D facet measure.Docs say “1D: Distance between two points,” but code returns unit measure (1). Align the docs or implement the distance variant. Given tests expect 1.0, update docs.
/// This function calculates the (D-1)-dimensional "area" of a facet in D-dimensional space: -/// - 1D: Distance between two points (length) +/// - 1D: Unit measure (1) by convention /// - 2D: Area of triangle using cross product /// - 3D: Volume of tetrahedron using scalar triple product /// - 4D+: Generalized volume using Gram matrix method
819-829: Preserve original error from circumradius_with_center.Mapping every error to MatrixInversionFailed hides root causes. Propagate as-is.
- circumradius_with_center(points, &circumcenter).map_err(|e| { - CircumcenterError::MatrixInversionFailed { - details: format!("Failed to calculate circumradius: {e}"), - } - }) + circumradius_with_center(points, &circumcenter)
1156-1163: Overly strict serde bounds on surface_measure generics.Requiring
[T; D]: DeserializeOwned + Serializehere isn’t needed and constrains callers. Consider removing serde bounds from this API.src/geometry/robust_predicates.rs (4)
223-249: Doc says row and column scaling; code scales rows only.Either implement column scaling too or adjust docs to “row scaling.”
-/// This method applies scaling and pivoting techniques to improve the -/// condition number of the matrix before computing the determinant. +/// This method applies row scaling to improve the +/// condition number of the matrix before computing the determinant.
289-322: Unnecessary serde bounds on orientation helpers.For numeric predicates,
DeserializeOwned + Serializeon[T; D]isn’t required. Dropping them would widen usability without behavior change.
419-427: Guard against non‑finite config tolerances explicitly.Currently relies on unwrap_or defaults; consider sanitizing NaN/inf to avoid surprising boundary results.
- let base_tol: f64 = safe_scalar_to_f64(config.base_tolerance).unwrap_or(1e-15); - let rel_factor: f64 = safe_scalar_to_f64(config.relative_tolerance_factor).unwrap_or(1e-12); + let mut base_tol: f64 = safe_scalar_to_f64(config.base_tolerance).unwrap_or(1e-15); + let mut rel_factor: f64 = safe_scalar_to_f64(config.relative_tolerance_factor).unwrap_or(1e-12); + if !base_tol.is_finite() { base_tol = 1e-15; } + if !rel_factor.is_finite() { rel_factor = 1e-12; }
578-607: Deterministic tie‑breaking is non‑geometric; document intent.Lexicographic OUTSIDE/INSIDE doesn’t reflect geometry, but it’s deterministic. Add a brief comment warning consumers this is last‑resort behavior.
src/geometry/algorithms/convex_hull.rs (5)
3131-3133: Avoid brittle assumption: cached_generation may be 0 initially
assert!(cached_gen.load(Ordering::Acquire) > 0);can fail ifTds::newdoesn’t bump generation before cache build. Compare totds.generation()instead.- let cached_gen = hull.cached_generation(); - assert!(cached_gen.load(std::sync::atomic::Ordering::Acquire) > 0); + let cached_gen = hull.cached_generation(); + assert_eq!( + cached_gen.load(std::sync::atomic::Ordering::Acquire), + tds.generation() + );
2461-2474: Relax time-based assertion to avoid CI flakinessHard-coding
< 1sfor 100 validations is brittle on shared runners. Gate by env or widen the bound.- assert!( - elapsed.as_millis() < 1000, - "Validation should be fast (< 1s for 100 calls)" - ); + let budget_ms: u128 = std::env::var("VALIDATION_BUDGET_MS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(2500); + assert!( + elapsed.as_millis() < budget_ms, + "Validation should be fast (< {budget_ms} ms for 100 calls); took {elapsed:?}" + );
4012-4016: Don’t rely on Arc pointer identity for cache reuse
Arc::ptr_eqis an implementation detail; reuse isn’t guaranteed by the trait. Validate semantics (non-empty, same keys), not identity.- // Verify the cache Arc is the same (reused) - assert!( - Arc::ptr_eq(&cache1, &cache2), - "Cache Arc should be reused when generation matches" - ); + // Validate semantic reuse: same visible mapping size + assert_eq!(cache1.len(), cache2.len(), "Cache should be reused (same size)");Also applies to: 4040-4043
3224-3231: 1D facet count assumption: confirm invariantAsserting
facet_count() == 2in 1D encodes an invariant of the boundary analysis. If upstream changes represent endpoints differently, this will break. Consider asserting “> 0” or document this invariant in boundary analysis.
2895-2972: Visibility error-path test doesn’t actually exercise the errorThis block only logs potential errors; it never asserts a specific failure path. Either assert the ok-path or craft an invalid facet to hit
InsufficientVerticesdeterministically, or mark with#[ignore]if purely smoke-test.benches/PERFORMANCE_RESULTS.md (2)
64-68: Align “100% agreement” with “Method Disagreements” narrativeThe doc says all methods agree 100%, but the “Method Disagreements” section still explains why disagreements happen. Clarify that this run observed 100% agreement and keep the disagreement notes as caveats for other datasets/hardware.
-### Numerical Accuracy Analysis +### Numerical Accuracy Analysis (this run) @@ -- **All three methods agree**: 100.0% (expected due to different numerical approaches) +- **All three methods agree**: 100.0% @@ -### Method Disagreements - -The disagreements between methods are expected due to: +### Method Disagreements (caveats for other runs) + +On different datasets/hardware/tolerances, methods can disagree due to:Also applies to: 137-144
6-13: Record toolchain and generator provenanceFor reproducibility, include the
uvversion and benchmark-utils version/commit that generated this file.**Generated By**: benchmark_utils.py +**Generator Version**: benchmark-utils {VERSION or git rev} +**uv**: {uv --version} **Git Commit**: 16321db9427557c7dadfc1c171f1005547c1bd56benches/README.md (2)
117-126: Add install command for uv to make snippet copy‑pasteableYou mention the prerequisite but don’t show how to install. Add a one-liner.
-# Prerequisite: install uv (e.g., `pipx install uv` or `brew install uv`) +# Prerequisite: install uv (one of) +# macOS: brew install uv +# Linux: curl -LsSf https://astral.sh/uv/install.sh | sh +# Python: pipx install uv
21-31: Note MSRV/toolchain expectation for benchmarksBench commands assume Rust 1.90.0 per PR. Document this to avoid local mismatches affecting results.
cargo bench --bench ci_performance_suite + +Note: Benchmarks are validated with Rust 1.90.0 (MSRV). Use `rustup override set 1.90.0` in repo root for comparable results.src/core/algorithms/bowyer_watson.rs (2)
124-145: Public type name typo: “Boyer” vs “Bowyer”. Add alias to avoid future confusion.The algorithm is “Bowyer–Watson”; struct is named IncrementalBoyerWatson. Keep the existing name, but add a correctly spelled alias to improve API ergonomics without breakage.
pub struct IncrementalBoyerWatson<T, U, V, const D: usize> @@ } + +// Back-compat friendly alias with correct spelling. +pub type IncrementalBowyerWatson<T, U, V, const D: usize> = + IncrementalBoyerWatson<T, U, V, D>;
195-209: Prefer UFCS for trait method call to avoid ambiguity.determine_insertion_strategy calls is_vertex_interior via method resolution on self. Use UFCS to make it explicit it’s the InsertionAlgorithm default method.
- if self.is_vertex_interior(tds, vertex) { + if <Self as InsertionAlgorithm<T, U, V, D>>::is_vertex_interior(self, tds, vertex) {src/core/facet.rs (2)
483-500: Micro-alloc nit: consider SmallVec to avoid heap alloc in key().Vertices per facet are small (D items). Using a small stack buffer avoids heap churn in hot paths.
- let mut uuid_values = Vec::with_capacity(vertices.len() * 2); + // Optional: use smallvec if already a dependency; otherwise keep Vec. + let mut uuid_values = Vec::with_capacity(vertices.len() * 2);If smallvec isn’t a dependency, ignore.
601-613: Same alloc nit in facet_key_from_vertex_keys.Minor, but SmallVec (or reusing a scratch buffer) would cut allocations in tight loops.
tests/COVERAGE.md (1)
37-44: Section title contradicts listed percentages.“Low Coverage Files (<40%)” contains 50–60% entries. Rename the section or fix thresholds.
-### Low Coverage Files (<40%) - **Priority for Improvement** +### Focus Files (≤60%) - **Priority for Improvement**WARP.md (1)
153-156: Coverage step: enable doctests and confirm integration-test exclusion.
- Add --doc to the cargo-tarpaulin command (or use --run-types Doctests,Tests to run both doctests and unit tests).
- Excluding tests/** omits integration-test coverage — remove that exclude if you want integration tests counted.
File: WARP.md lines 153-156
src/core/collections.rs (2)
826-836: Add spilled() assertions to validate heap fallback behaviorYou already assert capacity; also assert SmallVec spills when requested capacity exceeds inline size to catch regressions.
- let buffer = small_buffer_with_capacity_8::<i32>(5); - assert!(buffer.capacity() >= 5); + let mut buffer = small_buffer_with_capacity_8::<i32>(5); + assert!(buffer.capacity() >= 5); + // Force growth beyond inline 8 to validate spill + for i in 0..9 { buffer.push(i); } + assert!(buffer.spilled());
919-953: Great coverage boost; add one spill check per helperThese comprehensive tests look good. Consider adding explicit spill checks to ensure behavior across 2 and 16 inline capacities.
- let buffer2 = small_buffer_with_capacity_2::<i32>(10); + let mut buffer2 = small_buffer_with_capacity_2::<i32>(10); assert!(buffer2.capacity() >= 10); + buffer2.extend(0..3); // > inline(2) -> heap + assert!(buffer2.spilled()); - let buffer16 = small_buffer_with_capacity_16::<String>(25); + let mut buffer16 = small_buffer_with_capacity_16::<String>(25); assert!(buffer16.capacity() >= 25); + buffer16.extend(std::iter::repeat(String::new()).take(17)); // > inline(16) + assert!(buffer16.spilled());src/geometry/point.rs (2)
250-260: Casting path is fine; prefer NumCast::from for readability
cast::cast::<f64, T>(f)works, butNumCast::from(f)tends to be clearer to readers.- n.as_f64() - .and_then(|f| cast::cast::<f64, T>(f)) + n.as_f64() + .and_then(num_traits::cast::NumCast::from)
250-277: Deserialize impl is JSON-specific; consider format-agnostic VisitorUsing serde_json::Value inside a generic Deserialize ties this impl to JSON. If you plan to support other formats (CBOR, bincode), switch to an untagged enum and parse via the Deserializer to avoid JSON coupling.
// Sketch (outside selected range): #[derive(Deserialize)] #[serde(untagged)] enum CoordRepr<T> { Num(T), Str(String), Null } // In Visitor::visit_seq: let element: CoordRepr<T> = seq.next_element()?.ok_or_else(...)?;src/geometry/predicates.rs (5)
236-244: Minor: use method-form abs for clarityReadability nit:
(circumradius - radius).abs()is clearer thanFloat::abs(...), and leverages the Float extension method.- if Float::abs(circumradius - radius) < tolerance { + if (circumradius - radius).abs() < tolerance {
1310-1385: Debug-only test: consider marking #[ignore] to keep CI noise lowThis test prints diagnostics without assertions. Mark as ignored or gate logs.
- #[test] + #[test] + #[ignore = "diagnostic output only"] fn debug_circumsphere_properties() {
1445-1479: 2D consistency assertions are commented out—track with TODOIf 2D still has edge inconsistencies, leave a TODO with a minimal repro and link to an issue so it doesn’t get lost.
509-653: API inconsistency: error types differ between methods
inspherereturns CoordinateConversionError;insphere_liftedreturns CellValidationError. Consider a unifiedInSphereErrorenum to simplify call sites and testing.
1640-1694: Nice stress test; include D=2 and D=4 to broaden parity coverageExtend the stress test to sample 2D and 4D cases so parity logic stays protected.
src/core/algorithms/robust_bowyer_watson.rs (3)
927-941: Make DivAssign bound explicit at call site.This function calls fallback_visibility_heuristic which needs T: DivAssign. Add it here to surface constraints early.
) -> bool where - T: AddAssign<T> + ComplexField<RealField = T> + SubAssign<T> + Sum + From<f64>, + T: AddAssign<T> + ComplexField<RealField = T> + SubAssign<T> + Sum + From<f64> + DivAssign<T>, f64: From<T>, for<'a> &'a T: Div<T>,
3164-3173: Use the correct adjacent cell for the chosen test facet.The loop grabs an arbitrary boundary facet’s cell, which may not correspond to test_facet. Derive the facet key from test_facet and look it up.
- let mut adjacent_cell_key = None; - - for (_, cells_with_facets) in facet_to_cells.iter() { - if cells_with_facets.len() == 1 { - // Boundary facet - adjacent_cell_key = Some(cells_with_facets[0].0); - break; - } - } - - let adjacent_cell_key = adjacent_cell_key.expect("Should find adjacent cell"); + let key = derive_facet_key_from_vertices(&test_facet.vertices(), &tds) + .expect("Should derive facet key"); + let adjacent_cell_key = facet_to_cells + .get(&key) + .and_then(|cells| (cells.len() == 1).then_some(cells[0].0)) + .expect("Should find adjacent cell for test facet");
2997-3783: Tests substantially improve coverage; consider reducing noisy output.Great breadth. For faster CI logs, prefer avoiding println! in happy paths or gate behind cfg(test) debug flags.
src/core/vertex.rs (1)
682-716: Consider serde_repr for the enum in tests.You can derive Serialize/Deserialize via serde_repr to avoid manual impls.
-#[repr(u8)] -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[repr(u8)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, serde_repr::Serialize_repr, serde_repr::Deserialize_repr)] enum PointType { ... } - -impl Serialize for PointType { ... } -impl<'de> Deserialize<'de> for PointType { ... }src/core/boundary.rs (3)
153-156: Docs: return type still described as bool; now returns Result<bool, TriangulationValidationError>.Update the “Returns” sections to reflect the Result type.
-/// `true` if the facet is on the boundary (belongs to only one cell), `false` otherwise. +/// Ok(true) if the facet is on the boundary (belongs to only one cell), +/// Ok(false) otherwise. Err(...) on validation/lookup failures.Also applies to: 202-206
291-294: Avoid building/allocating all boundary Facet objects for just a count.Count directly from the facet map; this is O(|facets|) with no facet cloning or per‑cell
facets()calls.- // Delegate to boundary_facets() for consistent error handling - self.boundary_facets().map(|v| v.len()) + self.build_facet_to_cells_map() + .map(|m| m.values().filter(|v| v.len() == 1).count())
1247-1290: Nice cache/capacity exercise; avoid brittle line-number references in comments.The test is good for exercising cache paths, but hardcoding code line numbers in comments tends to rot. Drop those references.
src/core/util.rs (1)
147-150: Adjacency check uses hashing; consider cheaper compare for tiny D.For D ≤ 4, sorting by UUID (or stable key) and comparing arrays avoids two hash-set allocations and hashing.
Example alternative (no TDS needed):
let mut a: Vec<_> = facet1.vertices().into_iter().map(|v| v.uuid()).collect(); let mut b: Vec<_> = facet2.vertices().into_iter().map(|v| v.uuid()).collect(); a.sort_unstable(); b.sort_unstable(); a == bsrc/core/triangulation_data_structure.rs (8)
2118-2140: Initialize the first cell using vertex keys to avoid copying full Vertexs.When creating the initial simplex, you collect and sort full
Vertexcopies, then pass them toCellBuilder. Consider resolving keys once and building from keys (or lettingCellBuilderaccept keys) to avoid copying and to keep a consistent key-first path like the rest of the file.- let mut all_vertices: Vec<_> = self.vertices.values().copied().collect(); - all_vertices.sort_unstable_by_key(super::vertex::Vertex::uuid); - let cell = CellBuilder::default() - .vertices(all_vertices) + let mut keys: Vec<_> = self.vertices.keys().collect(); + keys.sort_unstable(); // deterministic + let all_vertices = keys.into_iter().map(|k| self.vertices[k]).collect(); + let cell = CellBuilder::default() + .vertices(all_vertices) .build()
2151-2158: Pass-by-value Vertex to algorithm after insertion can be misleading.You insert
vertexintoself.verticesand then pass the same value by value toalgorithm.insert_vertex(self, vertex). If the algorithm only needs identity, prefer passing theVertexKeyorUuidto avoid confusion and accidental divergence between the stored copy and the passed value.Would the algorithm API accept a key/UUID instead of a full
Vertex? If yes, adjust signature; if not, add a brief comment clarifying that the passed value is only used for identity lookups.
1739-1789: Return SmallBuffer in get_cell_vertex_keys to avoid heap allocs for D ≤ 7.Hot path helper returns
Vec<VertexKey>. You already useVertexKeyBuffer = SmallBuffer<_, MAX>elsewhere. ReturningVertexKeyBufferhere keeps stack allocation for common dimensions.- ) -> Result<Vec<VertexKey>, TriangulationValidationError> { + ) -> Result<VertexKeyBuffer, TriangulationValidationError> { - let mut vertex_keys = Vec::with_capacity(D + 1); + let mut vertex_keys = VertexKeyBuffer::with_capacity(D + 1); for vertex in cell.vertices() { let key = self.vertex_key_from_uuid(&vertex.uuid()).ok_or_else(|| { TriangulationValidationError::InconsistentDataStructure { message: format!("Vertex UUID {} not found in mapping", vertex.uuid()), } })?; vertex_keys.push(key); } Ok(vertex_keys) }
2352-2372: Facet buffer reuse is good; consider avoiding per-iteration Vec growth.You reuse
facet_verticesbut still push for each facet. For small fixed D, usingSmallBuffer<VertexKey, MAX_PRACTICAL_DIMENSION_SIZE>here would eliminate heap growth entirely.- let mut facet_vertices = Vec::with_capacity(vertex_keys.len().saturating_sub(1)); + let mut facet_vertices: SmallBuffer<VertexKey, MAX_PRACTICAL_DIMENSION_SIZE> = + SmallBuffer::with_capacity(vertex_keys.len().saturating_sub(1));
2435-2461: Two-phase neighbor UUID materialization: acceptable; document allocation tradeoff.The two-phase
updatesVec is a pragmatic borrow-checker workaround. Consider adding a short comment noting thatupdatescan be large for big meshes and is a known tradeoff.
3549-3561: Sorting by coordinates with partial_cmp: guard against NaN paths.
unwrap_or(CmpOrdering::Equal)silently equates incomparable points (NaNs). If NaNs should never appear, add adebug_assert!or comment. Otherwise, use a total-order wrapper (e.g.,ordered_float) for robust ordering.- a_coords - .partial_cmp(&b_coords) - .unwrap_or(CmpOrdering::Equal) + { + debug_assert!(a_coords.iter().all(|x| x.is_finite()) && b_coords.iter().all(|x| x.is_finite()), + "Coordinates must be finite for deterministic ordering"); + a_coords + .partial_cmp(&b_coords) + .unwrap_or(CmpOrdering::Equal) + }Also applies to: 3563-3569
3712-3715: Deserialize sets generation to 0: bump or carry over?On deserialization you reset
generationto 0. If callers compare generations across serialize/deserialize boundaries, this reset may be surprising. Consider documenting this explicitly in the struct docs or exposing a constructor to set it.
7635-7686: Cell duplicate insertion test: also verify UUID bimap consistency.The test inserts multiple cells but doesn’t assert
uuid_to_cell_key.len()returns to expected after duplicate removal. Add an assertion to strengthen coverage of bimap hygiene.let duplicates_removed = result.remove_duplicate_cells().expect( "Failed to remove duplicate cells: triangulation should be valid after construction", ); @@ assert_eq!(result.number_of_cells(), original_cell_count); assert_eq!(duplicates_removed, 3); + assert_eq!(result.uuid_to_cell_key.len(), original_cell_count);src/core/cell.rs (4)
1124-1134: Redundant generic bound in Hash implYou call vertex.hash(state), so the impl only needs Vertex<T,U,D>: Hash; the Point<T,D>: Hash bound isn’t used here.
Apply this small simplification:
-impl<T, U, V, const D: usize> Hash for Cell<T, U, V, D> +impl<T, U, V, const D: usize> Hash for Cell<T, U, V, D> where T: CoordinateScalar, U: DataType, V: DataType, [T; D]: Copy + Default + DeserializeOwned + Serialize + Sized, - Point<T, D>: Hash, // Add this bound to ensure Point implements Hash + Vertex<T, U, D>: Hash, {
691-693: Prefer borrowing in contains_vertexTaking Vertex by value forces a copy; take &Vertex to avoid moves and match common “contains” patterns.
- pub fn contains_vertex(&self, vertex: Vertex<T, U, D>) -> bool { - self.vertices.contains(&vertex) + pub fn contains_vertex(&self, vertex: &Vertex<T, U, D>) -> bool { + self.vertices.contains(vertex) }Follow-ups:
- Update call sites: pass references (e.g., cell.contains_vertex(&v)).
956-1062: Facet construction clones the entire Cell per facetThis is correct functionally, but cloning a Cell for every facet can be costly in hot paths.
If/when Facet supports constructing from identifiers (e.g., (cell_key, facet_index)), consider adding a lightweight constructor to avoid cloning here and in boundary-finding code paths.
989-992: Docs mention ComplexField but it’s not in the boundsThe docstring lists ComplexField<RealField = T>, but the where-clause uses CoordinateScalar + Clone + PartialEq + PartialOrd + Sum only.
Please align the docs or the bounds to avoid confusion.
src/core/traits/insertion_algorithm.rs (6)
390-397: Public SmallBuffer fields are a breaking API changeChanging Vec public fields to SmallBuffer<_, N> alters the public surface and downstream code. If external crates construct/inspect these, this is semver‑breaking.
Consider making fields pub(crate) and adding accessors, or providing From<Vec<_>>/into_inner() helpers to ease migration. If breakage is intended for 0.x, please note it in the changelog.
433-437: Capacity heuristics: guard tiny inputsvisible_facets_buffer uses capacity/2; for very small inputs this can be 0 (inline cap likely saves you, but it’s unclear).
- visible_facets_buffer: SmallBuffer::with_capacity(capacity / 2), + visible_facets_buffer: SmallBuffer::with_capacity(core::cmp::max(1, capacity / 2)),
1000-1027: Avoid cloning Cell when building boundary facets (future improvement)Facet::new(cell.clone(), …) is correct but alloc-heavy. A facet constructor from (cell_key, facet_index) would eliminate clones in this hot path.
Happy to sketch an API if desired.
1699-1767: Visibility test: conservative false on topology anomaliesReturning false when no opposite vertex is found is safe. Optional: add a debug_assert! to flag potential TDS corruption during tests.
debug_assert!(opposite_vertex.is_some(), "Facet lacked opposite vertex for cell {:?}", adjacent_cell_key);
4052-4056: Tautological assertion in test (always true)assert!(!visible_facets.is_empty() || visible_facets.is_empty()) is a no‑op.
Apply one of:
- Drop the assert; rely on the earlier Ok(...) expectation.
- Or assert a property with signal, e.g., “<= 4 facets” for a tetrahedron:
- assert!( - !visible_facets.is_empty() || visible_facets.is_empty(), // Either case is geometrically valid - "Visibility computation should complete without errors" - ); + assert!(visible_facets.len() <= 4, "Cannot see more than 4 facets from a tetrahedron");
806-812: Optional: pre‑reserve bad_cellsA tiny win: reserve to avoid a couple of reallocations in dense meshes.
- let mut bad_cells = Vec::new(); + let mut bad_cells = Vec::with_capacity(tds.number_of_cells());
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.gitignore(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)WARP.md(1 hunks)benches/PERFORMANCE_RESULTS.md(4 hunks)benches/README.md(1 hunks)clippy.toml(1 hunks)cspell.json(3 hunks)rust-toolchain.toml(1 hunks)src/core/algorithms/bowyer_watson.rs(2 hunks)src/core/algorithms/robust_bowyer_watson.rs(1 hunks)src/core/boundary.rs(12 hunks)src/core/cell.rs(11 hunks)src/core/collections.rs(2 hunks)src/core/facet.rs(5 hunks)src/core/traits/boundary_analysis.rs(3 hunks)src/core/traits/data_type.rs(1 hunks)src/core/traits/insertion_algorithm.rs(6 hunks)src/core/triangulation_data_structure.rs(6 hunks)src/core/util.rs(3 hunks)src/core/vertex.rs(10 hunks)src/geometry/algorithms/convex_hull.rs(4 hunks)src/geometry/point.rs(12 hunks)src/geometry/predicates.rs(5 hunks)src/geometry/robust_predicates.rs(2 hunks)src/geometry/util.rs(1 hunks)src/lib.rs(1 hunks)tests/COVERAGE.md(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: acgetchell
PR: acgetchell/delaunay#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
PR: acgetchell/delaunay#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.mdsrc/geometry/util.rssrc/geometry/predicates.rssrc/geometry/algorithms/convex_hull.rssrc/core/boundary.rsCargo.tomlsrc/lib.rssrc/core/vertex.rssrc/geometry/robust_predicates.rstests/COVERAGE.mdsrc/core/cell.rsbenches/PERFORMANCE_RESULTS.mdsrc/core/algorithms/robust_bowyer_watson.rssrc/core/algorithms/bowyer_watson.rssrc/core/util.rssrc/core/triangulation_data_structure.rssrc/core/traits/insertion_algorithm.rs
📚 Learning: 2025-09-02T02:40:54.439Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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:
WARP.md
📚 Learning: 2025-08-30T02:40:22.032Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/util.rssrc/geometry/algorithms/convex_hull.rsCargo.tomlsrc/core/util.rs
📚 Learning: 2025-08-30T00:51:50.849Z
Learnt from: acgetchell
PR: acgetchell/delaunay#55
File: CONTRIBUTING.md:481-512
Timestamp: 2025-08-30T00:51:50.849Z
Learning: The DOI 10.5281/zenodo.16931097 in the delaunay project documentation is valid and correctly resolves to acgetchell's Rust delaunay triangulation library on Zenodo.
Applied to files:
Cargo.toml
📚 Learning: 2025-08-30T02:40:22.032Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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:
Cargo.toml
📚 Learning: 2025-08-28T14:12:46.198Z
Learnt from: acgetchell
PR: acgetchell/delaunay#55
File: CITATION.cff:5-7
Timestamp: 2025-08-28T14:12:46.198Z
Learning: The DOI 10.5281/zenodo.16931097 in the CITATION.cff file belongs to the Rust delaunay triangulation library by acgetchell, not to any Julia DelaunayTriangulation.jl package.
Applied to files:
Cargo.toml
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/point.rssrc/core/cell.rs
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/point.rs
🧬 Code graph analysis (12)
src/geometry/util.rs (2)
src/core/algorithms/robust_bowyer_watson.rs (2)
safe_usize_to_scalar(1032-1032)new(113-123)src/geometry/point.rs (2)
try_from(314-337)new(79-81)
src/geometry/algorithms/convex_hull.rs (5)
src/core/triangulation_data_structure.rs (4)
vertices(578-580)new(1899-1931)default(216-218)generation(934-936)src/core/algorithms/robust_bowyer_watson.rs (3)
new(113-123)default(77-79)geometry(2059-2059)src/core/algorithms/bowyer_watson.rs (2)
new(174-183)default(220-222)src/geometry/point.rs (3)
new(79-81)default(178-182)validate(96-108)src/core/traits/facet_cache.rs (1)
new(377-382)
src/core/boundary.rs (3)
src/core/util.rs (1)
derive_facet_key_from_vertices(370-403)src/core/facet.rs (5)
vertices(464-471)vertex(397-399)new(314-320)cell(352-354)facet_vertices(726-738)src/core/traits/boundary_analysis.rs (2)
boundary_facets(92-92)is_boundary_facet_with_map(195-199)
src/core/vertex.rs (5)
src/core/cell.rs (4)
hash(1126-1133)deserialize(283-373)map(346-346)from(377-379)src/core/facet.rs (4)
hash(529-532)deserialize(195-266)map(244-244)vertex(397-399)src/geometry/point.rs (6)
hash(136-138)serialize(190-216)deserialize(224-294)default(178-182)from(355-357)from(374-376)src/core/util.rs (2)
make_uuid(94-96)data(929-929)src/core/triangulation_data_structure.rs (2)
deserialize(3633-3714)default(216-218)
src/geometry/robust_predicates.rs (2)
src/geometry/point.rs (1)
new(79-81)src/geometry/traits/coordinate.rs (1)
new(643-643)
src/geometry/point.rs (2)
src/geometry/util.rs (8)
std(238-238)std(278-278)std(494-494)std(502-502)std(1971-1971)std(2059-2059)std(4561-4561)core(1388-1388)src/core/traits/insertion_algorithm.rs (2)
cast(729-729)cast(2729-2729)
src/core/cell.rs (2)
src/core/vertex.rs (3)
point(404-406)hash(660-664)partial_cmp(587-589)src/geometry/point.rs (2)
hash(136-138)partial_cmp(159-168)
src/core/algorithms/robust_bowyer_watson.rs (8)
src/core/traits/insertion_algorithm.rs (2)
new(281-293)new(420-427)src/core/triangulation_data_structure.rs (4)
new(1899-1931)vertices(578-580)cells(537-539)is_valid(3350-3386)src/core/algorithms/bowyer_watson.rs (1)
new(174-183)src/core/facet.rs (3)
new(314-320)vertices(464-471)vertex(397-399)src/core/cell.rs (3)
vertices(478-480)is_valid(909-945)facets(1056-1061)src/core/traits/boundary_analysis.rs (1)
boundary_facets(92-92)src/geometry/robust_predicates.rs (3)
general_triangulation(666-674)high_precision(690-699)degenerate_robust(715-724)src/core/vertex.rs (2)
None(1899-1899)is_valid(539-555)
src/core/algorithms/bowyer_watson.rs (5)
src/geometry/algorithms/convex_hull.rs (2)
test_default_implementation(2840-2859)default(1170-1176)src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/traits/insertion_algorithm.rs (3)
new(281-293)new(420-427)default(406-408)src/core/triangulation_data_structure.rs (3)
new(1899-1931)default(216-218)vertices(578-580)src/core/traits/facet_cache.rs (1)
new(377-382)
src/core/util.rs (3)
src/core/triangulation_data_structure.rs (1)
vertices(578-580)src/core/cell.rs (1)
vertices(478-480)src/core/facet.rs (2)
vertices(464-471)vertex(397-399)
src/core/triangulation_data_structure.rs (3)
src/core/traits/insertion_algorithm.rs (4)
fmt(38-52)default(406-408)new(281-293)new(420-427)src/core/algorithms/robust_bowyer_watson.rs (2)
default(77-79)new(113-123)src/core/algorithms/bowyer_watson.rs (2)
default(220-222)new(174-183)
src/core/traits/insertion_algorithm.rs (5)
src/core/algorithms/robust_bowyer_watson.rs (2)
geometry(2059-2059)new(113-123)src/core/algorithms/bowyer_watson.rs (1)
new(174-183)src/core/facet.rs (3)
new(314-320)facet_key_from_vertex_keys(601-613)vertex(397-399)src/geometry/point.rs (1)
new(79-81)src/geometry/traits/coordinate.rs (1)
new(643-643)
⏰ 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 (ubuntu-latest)
- GitHub Check: performance-regression
- GitHub Check: Codacy Security Scan
- GitHub Check: audit
🔇 Additional comments (55)
cspell.json (3)
204-204: "quickcheck" is appropriate (Rust crate/test term).
No issues; this reduces false positives in test code and docs.
212-212: "repr" is appropriate (Rust attribute).
Good addition; avoids churn from #[repr(...)] mentions in docs/examples.
92-92: Verify 'cdef' (and the other added words) are referenced outside cspell.jsonAutomated ripgrep returned "No files were searched" so usage couldn't be confirmed. Run locally:
rg -n -S -w 'cdef|conv|quickcheck|repr' --hidden --no-ignore-vcs -g '!cspell.json' -g '!/node_modules/' -g '!/dist/' -g '!/build/'
Keep entries that are actually referenced (e.g., Cython/docs); remove unused ones.
Cargo.toml (1)
5-5: Edition/MSRV alignment LGTM.Edition 2024 with rust-version 1.90.0 is consistent here and across configs.
src/geometry/util.rs (1)
1367-1432: Large grid generation: f32 index precision caveat.Even with the improved safe_usize_to_scalar, very large indices with
T=f32can still collapse distinct coordinates. Consider documenting or clampingpoints_per_dimforT=f32.src/geometry/robust_predicates.rs (2)
147-190: robust_insphere fallback chain is sensible.Nice layered strategy with adaptive tol, conditioning, and perturbation.
728-772: Tests: solid coverage and invariants.Nice spread over general, degenerate, and dimensional variants.
src/core/traits/data_type.rs (1)
37-57: Docs improvement is solid — referenced test exists.Test
vertex_string_data_usage_examplesis defined at src/core/vertex.rs:1625; leave the doc reference as-is.clippy.toml (1)
5-5: MSRV bump looks good — verify CI/devcontainer alignment
- Verified: rust-toolchain.toml (channel = "1.90.0"), Cargo.toml (rust-version = "1.90.0"), clippy.toml (msrv = "1.90.0").
- No .devcontainer directory found.
- No literal "1.90.0" found in .github/workflows — ensure workflows (or any CI steps using setup-rust / rust-version) are using 1.90.0 or rely on the repo rust-toolchain.
src/geometry/algorithms/convex_hull.rs (1)
4297-4316: Add compile‑time Send/Sync assertion for ConvexHull used across threadsThreaded test assumes
ConvexHullisSend + Sync. Add a compile-time check to catch regressions.// Share the hull and TDS across threads let hull = Arc::new(hull); let tds = Arc::new(tds); + + // Compile-time guarantee for concurrency + fn assert_send_sync<T: Send + Sync>() {} + assert_send_sync::<ConvexHull<f64, Option<()>, Option<()>, 3>>();src/lib.rs (1)
12-12: LGTM: feature wording matches DataType constraintsThe doc now reflects that vertex/cell payloads must be Copy-able; consistent with
DataType.Ensure README and crate-level docs elsewhere use the same wording to avoid mixed messaging.
README.md (1)
26-26: LGTM: clarified “Copy‑able data types” featureClear and accurate; aligns with
DataTypetrait bounds.src/core/algorithms/bowyer_watson.rs (5)
1094-1126: Default implementation test: LGTM.Covers Default::default parity with new() and stats initialization.
1186-1222: Standard strategy branch coverage: LGTM.Exercises the Standard path mapping to cavity-based; assertions are reasonable.
1224-1240: FacetCacheProvider tests: LGTM.Verifies empty cache and generation counter semantics.
1243-1301: InsertionAlgorithm trait method tests: LGTM.Good coverage of counters, strategy determination, and reset.
1302-1341: Error-path probing: LGTM.Non-strict assertions keep this diagnostic-focused while adding branch coverage.
src/core/facet.rs (1)
187-267: Manual Deserialize impl routed through Facet::new: LGTM.Correctly enforces invariants, handles duplicates, and ignores unknown fields.
src/core/traits/boundary_analysis.rs (2)
155-199: Result-returning is_boundary_facet_with_map: LGTM.Error-aware API is an improvement; docs/examples reflect new Result semantics.
196-200: All call sites handle Result — no changes required.
Repo search shows every usage of is_boundary_facet / is_boundary_facet_with_map either calls .expect()/unwrap() or checks .is_ok() before unwrapping; no raw-bool call sites found.src/geometry/point.rs (3)
159-168: PartialOrd implementation is correct and clearerLexicographic walk with ordered_partial_cmp and explicit Ordering import is clean and consistent with your OrderedEq semantics.
321-327: Good diagnostics on conversion failureIncluding concrete from_type/to_type via type_name improves error messages. LGTM.
536-546: Array::map usage is MSRV‑safe and idiomaticUsing
.map(Into::into)on arrays (including copies from&[T; N]) is fine with MSRV 1.90. Nice cleanup.Also applies to: 573-582, 1831-1838
src/geometry/predicates.rs (3)
257-266: Helpful doc note on parity-driven sign interpretationGood to document the even/odd dimension behavior for lifted determinants. This will save future confusion.
609-651: Parity mapping logic looks correct; run local insphere tests to confirm (3D–5D).rg shows test_insphere_methods_{2d,3d,4d,5d}_comprehensive in src/geometry/predicates.rs, but cargo test failed in the sandbox with: "Could not locate working directory: no /proc/self/exe available". Run locally to verify (once the even-dimension check compiles):
cargo test -q predicates::tests::test_insphere_methods_3d_comprehensive
predicates::tests::test_insphere_methods_4d_comprehensive
predicates::tests::test_insphere_methods_5d_comprehensiveOptional: enable the commented 2D std vs lifted assert locally to evaluate 2D parity.
599-603: Build break:usize::is_multiple_ofdoes not exist on stable
D.is_multiple_of(2)isn’t an inherent method onusize(MSRV 1.90). Use a modulo check.- let dimension_is_even = D.is_multiple_of(2); + let dimension_is_even = D % 2 == 0;Likely an incorrect or invalid review comment.
src/core/vertex.rs (6)
12-12: Doc clarity improvement LGTM.Explicitly stating U: DataType is helpful.
42-45: Serde import rework LGTM.Using IgnoredAny and explicit de::* is idiomatic.
218-219: Visitor PhantomData usage LGTM.Avoids unused generic warnings and keeps types anchored.
Also applies to: 300-301
269-270: Unknown-field handling is correct.Ignoring with IgnoredAny keeps forward compatibility.
1069-1092: Collection semantics tests LGTM.Good real-world checks for Eq/Hash with FastHash{Set,Map} and mixed U types.
Also applies to: 1096-1119, 1127-1135, 1735-1743
2066-2090: Duplicate nil-UUID test — remove or change.validate_uuid (src/core/util.rs:64) only rejects nil UUIDs (UuidValidationError::NilUuid); uuid::Uuid::from_bytes([0;16]) produces the nil UUID, so this test duplicates the existing nil-UUID case. Either remove the test, construct a non‑nil but invalid UUID (e.g., use a v1/v3/v5 sample) if you intend to reject non‑v4 versions, or update validate_uuid to enforce version checks.
src/core/boundary.rs (3)
184-190: Good: delegation with error propagation.
is_boundary_facetnow builds the map once and delegates, returning a Result. This aligns with the new API and keeps callers honest about failures.
241-258: Good: derive key + precise error mapping.Using
derive_facet_key_from_vertices(...)?andis_some_and(|cells| cells.len() == 1)is clear and correct. The debug assertion on vertex count is a nice sanity check.Please confirm
Facet::vertices()is guaranteed to return exactly D vertices across dimensions (D ≥ 1) under all constructors, including degenerate cells.
1694-1727: Cross‑TDS facet error path test is solid.Great coverage for the vertex‑UUID mismatch by using a facet from another TDS; this validates the new error propagation.
src/core/util.rs (5)
291-315: Stable hash utility looks correct and deterministic.Constants match FNV-1 64-bit, avalanche step applied; empty-slice behavior documented and tested. LGTM.
191-242: generate_combinations implementation is correct; edge cases covered.Iterative next-combination logic handles k=0, k>n, k=n. Tests exercise termination and index bump paths.
370-403: Facet key derivation is clear; stack buffer avoids heap churn.LGTM. Docs note empty input → 0, and error path on unknown vertex UUIDs matches tests.
810-831: Good additions to cover combination edge cases and larger paths.Expanded tests materially improve branch coverage in
generate_combinations.Also applies to: 833-876
64-76: No action required — uuid 1.18.1 exposesget_version_numas aconst fn.Cargo.toml pins
uuid = "1.18.1"and the crate docs showpub const fn get_version_num(&self) -> usize, so calling it from yourconst fnis valid. (docs.rs)src/core/triangulation_data_structure.rs (5)
157-167: Good aliasing of Ordering to avoid atomic::Ordering collisions.Using
Ordering as CmpOrderingis the right call given the concurrent use ofatomic::Orderingbelow. No action needed.
191-199: Trait in scope for extension methods: keep InsertionAlgorithm imported.Bringing
InsertionAlgorithminto scope is necessary for theinsert_vertex/triangulatecalls. Looks correct and consistent with the new public exposure.
7688-7820: Nice additional coverage of error displays and enums.Good targeted tests for Display and enum defaults; they’ll catch regressions in error formatting.
5311-5350: Insufficient vertices path: Tds::new correctly propagates InsufficientVertices.Verified: the insertion algorithm returns TriangulationConstructionError::InsufficientVertices when vertices.len() < D+1 (src/core/traits/insertion_algorithm.rs:1443–1449), and Tds::new surfaces that error — tests inspect the propagated error and its CellValidationError::InsufficientVertices source (src/core/triangulation_data_structure.rs:5740–5750).
536-581: Sanity-check MSRV 1.90 — verify const accessors compileAttempted to run the supplied rustup script in the sandbox but execution failed (no /proc/self/exe). Run locally with Rust 1.90.0 to confirm the pub const fn vertices()/cells() returning &SlotMap<...> compile under MSRV 1.90.0.
Command: rustup toolchain install 1.90.0 && cargo +1.90.0 test --all --quiet
Applies to: src/core/triangulation_data_structure.rs lines 536-581 and 815-907
src/core/cell.rs (5)
50-58: Serde and std import cleanup looks goodDirectly importing Deserializer, Visitor, MapAccess, IgnoredAny and using cmp alias improves readability and keeps bounds local to this module.
Also applies to: 59-65
283-296: Manual Deserialize: solid implementation; unknown fields correctly ignored
- Requiring "vertices" and "uuid" while defaulting "neighbors"/"data" is consistent with your JSON round‑trip tests.
- Using IgnoredAny for unknown keys matches the documented behavior in tests.
Please confirm this stricter requirement for "uuid" (must be present when deserializing) is intentional versus generating a fresh UUID if missing (builder default). If you want “uuid” optional, I can provide a minimal patch.
Also applies to: 346-346, 365-373
409-414: Duplicate detection switched to FastHashSet — good moveThis aligns builder validation with is_valid() and leverages your faster hash set.
919-922: Distinct-vertex check mirrors builder validationThe FastHashSet-based check is correct and consistent with Vertex’s Eq/Hash contract.
1091-1094: PartialOrd return type via cmp::OrderingThe change to Optioncmp::Ordering is purely cosmetic and fine; matches your imports.
src/core/traits/insertion_algorithm.rs (5)
7-7: Introduce SmallBuffer/CellKeySet — good directionShifting to SmallBuffer (smallvec-like) is a sensible perf win for hot buffers.
449-475: Buffer prepare methods look correctClearing only the target buffer and returning &mut SmallBuffer promotes reuse without side effects on others.
806-841: Bad‑cell detection logic and 50% threshold optimizationGood use of preallocated vertex_points and integer math for 0.5 threshold. Semantics are “> 50%” (exactly 50% passes).
Please confirm “equal to 50% degenerate” should NOT trigger TooManyDegenerateCells. If it should, switch to >= in the integer branch.
Also applies to: 872-898
2018-2028: Comprehensive error types and stats APIs look solidNon‑exhaustive enums, helper constructors, and stats helpers are well factored.
420-426: new() changed from const → fn — verify const/static initializersNon-const new() can break const/static initializations; confirm there are no const/static items that call InsertionBuffers::new().
Location: src/core/traits/insertion_algorithm.rs:420–426
Automated ripgrep search returned no matches. Run these local checks and act accordingly (restore const fn or update initializers to lazy/ runtime init):
rg -n --type=rust 'InsertionBuffers::new' -S
rg -nP --type=rust '\b(?:const|static)\b[^\n]*InsertionBuffers' -C3
src/core/boundary.rs
Outdated
| let wrong_size_vertices = vec![vertex!([0.0, 0.0, 0.0]), vertex!([1.0, 0.0, 0.0])]; // Only 2 vertices for 3D facet (should be 3) | ||
|
|
||
| // Test derive_facet_key_from_vertices with wrong number of vertices | ||
| let error_result = derive_facet_key_from_vertices(&wrong_size_vertices, &tds); | ||
| assert!( | ||
| error_result.is_err(), | ||
| "derive_facet_key_from_vertices should fail for wrong number of vertices" | ||
| ); | ||
|
|
||
| if let Err(e) = error_result { | ||
| println!(" Got expected error for wrong-sized vertex list: {e}"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Test expectation contradicts util’s contract; wrong-size vertices currently succeed.
derive_facet_key_from_vertices allows any count (empty → Ok(0)), so expecting Err for 2 vertices in 3D is inconsistent and will fail.
Two options:
- Align the test to current behavior:
- let error_result = derive_facet_key_from_vertices(&wrong_size_vertices, &tds);
- assert!(
- error_result.is_err(),
- "derive_facet_key_from_vertices should fail for wrong number of vertices"
- );
+ let res = derive_facet_key_from_vertices(&wrong_size_vertices, &tds);
+ assert!(res.is_ok(), "Current utility accepts non-D vertex counts");- Or, if you want strict facet sizing, enforce
len == D || len == 0in the utility and return a dedicated error; then keep this test.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/boundary.rs around lines 1084 to 1096, the test expects
derive_facet_key_from_vertices to reject a vertex list of length 2 for a 3D
facet, but the util currently accepts any length; to fix, update
derive_facet_key_from_vertices to validate the incoming vertices vector length
and return an Err when its length is not equal to the dimensionality D (or zero
if you want to allow empty → Ok(0)); implement a clear, dedicated error
variant/message for wrong-size vertex lists, use that error in the function
return, and keep the existing test as-is so it now observes the expected Err for
the 2-vertex case.
There was a problem hiding this comment.
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 (4)
src/core/algorithms/bowyer_watson.rs (1)
288-313: Standard strategy has no fallback path after primary failureIf determine_insertion_strategy returns Standard and the first cavity-based attempt fails, we never try HullExtension nor Fallback, unlike the CavityBased branch. This can hide recoverable insertions.
Apply this diff to add symmetric fallback for Standard:
- if result.is_err() { + if result.is_err() { match strategy { + InsertionStrategy::Standard => { + // Mirror the CavityBased fallback chain + result = <Self as InsertionAlgorithm<T, U, V, D>>::insert_vertex_hull_extension( + self, tds, &vertex, + ); + if result.is_err() { + result = <Self as InsertionAlgorithm<T, U, V, D>>::insert_vertex_fallback( + self, tds, &vertex, + ); + } + } InsertionStrategy::CavityBased => {src/geometry/predicates.rs (1)
399-424: insphere() lacks the parity-aware sign handling promised in docsCurrent logic only branches on Orientation; even/odd dimension parity isn’t accounted for, diverging from insphere_lifted and the docstring above.
Apply this parity-aware mapping to match insphere_lifted:
- match orientation { + let dimension_is_even = D % 2 == 0; + match orientation { Orientation::DEGENERATE => Err(CoordinateConversionError::ConversionFailed { coordinate_index: 0, coordinate_value: "degenerate simplex".to_string(), from_type: "simplex", to_type: "circumsphere containment", }), - Orientation::POSITIVE => { - if det > tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det < -tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } - Orientation::NEGATIVE => { - if det < -tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det > tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } + Orientation::POSITIVE => { + if dimension_is_even { + if det < -tolerance_f64 { Ok(InSphere::INSIDE) } + else if det > tolerance_f64 { Ok(InSphere::OUTSIDE) } + else { Ok(InSphere::BOUNDARY) } + } else { + if det > tolerance_f64 { Ok(InSphere::INSIDE) } + else if det < -tolerance_f64 { Ok(InSphere::OUTSIDE) } + else { Ok(InSphere::BOUNDARY) } + } + } + Orientation::NEGATIVE => { + if dimension_is_even { + if det > tolerance_f64 { Ok(InSphere::INSIDE) } + else if det < -tolerance_f64 { Ok(InSphere::OUTSIDE) } + else { Ok(InSphere::BOUNDARY) } + } else { + if det < -tolerance_f64 { Ok(InSphere::INSIDE) } + else if det > tolerance_f64 { Ok(InSphere::OUTSIDE) } + else { Ok(InSphere::BOUNDARY) } + } + } }src/core/triangulation_data_structure.rs (1)
2111-2116: Missing generation bump when adding vertices before first simplex (stale caches).If
vertex_count < D + 1,add()returns early without callingbump_generation(), leaving cache consumers on the previous generation despite a structural change (vertex set changed). This can cause stale reads.Apply this fix to bump the generation in the early-return path:
- if vertex_count < D + 1 { - // Not enough vertices yet for a D-dimensional triangulation - // Just store the vertex without creating any cells - return Ok(()); - } + if vertex_count < D + 1 { + // Not enough vertices yet for a D-dimensional triangulation. + // Vertex set changed; invalidate dependent caches. + self.bump_generation(); + return Ok(()); + }src/core/util.rs (1)
64-77:validate_uuidshould not beconst fn(likely MSRV break).
Uuid::is_nil()/get_version_num()are not guaranteed const on Rust 1.90/uuid 1.x. Marking thisconst fnrisks a compile error.Apply this diff:
-pub const fn validate_uuid(uuid: &Uuid) -> Result<(), UuidValidationError> { +pub fn validate_uuid(uuid: &Uuid) -> Result<(), UuidValidationError> {
🧹 Nitpick comments (29)
.gitignore (1)
21-21: Ignore the whole coverage_report directory, not just specific files.Future tarpaulin versions or CI steps may emit additional artifacts (subdirs, CSS). Ignoring the directory is safer.
Apply:
-coverage_report/tarpaulin-report.* +coverage_report/WARP.md (2)
153-156: Tarpaulin usage: add the install hint and keep the output path note.Many environments won’t have tarpaulin preinstalled. A one-liner helps onboarding.
-# Test coverage analysis (excludes benchmarks, examples, and integration tests) -cargo tarpaulin --exclude-files 'benches/**' --exclude-files 'examples/**' --exclude-files 'tests/**' --out Html --output-dir target/tarpaulin -# View coverage report: open target/tarpaulin/tarpaulin-report.html +# Test coverage analysis (excludes benches, examples, integration tests) +# If needed: cargo install cargo-tarpaulin +cargo tarpaulin \ + --exclude-files 'benches/**' \ + --exclude-files 'examples/**' \ + --exclude-files 'tests/**' \ + --out Html --output-dir target/tarpaulin +# View: open target/tarpaulin/tarpaulin-report.html
202-204: Doc MSRV is stale (says 1.89.0). Update to 1.90.0.Ensures internal guidance matches the new pin.
-- **Language**: Rust (MSRV 1.89.0, Edition 2024) +- **Language**: Rust (MSRV 1.90.0, Edition 2024)src/core/vertex.rs (4)
274-291: Validate UUID during deserialization to reject invalid/nil UUIDs early.Currently you validate
pointbut accept anyuuid. Validating here tightens invariants and avoids constructing invalid vertices.let point = point.ok_or_else(|| de::Error::missing_field("point"))?; - let uuid = uuid.ok_or_else(|| de::Error::missing_field("uuid"))?; + let uuid = uuid.ok_or_else(|| de::Error::missing_field("uuid"))?; + // Validate UUID immediately + if let Err(e) = validate_uuid(&uuid) { + return Err(de::Error::custom(format!( + "Invalid UUID during deserialization: {e}" + ))); + } let incident_cell = incident_cell.unwrap_or(None); let data = data.unwrap_or(None);
691-716: Prefer deriving serde for PointType (or use serde_repr) to reduce boilerplate.Manual impls are correct but verbose and easy to drift. If you need stable u8 encoding,
serde_repris concise.Option A (no new dep, string variants acceptable):
-#[repr(u8)] -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[repr(u8)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] enum PointType { ... } -impl Serialize for PointType { ... } -impl<'de> Deserialize<'de> for PointType { ... }Option B (u8 wire format, add dev-dep):
# Cargo.toml (dev-dependencies) serde_repr = "0.1"-#[repr(u8)] -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[repr(u8)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, serde_repr::Serialize_repr, serde_repr::Deserialize_repr)] enum PointType { ... }
1243-1261: Simplify ordering test to assert equality directly.Multiple “Less|Equal”/“Greater|Equal” checks obscure intent. A single equality assertion is clearer.
-// Test that equal points result in equal ordering -assert!(vertex1.partial_cmp(&vertex2) != Some(Ordering::Less)); -assert!(vertex2.partial_cmp(&vertex1) != Some(Ordering::Less)); -assert!(matches!(vertex1.partial_cmp(&vertex2), Some(Ordering::Less | Ordering::Equal))); -assert!(matches!(vertex2.partial_cmp(&vertex1), Some(Ordering::Less | Ordering::Equal))); -assert!(matches!(vertex1.partial_cmp(&vertex2), Some(Ordering::Greater | Ordering::Equal))); -assert!(matches!(vertex2.partial_cmp(&vertex1), Some(Ordering::Greater | Ordering::Equal))); +// Equal points should compare equal in both directions +assert_eq!(vertex1.partial_cmp(&vertex2), Some(Ordering::Equal)); +assert_eq!(vertex2.partial_cmp(&vertex1), Some(Ordering::Equal));
2069-2090: This “invalid version” test is effectively another nil-UUID case.
Uuid::from_bytes([0;16])yields the nil UUID already covered bytest_set_uuid_nil_uuid. Consider removing or crafting a truly invalid UUID scenario if your validator distinguishes versions.-#[test] -fn test_set_uuid_invalid_version() { - // ... - let uuid_bytes = [0u8; 16]; // This creates a nil-like UUID - let invalid_uuid = uuid::Uuid::from_bytes(uuid_bytes); - // ... -} +// Removed redundant test; covered by test_set_uuid_nil_uuid.benches/README.md (1)
119-127: Unify CLI invocation style for benchmark-utils.Consider using uvx for tool-style invocations to avoid accidental virtualenv state and make commands copy/pasteable across environments.
Apply this diff if you prefer the tool runner:
-uv run benchmark-utils generate-summary +uvx benchmark-utils generate-summary -uv run benchmark-utils generate-summary --run-benchmarks +uvx benchmark-utils generate-summary --run-benchmarks -uv run benchmark-utils compare --baseline baseline-artifact/baseline_results.txt +uvx benchmark-utils compare --baseline baseline-artifact/baseline_results.txtsrc/geometry/algorithms/convex_hull.rs (4)
190-210: Trim overly strict generic bounds to reduce compile-time and friction.Several bounds (ComplexField, From, f64: From, OrderedFloat conversions, reference Div) are unused in this impl and constrain T unnecessarily.
Apply this diff to the where-clause:
impl<T, U, V, const D: usize> ConvexHull<T, U, V, D> where - T: CoordinateScalar - + AddAssign<T> - + SubAssign<T> - + Sub<Output = T> - + DivAssign<T> - + Zero - + One - + NumCast - + Copy - + Sum - + ComplexField<RealField = T> - + From<f64>, + T: CoordinateScalar + + AddAssign<T> + + SubAssign<T> + + Sub<Output = T> + + DivAssign<T> + + Zero + + One + + NumCast + + Copy + + Sum, U: DataType, V: DataType, - [T; D]: Copy + Default + Sized + Serialize + DeserializeOwned, - f64: From<T>, - for<'a> &'a T: std::ops::Div<T>, - OrderedFloat<f64>: From<T>, + [T; D]: Copy + Default + Sized + Serialize + DeserializeOwned, {Please confirm downstream call sites and tests still compile with MSRV 1.90.0 after this relaxation.
432-441: Misleading error mapping when facet vertex key lookup fails.Mapping a missing facet-vertex key to InsideVertexNotFound obscures root cause.
Prefer a dedicated Facet/vertex-key lookup error (e.g., FacetVertexKeyNotFoundInTds { uuid }) or, minimally, wrap InsufficientVertices/KeyNotFound with the offending UUID for diagnosis.
583-595: Make epsilon scale with scalar type.Hardcoding 1e-12 biases f32/f64 differently.
Use a type-relative epsilon (e.g., cast of f64::EPSILON only if T lacks a native epsilon), or accept an epsilon parameter for tests to tune per-type. Current code partly does this; consider removing the 1e-12 override and relying on NumCast::from(f64::EPSILON) fallback only.
3988-4053: Avoid relying on deprecated cache API in tests.Tests use get_or_build_facet_cache with #[allow(deprecated)]. Prefer exercising the non-deprecated try_get_or_build_facet_cache path to keep tests aligned with the public surface.
Replace deprecated calls where feasible; keep one targeted test to assert deprecation-compat if you must.
tests/COVERAGE.md (1)
37-44: Section label/thresholds are inconsistent with listed entries.“Low Coverage (<40%)” includes 60.23% and 50.00% rows.
Apply this diff to clarify the grouping:
-### Low Coverage Files (<40%) - **Priority for Improvement** +### Priority Focus Files (coverage < 65% or high-impact)benches/PERFORMANCE_RESULTS.md (3)
137-144: Resolve narrative inconsistency with 100% agreement above.The “Method Disagreements” section conflicts with the 100% agreement reported.
Apply this diff to reflect current data while retaining caveat language:
-### Method Disagreements - -The disagreements between methods are expected due to: - -1. Different numerical approaches and tolerances -2. Floating-point precision differences in multi-step calculations -3. Varying sensitivity to degenerate cases +### Numerical agreement caveats + +Current run shows 100% agreement across methods. In general, minor disagreements can occur due to: +1. Different numerical approaches and tolerances +2. Floating-point precision in multi-step calculations +3. Sensitivity to near-degenerate cases
7-13: Name the generator consistently with benches/README.md.This page says “Generated By: benchmark_utils.py”, while README uses the benchmark-utils CLI.
If the CLI is the canonical interface, update the label accordingly:
-**Generated By**: benchmark_utils.py +**Generated By**: benchmark-utils
96-98: Clarify aggregation basis for the summary latencies.State whether these are medians across dimensions/cases or a representative scenario to avoid misinterpretation.
README.md (1)
26-26: Clarify “Copy” wording (minor doc nit).“Copy-able” can be read informally; consider naming the Rust trait explicitly for precision.
Apply this diff:
-- [x] Copy-able data types associated with vertices and cells (integers, floats, chars, custom enums) +- [x] Copy data types (Rust `Copy` trait) for vertices and cells (integers, floats, chars, custom enums)src/core/collections.rs (1)
919-953: Add spilled()/stack vs heap assertions (optional).Also validate transition behavior for 2- and 16-capacity helpers when exceeding inline capacity.
Apply this diff within
test_small_buffer_utility_functions_comprehensive:@@ let mut test_buffer2: SmallBuffer<f64, 2> = small_buffer_with_capacity_2(3); test_buffer2.push(1.0); test_buffer2.push(2.0); assert_eq!(test_buffer2.len(), 2); + assert!(!test_buffer2.spilled()); + test_buffer2.push(3.0); + assert!(test_buffer2.spilled()); @@ let mut test_buffer16: SmallBuffer<char, 16> = small_buffer_with_capacity_16(5); test_buffer16.push('a'); test_buffer16.push('b'); assert_eq!(test_buffer16.len(), 2); + assert!(!test_buffer16.spilled()); + for ch in ['c','d','e','f','g','h','i','j','k','l','m','n','o','p','q'] { + test_buffer16.push(ch); + } + assert!(test_buffer16.spilled(), "Should spill past inline 16")src/geometry/util.rs (1)
893-902: Fix facet_measure docs to match implementation.Docs list is off for D=1/2 relative to code (1D returns 1; 2D computes segment length). Align bullets.
Apply this diff to the facet_measure doc block:
-/// - 1D: Distance between two points (length) -/// - 2D: Area of triangle using cross product -/// - 3D: Volume of tetrahedron using scalar triple product +/// - 1D: 0‑dimensional facet in 1D (single point) — conventionally 1 +/// - 2D: Distance between two points (segment length) +/// - 3D: Area of triangle using cross product +/// - 4D: Volume of tetrahedron (3D facet) via Gram matrixAlso applies to: 955-999
src/geometry/robust_predicates.rs (1)
578-607: Deterministic tie-breaker: add “order-dependent” note (optional).Add a brief doc note that outcome depends on point order; acceptable for last-resort progress.
/// Deterministic tie-breaking for degenerate cases. +/// Note: Result is order-dependent on `simplex_points` (used only as a last resort). fn deterministic_tie_breaking<T, const D: usize>(...src/geometry/predicates.rs (1)
655-671: Test additions look good; consider re‑enabling 2D equivalence asserts after parity fixOnce insphere parity is fixed, the commented 2D assertions can likely be restored to strengthen coverage.
Also applies to: 672-681, 1195-1211, 1213-1241, 1243-1269, 1271-1307, 1386-1426, 1429-1441, 1444-1480, 1482-1518, 1521-1559, 1561-1599, 1601-1637, 1639-1695
src/core/triangulation_data_structure.rs (4)
2094-2102: O(n) duplicate-coordinate check will scale poorly. Consider an index.For heavy incremental builds, scanning all vertices each insert will be quadratic. Recommend maintaining a
FastHashSet<[T; D]>(or quantized key if you need tolerance) gated behind a feature flag to keep current strict-equality behavior.If you want a minimal internal index:
+ /// Optional coordinate index for O(1) duplicate checks (exact equality). + #[serde(skip)] + coord_index: Option<FastHashSet<[T; D]>>, @@ - let new_coords: [T; D] = (&vertex).into(); + let new_coords: [T; D] = (&vertex).into(); - for val in self.vertices.values() { - let existing_coords: [T; D] = val.into(); - if existing_coords == new_coords { - return Err(TriangulationConstructionError::DuplicateCoordinates { - coordinates: format!("{new_coords:?}"), - }); - } - } + if let Some(idx) = &self.coord_index { + if idx.contains(&new_coords) { + return Err(TriangulationConstructionError::DuplicateCoordinates { + coordinates: format!("{new_coords:?}"), + }); + } + } else { + // fallback for now + for val in self.vertices.values() { + let existing_coords: [T; D] = val.into(); + if existing_coords == new_coords { + return Err(TriangulationConstructionError::DuplicateCoordinates { + coordinates: format!("{new_coords:?}"), + }); + } + } + } @@ - self.uuid_to_vertex_key.insert(uuid, key); + self.uuid_to_vertex_key.insert(uuid, key); + if let Some(idx) = &mut self.coord_index { + idx.insert(new_coords); + }
1617-1634: Avoid heap allocation for neighbor keys in hot path.
Vec<Option<CellKey>>allocates per call. You already useSmallBufferelsewhere; return aSmallBuffer<Option<CellKey>, MAX_PRACTICAL_DIMENSION_SIZE>here for stack-backed storage up to practical D.- pub fn find_neighbors_by_key(&self, cell_key: CellKey) -> Vec<Option<CellKey>> { - let mut neighbor_keys = vec![None; D + 1]; + pub fn find_neighbors_by_key( + &self, + cell_key: CellKey, + ) -> SmallBuffer<Option<CellKey>, MAX_PRACTICAL_DIMENSION_SIZE> { + let mut neighbor_keys = SmallBuffer::with_capacity(D + 1); + neighbor_keys.resize(D + 1, None);
1725-1746: This lookup is O(#cells×D). Consider a vertex→cells cache for queries.For repeated queries, keeping a maintained
VertexToCellsMap(like you build inassign_incident_cells) would reduce this to O(k).
3539-3609: Equality sorting usespartial_cmpon float arrays; NaN note acknowledged.Given construction rejects NaNs,
unwrap_or(CmpOrdering::Equal)is acceptable. Consider documenting that equality is by coordinates/vertex sets and ignores UUIDs/caches, which you already hint at.src/core/algorithms/robust_bowyer_watson.rs (1)
1456-1492: Tests are very chatty; consider reducing stdout noise.The numerous
println!calls make CI logs noisy and slow. Prefer assertions only, or gate logs behind a feature/env flag.Also applies to: 1508-1516, 1536-1558, 1591-1601, 1610-1643, 1685-1698, 1719-1731, 1754-1767, 1783-1794, 1831-1869, 1881-1893, 1907-1919, 1942-1953, 1993-2005, 2070-2091, 2140-2164
src/geometry/point.rs (3)
242-294: Deserialize ties to serde_json::Value; non‑JSON formats may surprise.Using serde_json::Value inside a generic Deserialize impl works for JSON but can behave unexpectedly for other formats (e.g., bincode). If non‑JSON formats are in scope, consider decoding via a small enum (Number/String/Null) with generic serde instead of json::Value.
1766-1785: Update this test’s expectation to current Deserialize behavior.Comment says null→NaN “may fail”, but Deserialize now maps null→NaN. The test prints instead of asserting, which can mask regressions. Recommend asserting success (or remove the test if superseded by point_deserialize_null_maps_to_nan).
- let result: Result<Point<f64, 3>, _> = serde_json::from_str(json_with_nulls); - // This should fail unless we implement special handling for null -> NaN/infinity - match result { - Ok(_) => { - println!("Deserialization of null succeeded unexpectedly"); - } - Err(e) => { - println!("Expected deserialization error: {e}"); - } - } + let p: Point<f64, 3> = serde_json::from_str(json_with_nulls).unwrap(); + let coords = p.to_array(); + assert!(coords[0].is_nan()); + assert_eq!(coords[1], 1.0); + assert_eq!(coords[2], 2.0);
2690-2693: Minor: explicit type on HashSet can aid inference noise.Current code works; if inference ever struggles, add the type:
let mut set: HashSet<Point<f64, 3>> = HashSet::new();
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.gitignore(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)WARP.md(1 hunks)benches/PERFORMANCE_RESULTS.md(4 hunks)benches/README.md(1 hunks)clippy.toml(1 hunks)cspell.json(3 hunks)rust-toolchain.toml(1 hunks)src/core/algorithms/bowyer_watson.rs(2 hunks)src/core/algorithms/robust_bowyer_watson.rs(1 hunks)src/core/boundary.rs(12 hunks)src/core/cell.rs(11 hunks)src/core/collections.rs(2 hunks)src/core/facet.rs(5 hunks)src/core/traits/boundary_analysis.rs(3 hunks)src/core/traits/data_type.rs(1 hunks)src/core/traits/insertion_algorithm.rs(6 hunks)src/core/triangulation_data_structure.rs(6 hunks)src/core/util.rs(3 hunks)src/core/vertex.rs(10 hunks)src/geometry/algorithms/convex_hull.rs(4 hunks)src/geometry/point.rs(12 hunks)src/geometry/predicates.rs(5 hunks)src/geometry/robust_predicates.rs(2 hunks)src/geometry/util.rs(1 hunks)src/lib.rs(1 hunks)tests/COVERAGE.md(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-30T02:40:22.032Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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.mdsrc/lib.rsCargo.toml
📚 Learning: 2025-09-04T20:03:49.896Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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.mdsrc/lib.rsCargo.tomlbenches/PERFORMANCE_RESULTS.mdsrc/core/traits/insertion_algorithm.rssrc/geometry/robust_predicates.rssrc/core/algorithms/bowyer_watson.rssrc/geometry/predicates.rssrc/core/util.rssrc/core/vertex.rstests/COVERAGE.mdsrc/geometry/util.rssrc/core/triangulation_data_structure.rssrc/core/algorithms/robust_bowyer_watson.rssrc/core/boundary.rssrc/geometry/algorithms/convex_hull.rssrc/core/cell.rs
📚 Learning: 2025-09-02T02:40:54.439Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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:
WARP.md
📚 Learning: 2025-08-30T00:51:50.849Z
Learnt from: acgetchell
PR: acgetchell/delaunay#55
File: CONTRIBUTING.md:481-512
Timestamp: 2025-08-30T00:51:50.849Z
Learning: The DOI 10.5281/zenodo.16931097 in the delaunay project documentation is valid and correctly resolves to acgetchell's Rust delaunay triangulation library on Zenodo.
Applied to files:
Cargo.toml
📚 Learning: 2025-08-30T02:40:22.032Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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:
Cargo.tomlsrc/core/util.rssrc/geometry/util.rssrc/geometry/algorithms/convex_hull.rs
📚 Learning: 2025-08-28T14:12:46.198Z
Learnt from: acgetchell
PR: acgetchell/delaunay#55
File: CITATION.cff:5-7
Timestamp: 2025-08-28T14:12:46.198Z
Learning: The DOI 10.5281/zenodo.16931097 in the CITATION.cff file belongs to the Rust delaunay triangulation library by acgetchell, not to any Julia DelaunayTriangulation.jl package.
Applied to files:
Cargo.toml
📚 Learning: 2025-09-02T20:32:05.985Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/point.rssrc/core/cell.rs
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/point.rs
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (63)
Cargo.toml (1)
5-5: Cargo MSRV set to 1.90.0 — LGTM.Aligned with rust-toolchain.toml and clippy.toml. No action needed.
clippy.toml (1)
5-5: Clippy MSRV updated to 1.90.0 — LGTM.Keeps lint expectations consistent with the toolchain pin.
src/core/vertex.rs (1)
142-142: DataType already requires Copy — no change needed.
DataType is declared to include Copy (src/core/traits/data_type.rs:57–58), so U: Copy is enforced and Vertex<T, U, D> deriving Copy is valid.rust-toolchain.toml (1)
3-3: MSRV bump to 1.90.0 — verify repo-wide referencesMatches Cargo.toml and clippy.toml in this PR’s scope; automated search in the sandbox failed ("No files were searched"). Run locally to confirm no lingering 1.89.0 / MSRV mentions:
# search tracked files git grep -n -e '1\.89(\.0)?' -e 'MSRV[^0-9]*1\.89' || true # fallback (search all files) grep -RIn --exclude-dir=.git --exclude-dir=target -E '1\.89(\.0)?|MSRV[^0-9]*1\.89' . || truesrc/geometry/algorithms/convex_hull.rs (1)
1133-1140: Cache invalidation semantics look good.Release on store with ArcSwapOption is appropriate given readers will Acquire-load via the builder path.
Ensure try_get_or_build_facet_cache uses Acquire when reading cached_generation to complete the HB relation.
tests/COVERAGE.md (1)
1-8: Regenerate numbers to avoid drift.Since this is derived output, please ensure the figures reflect the latest CI run (after adding the extensive ConvexHull tests).
Confirm by re-running your coverage job and updating this snapshot (or link to the CI artifact instead of committing generated data).
src/lib.rs (1)
12-12: Nice clarification on Copy-able data types.Consider linking to the Copy trait for quick reference.
Apply this doc tweak if desired:
-//! - Copy-able data types associated with vertices and cells (see [`DataType`](core::traits::DataType) for constraints) +//! - Data types associated with vertices and cells must implement the [`Copy`](https://doc.rust-lang.org/std/marker/trait.Copy.html) trait (see [`DataType`](core::traits::DataType) for additional constraints)benches/README.md (1)
119-127: Confirm benchmark-utils is available in CI and local PATH.File: benches/README.md (lines 119–127).
Attempted verification with
uvx -q benchmark-utils --versionfailed in the verification environment (uvx: command not found). Ensure the CLI invoked by the README (uv/uvx/benchmark-utils) is installed and on PATH in both local dev and CI. Verify in each environment with:
- command -v uv || command -v uvx || command -v benchmark-utils
- uv run benchmark-utils --version || uvx -q benchmark-utils --version
src/core/collections.rs (1)
829-836: Nice coverage for SmallBuffer helpers.Tests exercise zero-capacity and type polymorphism well.
Also applies to: 919-953
src/geometry/util.rs (2)
150-175: format_bytes fallback path is sane.Graceful fallback when conversion fails; human-readable units look good.
1189-1251: rand API usage — verified; no changes required.
Cargo.toml pins rand = "0.9.2", and rand::rng()/Rng::random_range are present in rand 0.9.x, so the usage in generate_random_points is valid.src/geometry/robust_predicates.rs (2)
497-525: Consistency check fallback is conservative — good.Treating Unverifiable as consistent avoids false negatives in degenerate cases.
731-754: Robust tests + Display checks LGTM.Good coverage and minimal flakiness risk.
Also applies to: 1293-1306
cspell.json (1)
92-93: Dictionary additions LGTM — confirm/remove "conv".'conv' only appears in cspell.json (line 111) and has no other repo matches; it's very short and can mask typos (e.g., "convience") — remove it unless it's an intentional token used by the project.
src/core/traits/boundary_analysis.rs (1)
155-163: Document breaking API change and bump semver; internal callers updated
- The API now returns Result<bool, TriangulationValidationError>. Internal call sites/tests were already updated (see src/core/boundary.rs and src/core/triangulation_data_structure.rs — many calls use
.expect(...)) and docs/OPTIMIZING_BOWYER_WATSON.md documents the change.- Action: add a CHANGELOG entry and bump the crate version (major) to signal the breaking change.
- Optional: provide a transitional helper for convenience:
fn is_boundary_facet_or_false(&self, facet: &Facet<T, U, V, D>) -> bool { self.is_boundary_facet(facet).unwrap_or(false) }src/core/traits/data_type.rs (1)
37-41: Approve — docs align with Copy constraint; referenced test present. The testvertex_string_data_usage_examplesexists at src/core/vertex.rs:1625 — ensure it stays green.src/core/facet.rs (3)
66-69: Serde import consolidation LGTMSwitching to prelude imports (Deserialize, Deserializer, Serialize, de::*) and PhantomData is clean and idiomatic.
Also applies to: 71-75
187-207: Manual Deserialize is correct and robust
- Properly validates required fields, rejects duplicates, and ignores unknowns via IgnoredAny.
- Calling Facet::new(cell, vertex) ensures invariants at deserialization time.
Please confirm all serde bounds on [T; D] are truly needed here (DeserializeOwned + Serialize). If they’re only required by Cell/Vertex, we could narrow the bounds in a follow‑up to reduce trait noise.
Also applies to: 209-216, 222-247, 258-266
244-245: Unknown fields intentionally ignoredThis matches the added tests that accept extra fields. No action.
src/core/algorithms/bowyer_watson.rs (1)
1094-1126: New tests add valuable coverage across trait methods, cache provider, and strategy branchingGood breadth and isolation; assertions are scoped and non‑flaky.
Also applies to: 1187-1222, 1225-1240, 1243-1301, 1303-1341
src/core/boundary.rs (3)
189-190: Method delegation LGTMis_boundary_facet now correctly delegates to the cached map variant and propagates errors.
241-258: derive_facet_key_from_vertices error mapping is correctmap_err(TriangulationValidationError::FacetError) makes the error surface explicit; use of is_some_and is fine with MSRV 1.90.
111-114: Conversion exists — TriangulationValidationError implements FromTriangulationValidationError includes
FacetError(#[from] super::facet::FacetError), socell.facets()?will auto-convert; tests in src/core/triangulation_data_structure.rs cover this.src/geometry/predicates.rs (2)
599-603: Compile error: usize::is_multiple_of doesn’t exist (MSRV 1.90)Use a parity check via modulo.
Apply:
- let dimension_is_even = D.is_multiple_of(2); + let dimension_is_even = D % 2 == 0;
599-603: No other.is_multiple_ofusages found — no action neededripgrep shows a single match at src/geometry/predicates.rs:601; no further changes required.
src/core/triangulation_data_structure.rs (12)
157-167: Imports look correct and avoidOrderingclash.Aliasing
std::cmp::OrderingtoCmpOrderingand usingatomic::Orderingis clean and avoids ambiguity.
191-198: Good exposure of algorithm/traits via crate paths.Bringing
IncrementalBoyerWatson,InsertionAlgorithm, andInsertionErrorinto scope matches the PR’s API exposure goals.
536-539: Accessor is fine asconst fn.Returning
&SlotMap<…>via aconst fnis fine and consistent withvertices().
577-580: Symmetric accessor LGTM.
732-739:dim()logic is correct for empty and capped dimensions.
1658-1710: Neighbor setter validates length and unknown keys — good guardrails.Please add a quick test asserting that mutuality is re-established after a full
assign_neighbors()following manualset_neighbors_by_keymutations (to catch accidental non-mutual configurations).
2346-2468: Facet-to-neighbor mapping enforces semantic ordering; bounds and error cases covered.Good use of vertex keys and explicit checks for >2 cells per facet and buffer overflow guard.
2621-2662: Duplicate-cell removal rebuilds topology and bumps generation.Flow (identify→remove→reassign neigh/incident) looks correct.
3633-3715: CustomDeserializerebuilds UUID↔key maps; sensible defaults for state/generation.
3790-3884: Tests for duplicate UUID on add cover the intended error paths.
5345-5350: MSRV 1.90: Expectation thatnew()errors on insufficient vertices is coherent.This relies on
IncrementalBoyerWatson::triangulatesurfacingInsufficientVertices. If this ever changes, adjust the test to match the constructor contract.
7688-7895: Nice additional coverage for enums, Display impls, and error formatting.These light-weight tests help catch regression in user-facing error messages.
src/core/util.rs (4)
147-149: Good swap toFastHashSetfor adjacency check.Set-based comparison reads cleanly and is likely faster than
HashSetfor these small vertex sets.
10-10: Import ofCoordinateScalaris correct and matches bounds used below.
810-831: Nice edge‑case coverage forgenerate_combinations(k=0, k>len, k=len).
833-876: Comprehensive path coverage for iterator-based combination generation.Good exercise of termination and inner-loop paths.
src/core/traits/insertion_algorithm.rs (5)
7-7: LGTM! SmallBuffer usage improves performance.The change from
VectoSmallBufferwith inline storage reduces heap allocations for small collections, which is a smart optimization for hot paths in insertion algorithms.
420-426: Function signature change fromconst fntofnis necessary.The removal of
constis required sinceSmallBuffer::new()is not a const function. This is an acceptable trade-off for the performance benefits of SmallBuffer.
2022-2027: Clean test imports.The test imports are well-organized with specific types being imported rather than wildcards. The addition of
facet_key_from_vertex_keysimport aligns with the test requirements.
2820-4635: Comprehensive test coverage added - excellent work!The extensive test suite covers:
- Error handling for all
InsertionErrorvariants- Edge cases for bad cells detection including the degenerate cell threshold
- Strategy determination logic
- Insertion buffer functionality
- Statistical tracking
This thorough testing will significantly improve code reliability and maintainability.
390-396: Align SmallBuffer capacities with measured usageNo benchmarks or usage metrics were found in the repository to validate the chosen capacities (bad_cells: 16, boundary_facets: 8, vertex_points: 16, visible_facets: 8). Run profiling/microbenchmarks on representative workloads and tune these constants to match typical allocation/size patterns.
Location: src/core/traits/insertion_algorithm.rs (lines 390–396)src/core/cell.rs (11)
50-50: LGTM! FastHashMap/FastHashSet improves performance.Switching to
FastHashMapandFastHashSetfrom the standard library collections is a good performance optimization, especially for hot paths in validation.
55-58: Import consolidation is cleaner.The reorganized imports with
Deserialize,Deserializer, andSerializedirectly fromserde, and the groupeddeimports improve readability.
59-65: Simplified std imports.Using direct imports instead of full paths (e.g.,
cmpinstead ofstd::cmp) reduces verbosity while maintaining clarity.
285-285: Correct generic parameter naming.The deserializer parameter type
De(instead of the fullserde::Deserializer<'de>) follows Rust conventions for generic parameter names.
294-294: PhantomData usage is correct.Using
PhantomDatadirectly instead ofstd::marker::PhantomDatais consistent with the import style changes.Also applies to: 370-370
346-346: IgnoredAny import is properly scoped.Using the locally imported
IgnoredAnyinstead of a fully qualified path is consistent with the new import structure.
409-409: FastHashSet for duplicate detection is appropriate.Using
FastHashSetinstead of the standardHashSetfor duplicate vertex detection provides better performance, which is important for validation operations.Also applies to: 919-919
1091-1091: Return type simplification.Returning
Option<cmp::Ordering>instead ofOption<std::cmp::Ordering>is cleaner and consistent with the import changes.
1126-1126: Hash trait bound is correctly simplified.Using
H: Hasherinstead ofH: std::hash::Hasheris cleaner and follows the new import pattern.
1149-1149: Test type aliases updated correctly.The test type aliases now use
Vertexdirectly instead of the fully qualified path, which is consistent with the public API changes.Also applies to: 1154-1155
1560-1561: Test assertions use local cmp enum.Using
cmp::Ordering::Lessinstead of the fully qualified path is consistent and cleaner.src/geometry/point.rs (6)
28-37: Import consolidation and path shortening look good.Brings commonly used items into scope cleanly (Ordering, fmt, any, PhantomData). No issues spotted.
306-338: TryFrom improvements (error detail and bounds) are solid.Adding Debug for better error messages and switching to any::type_name is a good polish. No functional issues found.
537-538: Nice use of array.map for upcasts.Idiomatic and MSRV‑compatible for 1.90.0.
Also applies to: 575-576
610-621: FastHashMap in tests: OK.Assuming FastHashMap is part of the crate, this improves test perf a bit. No changes requested.
1448-1449: Use of f64::consts::PI arrays: OK.Clear and compile‑time friendly.
159-168: Approve — ordered_partial_cmp is total; PartialOrd OK.
impl_ordered_cmp implements ordered_partial_cmp as Some(OrderedFloat(*self).cmp(&OrderedFloat(*other))) for f32/f64 (src/geometry/traits/coordinate.rs), so Point::partial_cmp is total — no change needed.
| #[test] | ||
| fn test_insert_vertex_fallback() { | ||
| let algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new(); | ||
|
|
||
| let vertices = vec![ | ||
| vertex!([0.0, 0.0, 0.0]), | ||
| vertex!([1.0, 0.0, 0.0]), | ||
| vertex!([0.0, 1.0, 0.0]), | ||
| vertex!([0.0, 0.0, 1.0]), | ||
| ]; | ||
| let mut tds = Tds::<f64, Option<()>, Option<()>, 3>::new(&vertices).unwrap(); | ||
|
|
||
| // Test fallback insertion with a vertex that should work | ||
| let vertex_to_insert = vertex!([0.5, 0.5, 0.5]); | ||
| let result = algorithm.insert_vertex_fallback(&mut tds, &vertex_to_insert); | ||
|
|
||
| match result { | ||
| Ok(info) => { | ||
| println!("Fallback insertion succeeded: {info:?}"); | ||
| assert!(info.success); | ||
| assert_eq!(info.strategy, InsertionStrategy::Fallback); | ||
| } | ||
| Err(e) => { | ||
| println!("Fallback insertion failed: {e:?}"); | ||
| // TDS should still be valid | ||
| assert!(tds.is_valid().is_ok()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix: algorithm must be mutable to call insert_vertex_fallback(&mut self, ...).
The test won’t compile because insert_vertex_fallback takes &mut self but algorithm isn’t declared mut.
Apply this diff:
#[test]
fn test_insert_vertex_fallback() {
- let algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new();
+ let mut algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_insert_vertex_fallback() { | |
| let algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new(); | |
| let vertices = vec![ | |
| vertex!([0.0, 0.0, 0.0]), | |
| vertex!([1.0, 0.0, 0.0]), | |
| vertex!([0.0, 1.0, 0.0]), | |
| vertex!([0.0, 0.0, 1.0]), | |
| ]; | |
| let mut tds = Tds::<f64, Option<()>, Option<()>, 3>::new(&vertices).unwrap(); | |
| // Test fallback insertion with a vertex that should work | |
| let vertex_to_insert = vertex!([0.5, 0.5, 0.5]); | |
| let result = algorithm.insert_vertex_fallback(&mut tds, &vertex_to_insert); | |
| match result { | |
| Ok(info) => { | |
| println!("Fallback insertion succeeded: {info:?}"); | |
| assert!(info.success); | |
| assert_eq!(info.strategy, InsertionStrategy::Fallback); | |
| } | |
| Err(e) => { | |
| println!("Fallback insertion failed: {e:?}"); | |
| // TDS should still be valid | |
| assert!(tds.is_valid().is_ok()); | |
| } | |
| } | |
| } | |
| #[test] | |
| fn test_insert_vertex_fallback() { | |
| let mut algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new(); | |
| let vertices = vec![ | |
| vertex!([0.0, 0.0, 0.0]), | |
| vertex!([1.0, 0.0, 0.0]), | |
| vertex!([0.0, 1.0, 0.0]), | |
| vertex!([0.0, 0.0, 1.0]), | |
| ]; | |
| let mut tds = Tds::<f64, Option<()>, Option<()>, 3>::new(&vertices).unwrap(); | |
| // Test fallback insertion with a vertex that should work | |
| let vertex_to_insert = vertex!([0.5, 0.5, 0.5]); | |
| let result = algorithm.insert_vertex_fallback(&mut tds, &vertex_to_insert); | |
| match result { | |
| Ok(info) => { | |
| println!("Fallback insertion succeeded: {info:?}"); | |
| assert!(info.success); | |
| assert_eq!(info.strategy, InsertionStrategy::Fallback); | |
| } | |
| Err(e) => { | |
| println!("Fallback insertion failed: {e:?}"); | |
| // TDS should still be valid | |
| assert!(tds.is_valid().is_ok()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/core/algorithms/robust_bowyer_watson.rs around lines 3281 to 3309 the
test declares `algorithm` as immutable but calls `insert_vertex_fallback(&mut
self)`, causing a compile error; change the binding to be mutable (e.g., `let
mut algorithm = ...`) so the method can be called with `&mut algorithm` and
re-run tests to ensure compilation.
| let point_from_ref: Point<f64, 3> = Point::new(coords_ref.map(Into::into)); | ||
| assert_relative_eq!( |
There was a problem hiding this comment.
Bug: calling array .map() on a slice reference won’t compile.
coords_ref is &[f32; 3]; .map is on arrays, not on &[T; N]. Deref before mapping (or use from_fn).
- let point_from_ref: Point<f64, 3> = Point::new(coords_ref.map(Into::into));
+ let point_from_ref: Point<f64, 3> = Point::new((*coords_ref).map(Into::into));
+ // Alternatively:
+ // let point_from_ref: Point<f64, 3> = Point::new(std::array::from_fn(|i| coords_ref[i].into()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let point_from_ref: Point<f64, 3> = Point::new(coords_ref.map(Into::into)); | |
| assert_relative_eq!( | |
| let point_from_ref: Point<f64, 3> = Point::new((*coords_ref).map(Into::into)); | |
| // Alternatively: | |
| // let point_from_ref: Point<f64, 3> = Point::new(std::array::from_fn(|i| coords_ref[i].into())); | |
| assert_relative_eq!( |
🤖 Prompt for AI Agents
In src/geometry/point.rs around lines 1833-1834, the code calls .map() on
coords_ref which is a & [f32; 3] (a reference to an array) and therefore doesn't
compile; fix it by dereferencing the array before calling .map (for example use
(*coords_ref).map(Into::into)) or alternatively construct the array with
core::array::from_fn(|i| coords_ref[i].into()) so you convert the f32 values to
f64 and pass an actual [f64;3] into Point::new.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/util.rs (1)
64-77: Remove const from validate_uuid.Uuid::get_version_num is unlikely const; const fn here is unnecessary and risks compile errors. Make it a regular fn.
-pub const fn validate_uuid(uuid: &Uuid) -> Result<(), UuidValidationError> { +pub fn validate_uuid(uuid: &Uuid) -> Result<(), UuidValidationError> { // Check if UUID is nil if uuid.is_nil() { return Err(UuidValidationError::NilUuid); } // Check if UUID is version 4 let version = uuid.get_version_num(); if version != 4 { return Err(UuidValidationError::InvalidVersion { found: version }); } Ok(()) }src/core/triangulation_data_structure.rs (1)
2069-2176: add() can return Err while leaving the vertex inserted. Roll back on insertion failure.Currently, the vertex is inserted into vertices/uuid map before algorithm.insert_vertex(); on error, the TDS remains mutated, violating API expectations and invariants. Use insert_vertex_with_mapping and remove the vertex on failure.
Apply:
@@ - let uuid = vertex.uuid(); - - // Check if UUID already exists - if self.uuid_to_vertex_key.contains_key(&uuid) { - return Err(TriangulationConstructionError::DuplicateUuid { - entity: EntityKind::Vertex, - uuid, - }); - } + let uuid = vertex.uuid(); @@ - // Add vertex to SlotMap and create bidirectional UUID-to-key mapping - let key = self.vertices.insert(vertex); - self.uuid_to_vertex_key.insert(uuid, key); + // Insert vertex atomically; returns its key and bumps generation + let new_vertex_key = self.insert_vertex_with_mapping(vertex)?; @@ - if vertex_count == D + 1 && self.number_of_cells() == 0 { + if vertex_count == D + 1 && self.number_of_cells() == 0 { @@ - let cell_key = self.cells.insert(cell); - let cell_uuid = self.cells[cell_key].uuid(); - self.uuid_to_cell_key.insert(cell_uuid, cell_key); + // Use helper to maintain invariants and UUID mapping + let _ = self.insert_cell_with_mapping(cell)?; @@ - let mut algorithm = IncrementalBowyerWatson::new(); - algorithm.insert_vertex(self, vertex).map_err(|e| match e { - InsertionError::TriangulationConstruction(tc_err) => tc_err, - other => TriangulationConstructionError::FailedToAddVertex { - message: format!("Vertex insertion failed: {other}"), - }, - })?; + let mut algorithm = IncrementalBowyerWatson::new(); + if let Err(e) = algorithm.insert_vertex(self, vertex) { + // Roll back vertex insertion to keep TDS consistent + self.vertices.remove(new_vertex_key); + self.uuid_to_vertex_key.remove(&uuid); + self.bump_generation(); + return Err(match e { + InsertionError::TriangulationConstruction(tc_err) => tc_err, + other => TriangulationConstructionError::FailedToAddVertex { + message: format!("Vertex insertion failed: {other}"), + }, + }); + }
🧹 Nitpick comments (25)
src/geometry/predicates.rs (3)
609-652: Reduce duplicated sign logic; normalize determinant by parity/orientationCurrent branches duplicate comparisons. Normalize once, then compare. This reduces risk of future drift and makes intent clearer.
- match orientation { - Orientation::DEGENERATE => { - // Degenerate simplex - cannot determine containment reliably - Err(CellValidationError::DegenerateSimplex) - } - Orientation::POSITIVE => { - // For positive orientation, the sign interpretation depends on dimension parity - if dimension_is_even { - // Even dimensions: inverted sign compared to odd dimensions - if det < -tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det > tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } else { - // Odd dimensions: standard sign interpretation - if det > tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det < -tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } - } - Orientation::NEGATIVE => { - // For negative orientation, the sign interpretation also depends on dimension parity - if dimension_is_even { - // Even dimensions: inverted sign compared to odd dimensions - if det > tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det < -tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } else { - // Odd dimensions: standard sign interpretation - if det < -tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det > tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } - } - } + match orientation { + Orientation::DEGENERATE => Err(CellValidationError::DegenerateSimplex), + Orientation::POSITIVE | Orientation::NEGATIVE => { + // Normalize determinant by parity (even dims invert sign) and orientation + let parity_sign = if dimension_is_even { -1.0 } else { 1.0 }; + let orient_sign = if matches!(orientation, Orientation::POSITIVE) { 1.0 } else { -1.0 }; + let det_norm = det * parity_sign * orient_sign; + if det_norm > tolerance_f64 { + Ok(InSphere::INSIDE) + } else if det_norm < -tolerance_f64 { + Ok(InSphere::OUTSIDE) + } else { + Ok(InSphere::BOUNDARY) + } + } + }
259-266: Docs/code mismatch: parity mention applies only toinsphere_liftedThe comment says all three methods are parity-aware; only
insphere_liftedapplies parity logic. Either adjustinsphere/insphere_distanceor narrow the docs to the lifted formulation.
656-656: Fix Clippy lint path for test attributeUse the fully qualified Clippy lint to avoid unknown-lint warnings.
-#[allow(unnameable_test_items)] +#[allow(clippy::unnameable_test_items)].github/workflows/codecov.yml (1)
101-105: Guard use of rg to avoid CI failures if ripgrep is missing
rgisn’t guaranteed on all runners. Add a fallback togrepso coverage verification doesn’t fail spuriously.Apply this diff:
- if rg -n '(^|/)(benches|examples)/' coverage/cobertura.xml; then - echo "::warning::benches/ or examples/ paths detected in coverage report" - fi + if command -v rg >/dev/null 2>&1; then + if rg -n '(^|/)(benches|examples)/' coverage/cobertura.xml; then + echo "::warning::benches/ or examples/ paths detected in coverage report" + fi + else + if grep -nE '(^|/)(benches|examples)/' coverage/cobertura.xml; then + echo "::warning::benches/ or examples/ paths detected in coverage report (grep fallback)" + fi + fisrc/geometry/point.rs (1)
218-231: Deserialization improvements are good; consider accepting "NaN" strings and case-insensitive special valuesOptional polish: accept "NaN"/"nan" in addition to null, and case-insensitive Infinity tokens for robustness.
Apply this diff to extend special-value handling:
- CoordRepr::Str(s) => { - // Handle special string representations - match s.as_str() { - "Infinity" => T::infinity(), - "-Infinity" => T::neg_infinity(), - _ => { - return Err(Error::custom(format!( - "Unknown special value: {s}" - ))); - } - } - } + CoordRepr::Str(s) => { + // Handle special string representations (case-insensitive) + let sl = s.trim().to_ascii_lowercase(); + match sl.as_str() { + "infinity" | "inf" => T::infinity(), + "-infinity" | "-inf" => T::neg_infinity(), + "nan" => T::nan(), + _ => { + return Err(Error::custom(format!( + "Unknown special value: {s}" + ))); + } + } + }Also applies to: 262-289
src/core/collections.rs (1)
858-863: Verify MSRV support for std::iter::repeat_n; consider a conservative fallbackIf
repeat_nisn’t available on 1.90, tests will fail. Userepeat(...).take(...)as a portable alternative.Apply this diff if needed:
- buffer_16.extend(std::iter::repeat_n(String::new(), 17)); // > inline(16) + buffer_16.extend(std::iter::repeat(String::new()).take(17)); // > inline(16)If you want me to confirm via web search whether
repeat_nis stable in Rust 1.90, I can run it.src/core/traits/insertion_algorithm.rs (9)
489-493: Avoid const on mutable accessors.Marking mut accessors as const is unnecessary and may constrain usage. Recommend regular fn.
- pub const fn bad_cells_buffer_mut( + pub fn bad_cells_buffer_mut( &mut self, ) -> &mut SmallBuffer<crate::core::triangulation_data_structure::CellKey, 16> { &mut self.bad_cells_buffer }
502-504: Same: drop const on mutable accessor.- pub const fn boundary_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { + pub fn boundary_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { &mut self.boundary_facets_buffer }
515-519: Same: drop const on mutable accessor.- pub const fn vertex_points_buffer_mut( + pub fn vertex_points_buffer_mut( &mut self, ) -> &mut SmallBuffer<crate::geometry::point::Point<T, D>, 16> { &mut self.vertex_points_buffer }
528-530: Same: drop const on mutable accessor.- pub const fn visible_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { + pub fn visible_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { &mut self.visible_facets_buffer }
916-921: Minor perf: prefer SmallVec for tiny temporary buffers.This hot path avoids heap allocs better with SmallVec for vertex_points.
- // Preallocate vertex_points buffer outside the loop to avoid per-iteration allocations - let mut vertex_points = Vec::with_capacity(D + 1); + // Reuse a small stack-allocated buffer to avoid heap traffic + let mut vertex_points: SmallVec<[Point<T, D>; 8]> = SmallVec::with_capacity(D + 1);
759-765: Same perf nit: SmallVec suits D+1 points.- // Reserve exact capacity once before the loop to avoid reallocations - let mut vertex_points = Vec::with_capacity(D + 1); + // Reserve exact capacity once; keep on stack for typical small D + let mut vertex_points: SmallVec<[Point<T, D>; 8]> = SmallVec::with_capacity(D + 1);
839-861: Potential integer over/underflow on bbox expansion for extreme integer coordinates.For signed integer T, min_margin subtraction/addition can overflow. Consider clamping or using wider intermediates for integer types.
Short fix idea: detect integer T (mf == 0 path) and clamp expanded_min/max with checked_sub/checked_add backstopped to i{32,64}::MIN/MAX via NumCast if available.
1129-1137: Avoid cloning Cell in hot path (follow-up to TODO).Consider a Facet::from_cell_and_index(cell_key, facet_index, tds) constructor to sidestep Cell clone.
1896-1905: Consider surfacing orientation failures as recoverable errors.Returning Ok(false) on predicate failure may hide genuine geometric issues; returning InsertionError::geometric_failure could improve fallback behavior.
tests/convex_hull_bowyer_watson_integration.rs (1)
189-219: Make boundary-facet growth assertion non-strict to reduce flakiness.Some exterior insertions can leave boundary count unchanged (rare geometry). Prefer >= over >.
- assert!( - final_boundary_count > initial_boundary_count, - "Should have more boundary facets after extensions" - ); + assert!( + final_boundary_count >= initial_boundary_count, + "Boundary facets should not decrease after hull extensions" + );src/core/facet.rs (2)
197-208: Deserialize impl: make key parsing explicit and future‑proof.Use next_key::<&str>() (or a small Field enum) to avoid inference pitfalls and get better unknown/duplicate field diagnostics.
Apply this minimal change inside visit_map:
- while let Some(key) = map.next_key()? { + while let Some(key) = map.next_key::<&str>()? {If you want stricter checking and nicer errors, switch to a Field enum:
#[derive(Deserialize)] #[serde(field_identifier, rename_all = "lowercase")] enum Field { Cell, Vertex, #[serde(other)] Ignore }And then use next_key::() with a match on Field.
493-510: Avoid heap allocations in key() by using a small stack buffer.This path runs frequently; prefer SmallBuffer to reduce allocations and improve cache locality.
Apply this diff:
- let mut uuid_values = Vec::with_capacity(vertices.len() * 2); + use crate::core::collections::SmallBuffer; + let mut uuid_values: SmallBuffer<u64, { crate::core::collections::MAX_PRACTICAL_DIMENSION_SIZE * 2 }> = + SmallBuffer::new(); for vertex in vertices { let uuid_bytes = vertex.uuid().as_u128(); // Intentionally truncate to u64 to get low bits, then high bits #[allow(clippy::cast_possible_truncation)] { uuid_values.push(uuid_bytes as u64); uuid_values.push((uuid_bytes >> 64) as u64); } } // Use the shared stable hash function - stable_hash_u64_slice(&uuid_values) + stable_hash_u64_slice(&uuid_values)Add this import near the other uses:
use crate::core::collections::SmallBuffer;src/core/algorithms/robust_bowyer_watson.rs (3)
1018-1069: Guard against empty facet_vertices before centroid division.Centroid division by zero is theoretically possible if D=0 (degenerate), or if a facet is malformed. Add a simple early return.
fn fallback_visibility_heuristic( &self, facet: &Facet<T, U, V, D>, vertex: &Vertex<T, U, D>, ) -> bool where T: DivAssign<T> + AddAssign<T> + std::ops::Sub<Output = T> + std::ops::Mul<Output = T>, { - let facet_vertices = facet.vertices(); + let facet_vertices = facet.vertices(); + if facet_vertices.is_empty() { + // Conservatively treat as not visible if we cannot compute a centroid + return false; + }Optionally consider moving VISIBILITY_THRESHOLD_MULTIPLIER into RobustPredicateConfig as the TODO notes.
975-983: Use a stack-friendly buffer for small simplices.These two small Vec<Point<..>> allocations are per-facet; switch to SmallBuffer to avoid heap churn.
I can provide a patch if you want to adopt SmallBuffer here and elsewhere in this hot path.
1380-1401: Optional: route println! through debug_println in tests.A few println! calls remain unguarded; consider using debug_println! to keep CI quieter by default.
src/core/boundary.rs (1)
914-919: Gitleaks false positive: hex literal is not a secret.The 0x1234_5678_9ABC_DEF0 constant in tests (used for formatting validation) is a benign placeholder. Consider adding a repo-level gitleaks allowlist entry for this test path to avoid CI noise.
If you want, I can draft a minimal .gitleaks.toml addition to ignore this specific path/pattern.
src/core/cell.rs (2)
916-919: FastHashSet in is_valid for dedup — LGTM; optional micro‑optCurrent approach copies
Vertexinto the set. If desired, you can avoid copies using references:Apply this diff:
- let mut seen = FastHashSet::default(); - if !self.vertices.iter().all(|vertex| seen.insert(*vertex)) { + let mut seen: FastHashSet<&Vertex<T, U, D>> = FastHashSet::default(); + if !self.vertices.iter().all(|vertex| seen.insert(vertex)) { return Err(CellValidationError::DuplicateVertices); }
986-987: Doc bound mentionsSum; confirm necessityThe method body doesn’t appear to use
Sum. If unused, consider dropping it from the impl bound for clarity.src/core/triangulation_data_structure.rs (1)
2131-2134: Prefer insert_cell_with_mapping() for the initial simplex.Keeps UUID↔key mappings consistent and centralizes invariants.
- let cell_key = self.cells.insert(cell); - let cell_uuid = self.cells[cell_key].uuid(); - self.uuid_to_cell_key.insert(cell_uuid, cell_key); + let _ = self.insert_cell_with_mapping(cell)?;
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/codecov.yml(2 hunks).gitignore(1 hunks)Cargo.toml(2 hunks)REFERENCES.md(1 hunks)WARP.md(2 hunks)rust-toolchain.toml(2 hunks)src/core/algorithms/bowyer_watson.rs(15 hunks)src/core/algorithms/robust_bowyer_watson.rs(10 hunks)src/core/boundary.rs(9 hunks)src/core/cell.rs(17 hunks)src/core/collections.rs(3 hunks)src/core/facet.rs(7 hunks)src/core/traits/insertion_algorithm.rs(26 hunks)src/core/triangulation_data_structure.rs(25 hunks)src/core/util.rs(13 hunks)src/geometry/point.rs(18 hunks)src/geometry/predicates.rs(4 hunks)src/geometry/robust_predicates.rs(10 hunks)src/geometry/traits/coordinate.rs(4 hunks)src/geometry/util.rs(13 hunks)tests/COVERAGE.md(1 hunks)tests/convex_hull_bowyer_watson_integration.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/COVERAGE.md
- Cargo.toml
- .gitignore
- WARP.md
- src/geometry/robust_predicates.rs
- rust-toolchain.toml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-30T02:40:22.032Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/util.rssrc/geometry/traits/coordinate.rssrc/geometry/util.rs
📚 Learning: 2025-08-30T02:40:22.032Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/util.rs
📚 Learning: 2025-09-04T20:03:49.896Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/convex_hull_bowyer_watson_integration.rssrc/geometry/traits/coordinate.rssrc/core/algorithms/bowyer_watson.rssrc/core/boundary.rssrc/geometry/util.rssrc/geometry/predicates.rssrc/core/algorithms/robust_bowyer_watson.rssrc/core/triangulation_data_structure.rssrc/core/cell.rssrc/core/traits/insertion_algorithm.rs
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/traits/coordinate.rssrc/geometry/point.rssrc/core/cell.rs
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/traits/coordinate.rssrc/geometry/point.rssrc/core/cell.rs
🧬 Code graph analysis (11)
src/core/util.rs (2)
src/core/facet.rs (4)
cell(362-364)vertex(407-409)new(324-330)vertices(474-481)src/core/triangulation_data_structure.rs (2)
new(1899-1931)vertices(578-580)
tests/convex_hull_bowyer_watson_integration.rs (3)
src/core/triangulation_data_structure.rs (3)
bowyer_watson(2230-2247)new(1899-1931)is_valid(3362-3398)src/core/algorithms/bowyer_watson.rs (1)
new(184-193)src/core/traits/insertion_algorithm.rs (2)
new(281-293)new(421-428)
src/geometry/traits/coordinate.rs (1)
src/geometry/point.rs (3)
new(78-80)validate(95-107)ordered_equals(117-122)
src/geometry/point.rs (3)
src/geometry/util.rs (8)
std(238-238)std(278-278)std(499-499)std(507-507)std(1950-1950)std(1997-1997)std(4350-4350)core(1388-1388)src/core/facet.rs (3)
hash(539-542)new(324-330)get_hash(1261-1265)src/core/vertex.rs (3)
hash(660-664)partial_cmp(587-589)point(404-406)
src/core/algorithms/bowyer_watson.rs (3)
src/core/traits/insertion_algorithm.rs (4)
is_vertex_interior(752-775)new(281-293)new(421-428)default(407-409)src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/traits/facet_cache.rs (1)
new(377-382)
src/core/boundary.rs (4)
src/core/triangulation_data_structure.rs (5)
cells(537-539)vertices(578-580)new(1899-1931)number_of_cells(800-802)dim(732-739)src/core/cell.rs (10)
cell(2193-2193)cell(2322-2322)cell(2324-2324)cell(2328-2328)cell(2527-2527)cell(2540-2540)facets(1052-1057)from(374-376)vertices(475-477)dim(657-659)src/core/traits/boundary_analysis.rs (3)
boundary_facets(92-92)number_of_boundary_facets(232-232)is_boundary_facet_with_map(195-199)src/core/util.rs (1)
derive_facet_key_from_vertices(369-412)
src/geometry/util.rs (3)
src/core/algorithms/robust_bowyer_watson.rs (1)
safe_usize_to_scalar(1037-1037)benches/profiling_suite.rs (3)
safe_usize_to_scalar(190-190)safe_usize_to_scalar(191-191)cast(196-196)src/geometry/traits/coordinate.rs (3)
mantissa_digits(546-546)mantissa_digits(555-557)mantissa_digits(565-567)
src/core/algorithms/robust_bowyer_watson.rs (6)
src/core/triangulation_data_structure.rs (4)
is_valid(3362-3398)new(1899-1931)vertices(578-580)cells(537-539)src/core/algorithms/bowyer_watson.rs (1)
new(184-193)src/core/facet.rs (4)
new(324-330)vertices(474-481)vertex(407-409)key(492-510)src/core/traits/insertion_algorithm.rs (2)
new(281-293)new(421-428)src/core/boundary.rs (1)
boundary_facets(88-151)src/core/util.rs (1)
derive_facet_key_from_vertices(369-412)
src/core/triangulation_data_structure.rs (6)
src/core/traits/insertion_algorithm.rs (5)
fmt(38-52)with_capacity(432-439)new(281-293)new(421-428)default(407-409)src/core/cell.rs (9)
cell(2193-2193)cell(2322-2322)cell(2324-2324)cell(2328-2328)cell(2527-2527)cell(2540-2540)vertices(475-477)dim(657-659)is_valid(906-942)src/core/facet.rs (5)
cell(362-364)facet_key_from_vertex_keys(611-623)new(324-330)vertices(474-481)vertex(407-409)src/core/algorithms/bowyer_watson.rs (2)
new(184-193)default(230-232)src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/vertex.rs (3)
from_points(348-353)dim(503-505)is_valid(539-555)
src/core/cell.rs (3)
src/core/facet.rs (3)
hash(539-542)vertex(407-409)cell(362-364)src/geometry/point.rs (2)
hash(135-137)partial_cmp(158-167)src/core/vertex.rs (2)
hash(660-664)partial_cmp(587-589)
src/core/traits/insertion_algorithm.rs (6)
src/core/algorithms/robust_bowyer_watson.rs (1)
new(113-123)src/core/algorithms/bowyer_watson.rs (1)
new(184-193)src/core/facet.rs (4)
new(324-330)vertex(407-409)facet_key_from_vertex_keys(611-623)vertices(474-481)src/core/triangulation_data_structure.rs (5)
new(1899-1931)bowyer_watson(2230-2247)vertices(578-580)number_of_cells(800-802)vertex_keys(1523-1525)src/core/boundary.rs (1)
boundary_facets(88-151)src/core/traits/boundary_analysis.rs (1)
boundary_facets(92-92)
🪛 Gitleaks (8.28.0)
src/core/boundary.rs
[high] 915-915: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: Codacy Security Scan
- GitHub Check: build (windows-latest)
- GitHub Check: performance-regression
🔇 Additional comments (74)
src/geometry/predicates.rs (2)
599-603: Compile error:usizehas nois_multiple_ofmethodReplace with a parity check that compiles on stable Rust.
- let dimension_is_even = D.is_multiple_of(2); + let dimension_is_even = D % 2 == 0;
592-597: Resolved — implicit conversion exists
CellValidationError definesCoordinateConversion { #[from] source: CoordinateConversionError }in src/core/cell.rs, sosimplex_orientation(...) ?converts correctly..github/workflows/codecov.yml (1)
63-76: Doctest coverage enablement looks goodIncluding
--docis a sensible addition; the comments reflect the change.REFERENCES.md (1)
66-80: Solid additions on degeneracy handling and EGCThe SoS, perturbations, and EGC citations are accurate and helpful context for robust predicate docs.
src/geometry/traits/coordinate.rs (1)
523-547: Newmantissa_digits()API is appropriate and correctly implemented
- Trait surface is clear and useful for precision-aware logic.
- Implementations for f32/f64 returning
Self::MANTISSA_DIGITSare correct.Also applies to: 555-557, 565-567
src/geometry/point.rs (2)
158-167: PartialOrd via OrderedFloat semantics LGTMLexicographic compare with
ordered_partial_cmpis correct and consistent with Eq/Hash semantics.
1743-1745: Fix: calling.map()on&[T; N]doesn’t compile; deref first (or use from_fn)This test won’t compile as
coords_refis a reference. Deref before calling.map().Apply this diff:
- let coords_ref = &[1.0f32, 2.0f32, 3.0f32]; - let point_from_ref: Point<f64, 3> = Point::new(coords_ref.map(Into::into)); + let coords_ref = &[1.0f32, 2.0f32, 3.0f32]; + let point_from_ref: Point<f64, 3> = Point::new((*coords_ref).map(Into::into)); + // Alternatively: + // let point_from_ref: Point<f64, 3> = + // Point::new(std::array::from_fn(|i| coords_ref[i].into()));src/geometry/util.rs (13)
437-441: LGTM! Improved precision handling with mantissa-aware bounds.The two-stage precision narrative (f64 intermediate then target type) provides clearer understanding of the conversion chain. The dynamic
max_precise_u128calculation usingmin(F64_MANTISSA_BITS, T::mantissa_digits())is mathematically sound and properly handles precision boundaries for both f32 and f64 targets.
476-483: Excellent precision documentation with concrete examples.The updated documentation clearly explains the precision limits with specific numeric examples:
- f64: exact up to 2^53 − 1 (9,007,199,254,740,991)
- f32: exact up to 2^24 − 1 (16,777,215)
This helps developers understand when precision loss occurs and the platform-dependent behavior.
486-492: Well-implemented mantissa-aware precision checking.The implementation correctly:
- Uses
T::mantissa_digits()to get target type precision- Takes the minimum of f64 (53 bits) and target mantissa bits
- Calculates
max_precise_u128dynamically based on the limiting precisionThis approach ensures precision safety for both the f64 intermediate step and final target conversion.
502-502: Correct precision boundary enforcement.The comparison
u128::from(value_u64) > max_precise_u128properly enforces the calculated precision limit, ensuring no precision loss occurs in either conversion step.
829-829: Good refactoring to eliminate redundant computation.Using
circumradius_with_center(points, &circumcenter)directly removes the extra error-mapping closure layer while maintaining the same functionality. This is a clean simplification.
896-898: Semantically correct 1D facet measure.Changing the 1D case to return
0.0instead of1.0is mathematically correct - a point (0-dimensional facet) has measure 0, not 1. This aligns with proper dimensional analysis.
958-968: Consistent implementation of 0-dimensional measure.The 1D case properly validates the point count and returns
T::zero()for a 0-dimensional point measure. The error handling for incorrect point counts is also appropriate.
2014-2049: Comprehensive f32 precision boundary tests.The test correctly validates:
- f32's 24-bit mantissa limit (2^24 - 1 = 16,777,215)
- Values within f32 precision should succeed
- Values exceeding f32 precision should fail
- The same values should still work for f64 due to higher precision
The test uses proper casting with
num_traits::castand appropriate epsilon values.
3723-3737: Complete 5D grid generation test coverage.The test extends coverage to 5D (2^5 = 32 points) with proper coordinate range validation. This ensures the grid generation algorithm works correctly across higher dimensions.
3883-3915: Thorough 4D and 5D Poisson disk sampling tests.These tests properly validate:
- Correct point generation within bounds
- Minimum distance constraints in higher dimensions
- Different coordinate types (f32 for 4D, f64 for 5D)
- Appropriate epsilon values based on floating-point precision
The distance calculations using
hypotare numerically stable for higher dimensions.Also applies to: 3917-3949
4142-4148: Excellent 5D triangulation test coverage.The test validates 5D triangulation creation with sufficient points (10) for a full 5D triangulation. This ensures the random triangulation generation works correctly across all supported dimensions.
4191-4667: Comprehensive edge case and error path testing.These extensive tests cover critical error handling scenarios:
Non-finite value handling:
- NaN, infinity, and negative infinity detection in coordinate conversions
- Proper error types and messages
Precision boundary testing:
- Extended usize-to-scalar precision limit validation
- Platform-specific behavior testing
Coordinate conversion robustness:
- Array conversions with mixed valid/invalid values
- Proper error propagation through conversion chains
Geometric function edge cases:
- Degenerate simplices and matrix inversions
- Empty point sets and invalid simplex configurations
Utility function coverage:
- Byte formatting, safety caps, error message formatting
- Comprehensive testing of all error variants
This thorough testing significantly improves code reliability and error handling robustness.
437-441: mantissa_digits() implemented for f32/f64 — confirm other scalarsFound impls in src/geometry/traits/coordinate.rs (impl CoordinateScalar for f32 at ~550 and for f64 at ~560) — both return Self::MANTISSA_DIGITS and tests assert f32==24 and f64==53. If other scalar types are intended to be supported, add CoordinateScalar impls with the correct mantissa values.
src/core/traits/insertion_algorithm.rs (6)
382-398: Good move to SmallBuffer; capacities look sane.Inline capacities (16 for keys/points, 8 for facets) are reasonable defaults for D≤5. Nice perf-focused change.
432-438: Capacity heuristics LGTM.Scaling vertex_points_buffer by (D+1) and guarding visible_facets_buffer against 0 are sensible.
1841-1848: Good: surface topology corruption as TriangulationState error.Clear message; helps diagnostics.
1867-1876: Good: explicit opposite-vertex validation.Right failure mode for malformed facets.
1773-1783: LGTM: visibility errors propagate cleanly.Match arms handle Ok/Err as expected.
1179-1192: Approve — Result-returning visibility API used consistently.
rg found only the trait declaration and a single callsite at src/core/traits/insertion_algorithm.rs:1773 which matches on Ok(true)/Ok(false); no boolean-only expectations remain.tests/convex_hull_bowyer_watson_integration.rs (5)
14-16: Import rename aligns with API.IncrementalBowyerWatson usage looks correct.
59-93: Solid initial-state assertions.Boundary/internal/invalid facet counts for a tetrahedron are correct and helpful sanity checks.
141-164: Post-insertion validations are robust.Validating TDS and facet invariants after each hull extension is great for catching regressions.
242-267: Mixed strategy coverage looks good.Asserting both cavity and hull strategies are exercised improves confidence in the dispatcher.
290-357: State reset test is useful.Verifies stats reset and post-reset functionality. Nice.
src/core/util.rs (3)
333-341: Docs clarify validation behavior well.Explicit InsufficientVertices and VertexNotFound are good, predictable errors.
383-392: Early vertex-count validation is correct.Fail-fast on facet_vertices.len() != D avoids bogus keys.
146-149: Vertex Eq/Hash are coordinate-based — original UUID-based suggestion is incorrectVertex implements PartialEq/Eq/Hash using point coordinates (Point-based hashing/equality) — see src/core/vertex.rs (impl Eq / impl Hash); tests and collections rely on this. If you want identity-by-UUID instead, change the Vertex Eq/Hash impls; otherwise accept current design but be mindful of floating-point edge cases (NaN, -0.0, precision).
Likely an incorrect or invalid review comment.
src/core/facet.rs (5)
66-69: Serde import consolidation looks good.Unified serde imports improve readability and keep de::{...} items local. No issues.
151-160: Good addition: InvalidFacetMultiplicity error variant.Aligns with boundary validation; error message is clear and actionable.
254-255: Correct use of IgnoredAny for unknown fields.Extra fields are safely ignored during deserialization.
273-274: PhantomData tuple is appropriate.Using PhantomData<(T, U, V)> is correct and clearer than separate markers.
325-325: Reference check in contains_vertex is the right call.Passing &vertex avoids an unnecessary move/copy and matches typical API shapes.
src/core/algorithms/robust_bowyer_watson.rs (3)
935-941: Trait bounds update is appropriate.Adding DivAssign matches the use of /= in the fallback visibility heuristic. Keeps constraints local to where they’re needed.
1286-1291: Nice: conditional debug macro for tests.Prevents noisy CI logs when not feature-flagged.
3293-3307: Fix: algorithm must be mutable to call insert_vertex_fallback(&mut self, ...).The test as written won’t compile; declare the binding as mut.
- let algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new(); + let mut algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new();src/core/boundary.rs (5)
101-147: Boundary facet collection and multiplicity validation look solid.
- Single-pass over facet_to_cells with strict validation
- Correct error mapping: InvalidFacetIndex, CellNotFoundInTriangulation, InvalidFacetMultiplicity
- Per-cell facet cache avoids repeated allocations
Please ensure build_facet_to_cells_map() has corresponding strict/error variants used here (it appears so from context). If any callers relied on the old lenient path, they should be updated to handle Result.
169-171: Doc accuracy improvement.Good clarification on Ok(true)/Ok(false) semantics with error propagation.
199-205: Efficient delegation via prebuilt facet map.This avoids recomputation on repeated checks.
257-274: Key derivation path is correct and defensive.
- Debug assert for D vertices
- derive_facet_key_from_vertices error is surfaced as TriangulationValidationError::FacetError
- Simple multiplicity check against the prebuilt map
310-312: Counting from facet map is the right optimization.Avoids cloning Facet objects; time complexity is linear in number of unique facets.
src/core/algorithms/bowyer_watson.rs (5)
147-156: Good deprecation alias for backward compatibility.Clear migration path with corrected spelling; avoids breaking downstream code immediately.
170-176: Docs and example updated to new type name.Example compiles as-is with the trait import; looks good.
214-215: Explicit trait qualification is preferable here.Calling is_vertex_interior via the trait avoids method resolution surprises and makes intent clear.
235-251: FacetCacheProvider impl is correct.Returning &AtomicU64 from Arc with as_ref() is clean; cache field wiring is consistent with other algorithms.
1098-1112: Default impl delegating to new() is fine.Keeps behavior in one place; tests verify parity.
src/core/cell.rs (18)
50-61: FastHashMap/FastHashSet + serde import consolidation — LGTMNice swap to fast collections and cleaned imports. Matches the PR’s perf goals.
282-282: Deserializer type aliasing — LGTMUsing
De: Deserializer<'de>reads cleaner.
291-291: PhantomData usage in Visitor — LGTMCorrectly carries generics without affecting size.
343-343: Unknown fields ignored via IgnoredAny — LGTMMatches tests and desired forward-compat behavior.
367-367: PhantomData init — LGTMNo-op size impact; correct construction.
406-411: Duplicate detection switched to FastHashSet — LGTMGood perf-minded change in builder validate.
686-686: Docs updated for reference param — LGTMExample correctly uses
&vertex1.
1087-1090: PartialOrd using cmp::Ordering — LGTMConsistent with sorted vertex comparison semantics.
1120-1129: Hash impl constrained on Vertex: Hash and uses sorted vertices — LGTMRespects Eq/Hash contract and aligns with Vertex hash semantics.
1145-1146: Test imports consolidated — LGTM
1151-1152: Test type aliases for Vertex — LGTMImproves readability across tests.
1167-1167: Test generic bounds include Serialize — LGTMMatches production bounds.
1556-1558: Ordering edge-case assertions — LGTMCovers non-less-than for equal cells.
1595-1599: contains_vertex assertions updated to pass references — LGTM
2226-2227: Negative contains_vertex case via reference — LGTM
2407-2412: from_facet_and_vertex containment checks via & — LGTM
2451-2456: Post-creation checks for new/removed vertices — LGTM
688-690: contains_vertex now takes &Vertex — call sites updatedAvoids needless copies; I scanned usages and all calls already pass &vertex.
src/core/triangulation_data_structure.rs (4)
2240-2241: Switch to IncrementalBowyerWatson acknowledged.Good alignment with the new insertion algorithm surface.
2368-2370: Good use of SmallBuffer and saturating capacity.Avoids heap alloc in the facet loop and guards zero-dimension edge cases.
3727-3731: Generation reset on deserialization is the right call.Prevents stale cache reuse across serde boundaries.
1769-1789: Approve: VertexKeyBuffer return is safe — callsites verified.Only one callsite (src/geometry/algorithms/convex_hull.rs:427); it uses iterator methods (.iter(), .find(), .copied()) which are compatible with VertexKeyBuffer (SmallVec), so no changes required.
Enhances the robustness and stability of triangulation data structure (TDS) operations by addressing several potential issues. * Prevents potential overflow issues during bounding box expansion by clamping the margin calculation for integer types. * Improves handling of near-degenerate cases by adding a configurable visibility threshold multiplier. * Uses stack allocation with SmallVec for vertex points to reduce memory allocations in `find_bad_cells` and `vertex_outside_circumcircle` for faster performance. * Adds a rollback mechanism during vertex insertion to guarantee TDS consistency if the insertion fails. * Fixes potential issues with exterior vertex insertion in the robust Bowyer-Watson algorithm, improves point deserialization, and avoids double counting in statistics. * Handles cases with potentially empty facets conservatively to avoid crashes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/traits/insertion_algorithm.rs (2)
20-20: Import Bounded to support safe clamping.Required by the overflow‑safe bbox expansion fix.
-use num_traits::{NumCast, One, Zero, cast}; +use num_traits::{Bounded, NumCast, One, Zero, cast};
791-793: Add minimal bounds for safe clamping in bbox expansion.
Bounded,Add,Sub, andPartialOrdare needed by the corrected logic.- where - T: AddAssign<T> + SubAssign<T> + Sum + NumCast + One + Zero + PartialEq + Div<Output = T>, + where + T: AddAssign<T> + + SubAssign<T> + + Sum + + NumCast + + One + + Zero + + PartialEq + + Div<Output = T> + + std::cmp::PartialOrd + + std::ops::Add<Output = T> + + std::ops::Sub<Output = T> + + Bounded,src/core/triangulation_data_structure.rs (1)
2995-3007: Do not sort SlotMap keys for determinism — use a stable identifierSorting CellKey relies on an unspecified ordering. Use a stable key (e.g., cell UUID) to ensure deterministic behavior across runs/platforms.
Apply:
- valid_cells.sort_unstable(); // Deterministic ordering by CellKey + // Deterministic ordering by stable UUID instead of SlotMap key + valid_cells.sort_unstable_by_key(|&k| self.cells[k].uuid());
🧹 Nitpick comments (13)
.gitleaks.toml (1)
21-25: Simplify regexes with case-insensitive flagConsolidate upper/lowercase variants using (?i).
Apply this diff:
regexes = [ - '''0x1234_5678_9ABC_DEF0''', # Test constant for facet key formatting in boundary tests - '''0x1234_5678_90ab_cdef''', # Test constant for facet key formatting in convex hull tests - '''0xdead_beef_cafe_babe''', # Additional test constant used for comparison tests + '''(?i)0x1234_5678_9abc_def0''', # Test constant for facet key formatting in boundary tests + '''(?i)0x1234_5678_90ab_cdef''', # Test constant for facet key formatting in convex hull tests + '''(?i)0xdead_beef_cafe_babe''', # Additional test constant used for comparison tests ]src/geometry/predicates.rs (3)
703-707: Re-enable the 3D OUTSIDE assertionWith the parity fix, this should now pass; avoid leaving placeholder comments in tests.
Apply this diff:
- let _point_far_outside = Point::new([10.0, 10.0, 10.0]); - // Using placeholder assertion due to method inconsistency - // assert_eq!(insphere_lifted(&simplex_points, point_far_outside).unwrap(), InSphere::OUTSIDE); + let point_far_outside = Point::new([10.0, 10.0, 10.0]); + assert_eq!( + insphere_lifted(&simplex_points, point_far_outside).unwrap(), + InSphere::OUTSIDE + );
870-885: Revisit TODOs; either assert or mark as ignoredNow that sign handling is fixed, these should be assertable. If still flaky, mark with #[ignore] and reference an issue.
Example (choose one):
- // TODO: Matrix method should correctly identify this point as inside, but currently fails - // This is why we use circumsphere_contains_vertex in bowyer_watson instead - // assert_eq!( - // insphere_lifted(&simplex_points, _actually_inside).unwrap(), - // InSphere::INSIDE - // ); + assert_eq!( + insphere_lifted(&simplex_points, _actually_inside).unwrap(), + InSphere::INSIDE + );or
- #[test] + #[test] + #[ignore = "track at #<issue-id> until parity behavior confirmed on all platforms"]
627-627: Verify lint availability on MSRV 1.90Ensure
unnameable_test_itemsis recognized; otherwise add#[allow(unknown_lints)]above it to avoid warnings on toolchains lacking the lint.Apply this diff if needed:
-#[allow(unnameable_test_items)] +#[allow(unknown_lints)] +#[allow(unnameable_test_items)]src/geometry/point.rs (1)
279-282: Improve error message with coordinate indexInclude the element index to aid debugging malformed inputs.
Apply this diff:
- _ => { - return Err(Error::custom(format!( - "Unknown special value: {s}" - ))); - } + _ => { + return Err(Error::custom(format!( + "Unknown special value at index {i}: {s}" + ))); + }src/core/traits/insertion_algorithm.rs (3)
489-493: Removeconstfrom mutating accessors; they’re not usable in const contexts and will trigger clippy.These methods take
&mut selfand/or return&mut _. Marking themconstprovides no benefit and is misleading.- pub const fn bad_cells_buffer_mut( + pub fn bad_cells_buffer_mut( @@ - pub const fn boundary_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { + pub fn boundary_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { @@ - pub const fn vertex_points_buffer_mut( + pub fn vertex_points_buffer_mut( @@ - pub const fn visible_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> { + pub fn visible_facets_buffer_mut(&mut self) -> &mut SmallBuffer<Facet<T, U, V, D>, 8> {Optionally also drop
constfrom other mutating methods likereset,record_*, and getters returning&SmallBuffer<_>for consistency.Also applies to: 502-504, 515-519, 528-530
1919-1925: Return the correct attempted strategy in visibility error.This path is used by hull‑extension visibility. Tagging the error as
CavityBasedis misleading during debugging/telemetry.- Err(InsertionError::geometric_failure( - "Orientation predicate failed during facet visibility test", - InsertionStrategy::CavityBased, - )) + Err(InsertionError::geometric_failure( + "Orientation predicate failed during facet visibility test", + InsertionStrategy::HullExtension, + ))Alternatively, pass the strategy in from the caller and thread it through.
1141-1147: AvoidCellclone in facet construction (hot path).Cloning
cellper facet is avoidable. Prefer a constructor that derives aFacetfrom(cell_key, facet_index)with TDS lookup to skip cloning.Example direction (API sketch):
// Facet::from_cell_and_index(cell_key, facet_index, tds) -> Result<Facet<...>>Then use it here to eliminate the allocation/copy.
src/core/triangulation_data_structure.rs (2)
1768-1789: Prefer delegating to vertex_keys_for_cell_direct to avoid duplicationThis implementation duplicates the direct-key path. Delegate to the existing helper to keep one source of truth.
pub fn get_cell_vertex_keys( &self, cell_key: CellKey, ) -> Result<VertexKeyBuffer, TriangulationValidationError> { - let Some(cell) = self.get_cell_by_key(cell_key) else { - return Err(TriangulationValidationError::InconsistentDataStructure { - message: format!("Cell with key {cell_key:?} not found"), - }); - }; - - // Pre-size to D+1 to reflect simplex invariant and avoid reallocation - let mut vertex_keys = VertexKeyBuffer::with_capacity(D + 1); - for vertex in cell.vertices() { - let key = self.vertex_key_from_uuid(&vertex.uuid()).ok_or_else(|| { - TriangulationValidationError::InconsistentDataStructure { - message: format!("Vertex UUID {} not found in mapping", vertex.uuid()), - } - })?; - vertex_keys.push(key); - } - - Ok(vertex_keys) + self.vertex_keys_for_cell_direct(cell_key) }
2147-2157: Make add() atomic on post-insert failuresYou rollback on algorithm.insert_vertex() errors, but not if assign_neighbors()/assign_incident_cells() fail afterward. Consider reverting the inserted vertex (and any transient mappings) on those errors to keep TDS consistent after a failed add.
Do current tests cover a forced failure in assign_neighbors()/assign_incident_cells() right after a successful algorithm insertion? If not, add one or guard with a transactional wrapper.
tests/convex_hull_bowyer_watson_integration.rs (2)
60-96: Nice coverage expansion; consider gating verbosity.The comprehensive assertions are solid. To reduce CI noise, consider using a debug_println!-style macro (behind a test feature) for these prints, mirroring robust_bowyer_watson.rs tests.
Also applies to: 179-226
208-223: Boundary facet monotonicity may be too strict.Boundary facet counts can occasionally decrease depending on hull topology changes. If this flakes in practice, relax to an invariant like “no invalid facets; triangulation valid; final cell count increases.”
src/core/cell.rs (1)
1079-1090: PartialOrd/Hash consistent with PartialEq.Sorting vertices for equality/order/hash maintains Eq/Hash contract. Consider caching a normalized signature if this becomes a hotspot.
Also applies to: 1119-1129
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitleaks.toml(1 hunks)cspell.json(6 hunks)src/core/algorithms/robust_bowyer_watson.rs(18 hunks)src/core/cell.rs(18 hunks)src/core/traits/insertion_algorithm.rs(29 hunks)src/core/triangulation_data_structure.rs(28 hunks)src/geometry/point.rs(18 hunks)src/geometry/predicates.rs(5 hunks)src/geometry/robust_predicates.rs(15 hunks)tests/convex_hull_bowyer_watson_integration.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cspell.json
- src/geometry/robust_predicates.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-04T20:03:49.896Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/predicates.rssrc/core/triangulation_data_structure.rssrc/core/algorithms/robust_bowyer_watson.rstests/convex_hull_bowyer_watson_integration.rssrc/core/traits/insertion_algorithm.rssrc/core/cell.rs
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/point.rssrc/core/cell.rs
📚 Learning: 2025-08-20T21:55:04.552Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/geometry/point.rssrc/core/cell.rs
🧬 Code graph analysis (6)
src/core/triangulation_data_structure.rs (3)
src/core/traits/insertion_algorithm.rs (5)
fmt(38-52)with_capacity(432-439)new(281-293)new(421-428)default(407-409)src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/algorithms/bowyer_watson.rs (2)
new(184-193)default(230-232)
src/core/algorithms/robust_bowyer_watson.rs (6)
src/core/facet.rs (5)
facet_vertices(736-748)vertex(407-409)new(324-330)vertices(474-481)key(492-510)src/core/vertex.rs (1)
point(404-406)src/core/triangulation_data_structure.rs (4)
number_of_cells(800-802)new(1899-1931)vertices(578-580)cells(537-539)src/core/traits/insertion_algorithm.rs (2)
new(281-293)new(421-428)src/geometry/robust_predicates.rs (1)
general_triangulation(774-784)src/core/util.rs (1)
derive_facet_key_from_vertices(369-412)
src/geometry/point.rs (3)
src/geometry/util.rs (8)
std(238-238)std(278-278)std(499-499)std(507-507)std(1950-1950)std(1997-1997)std(4350-4350)core(1388-1388)src/core/cell.rs (2)
hash(1121-1128)partial_cmp(1086-1089)src/core/vertex.rs (3)
hash(660-664)partial_cmp(587-589)point(404-406)
tests/convex_hull_bowyer_watson_integration.rs (4)
src/core/triangulation_data_structure.rs (3)
bowyer_watson(2225-2242)new(1899-1931)is_valid(3363-3399)src/core/algorithms/robust_bowyer_watson.rs (1)
new(113-123)src/core/traits/insertion_algorithm.rs (2)
new(281-293)new(421-428)src/core/algorithms/bowyer_watson.rs (1)
new(184-193)
src/core/traits/insertion_algorithm.rs (5)
src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/triangulation_data_structure.rs (5)
new(1899-1931)bowyer_watson(2225-2242)default(216-218)vertices(578-580)number_of_cells(800-802)src/core/algorithms/bowyer_watson.rs (2)
new(184-193)default(230-232)src/core/facet.rs (3)
new(324-330)vertex(407-409)vertices(474-481)src/core/boundary.rs (1)
boundary_facets(88-151)
src/core/cell.rs (3)
src/geometry/point.rs (2)
hash(135-137)partial_cmp(158-167)src/core/facet.rs (3)
hash(539-542)vertex(407-409)cell(362-364)src/core/vertex.rs (2)
hash(660-664)partial_cmp(587-589)
⏰ 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: performance-regression
- GitHub Check: Codacy Security Scan
- GitHub Check: build (windows-latest)
🔇 Additional comments (31)
.gitleaks.toml (3)
11-12: Using built-in rules looks goodEnabling default rules via [extend] is correct.
16-25: Sanity-check passed — literals exist only in expected files
Matches found: 0x1234_5678_9ABC_DEF0 → src/core/boundary.rs:915; 0x1234_5678_90ab_cdef → src/geometry/algorithms/convex_hull.rs:4213; 0xdead_beef_cafe_babe → src/geometry/algorithms/convex_hull.rs:4273. Referenced files exist; allowlist is narrowly scoped.
14-25: Usepaths(notfiles) for the top-level [allowlist] in Gitleaks v8Confirmed: Gitleaks v8 expects [allowlist].paths — your .gitleaks.toml already uses
paths, so no change needed.src/geometry/predicates.rs (3)
605-615: Parity-normalized sign handling: LGTMThe orientation/parity normalization and single thresholding path is clean and correct after fixing the even-dimension check.
257-267: Docs parity note: LGTMClear distinction between standard vs lifted formulations and when parity applies.
600-603: Fix:usizehas nois_multiple_of; use modulo check
D.is_multiple_of(2)won’t compile on stable Rust (MSRV 1.90). UseD % 2 == 0.Apply this diff:
- let dimension_is_even = D.is_multiple_of(2); + let dimension_is_even = D % 2 == 0;src/geometry/point.rs (11)
158-167: Lexicographic PartialOrd with ordered-float semantics: LGTMConsistent with CoordinateScalar’s ordered comparisons; aligns with downstream Vertex/Cell ordering.
189-215: Confirm heterogenous element serialization across binary formatsSerializing NaN as null and ±∞ as strings is great for JSON; in non-self-describing formats (e.g., bincode), the mixed element types may be surprising. Confirm this is intentional for all enabled formats.
320-339: TryFrom error reporting: LGTMClear error kinds and informative type names via std::any::type_name; finiteness check post-cast is correct.
251-253: Nice: precise Deserialize expectation messageHelpful and explicit; matches the accepted special values and null mapping.
531-535: Array.map upcast pattern: LGTMIdiomatic and zero-allocation conversion from [f32; N] to [f64; N].
592-605: FastHashMap usage in hashing test: LGTMValidates Eq/Hash semantics with custom float ordering using the intended map.
1008-1045: Hash consistency with NaN/∞: LGTMGood coverage; verifies stable hashing across special values and lookup behavior.
1640-1709: Format‑agnostic (JSON) deserialization tests: LGTMCovers numbers, null→NaN, ±∞ strings, round‑trip, and type variance.
1711-1823: Case‑insensitive special value handling: LGTMComprehensive coverage for "inf"/"infinity"/"nan" variants and whitespace trimming.
2717-2720: Set membership with special values: LGTMValidates Hash+Eq behavior in sets, including NaN and signed zeros.
1859-1861: Bug: calling .map() on a &[T; N] (array reference) won’t compileDereference the array first (or use from_fn).
Apply one of the fixes:
- let point_from_ref: Point<f64, 3> = Point::new(coords_ref.map(Into::into)); + let point_from_ref: Point<f64, 3> = Point::new((*coords_ref).map(Into::into));Alternative:
- let point_from_ref: Point<f64, 3> = Point::new(coords_ref.map(Into::into)); + let point_from_ref: Point<f64, 3> = + Point::new(std::array::from_fn(|i| coords_ref[i].into()));src/core/triangulation_data_structure.rs (2)
2112-2115: Deterministic initial simplex ordering: sorting by UUID is the right callGood fix. Avoiding sort on SlotMap keys sidesteps unspecified Ord and ensures stable initial simplex creation.
1768-1771: Signature change OK — callers already handle VertexKeyBufferOnly usage found: src/geometry/algorithms/convex_hull.rs — the call uses .iter()/.find()/.copied() on the returned buffer (SmallVec/VertexKeyBuffer supports these), so no caller changes required.
tests/convex_hull_bowyer_watson_integration.rs (2)
14-17: API alignment looks good.Using IncrementalBowyerWatson via the public API is correct and matches core changes.
119-173: Assertion on hull extension invariants is appropriate.Validating strategy, cell deltas, and triangulation validity after each insertion is on point.
Also applies to: 141-173
src/core/algorithms/robust_bowyer_watson.rs (5)
935-945: Trait bound update is correct.Adding DivAssign to is_facet_visible_from_vertex_robust aligns with fallback_visibility_heuristic’s requirements.
1019-1072: Visibility heuristic: sensible guardrails.Empty-facet guard and threshold derivation from config are reasonable; fallback uses f64: From correctly.
734-776: Facet map validation is robust.Deduplication and ≤2-cell sharing checks are good defensive additions.
Also applies to: 782-797
328-334: Fix: make algorithm mutable in test_insert_vertex_fallback.insert_vertex_fallback requires &mut self; the test declares algorithm as immutable, causing a compile error.
Apply this diff:
#[test] fn test_insert_vertex_fallback() { - let algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new(); + let mut algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new();Also applies to: 3299-3326
1289-1296: Good: gated debug output macro.Using debug_println! behind a feature flag reduces test noise.
src/core/cell.rs (5)
50-61: Improved imports and FastHash usage are appropriate.Consolidating serde imports and switching to FastHashMap/Set is fine.
404-411: Duplicate vertex detection now O(n).Using FastHashSet for builder validation and is_valid is a good improvement over pairwise checks.
Also applies to: 915-918
685-689: API tweak: contains_vertex takes &Vertex — good.Call sites updated accordingly. This avoids unnecessary copies.
Also applies to: 1594-1598, 2225-2226, 2406-2411, 2450-2456
107-125: Validation errors and neighbor-length checks are precise.Clear, actionable errors; neighbors length invariant enforced correctly.
Also applies to: 858-942
797-856: into_hashmap return type change to FastHashMap is sensible.Public surface remains intuitive; usage unchanged.
| expanded_max[i] = | ||
| if max_coords[i] <= (cast::<f64, T>(f64::MAX).unwrap_or_else(T::zero) - margin) { | ||
| max_coords[i] + margin | ||
| } else { | ||
| max_coords[i] // Saturate at current value to prevent overflow | ||
| }; |
There was a problem hiding this comment.
Fix unsound integer overflow guard in bbox expansion (can underflow/overflow for non‑float T).
cast::<f64, T>(f64::MAX) returns None for integer T, so you end up doing T::zero() - margin in the check, which can underflow (debug panic) or yield bogus behavior. Use num_traits::Bounded and clamp via max_value instead.
Apply this diff:
- expanded_max[i] =
- if max_coords[i] <= (cast::<f64, T>(f64::MAX).unwrap_or_else(T::zero) - margin) {
- max_coords[i] + margin
- } else {
- max_coords[i] // Saturate at current value to prevent overflow
- };
+ let max_val = num_traits::Bounded::max_value();
+ expanded_max[i] = if max_coords[i] <= (max_val - margin) {
+ max_coords[i] + margin
+ } else {
+ max_val // Clamp to type max to prevent overflow
+ };You’ll also need the import and a small bound tweak; see the next two comments.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/traits/insertion_algorithm.rs around lines 871 to 876, the current
overflow guard uses cast::<f64, T>(f64::MAX) which returns None for integer T
and makes the check evaluate to T::zero() - margin (causing underflow); replace
this with num_traits::Bounded logic: import num_traits::Bounded, ensure the
generic T: Bounded (or add Bounded to the trait bounds), and change the branch
to compare max_coords[i] against T::max_value() - margin, clamping
expanded_max[i] to max_value() when the subtraction would overflow; do the
analogous change for the min side if present so all saturation uses
T::min_value()/T::max_value() instead of casting from f64.
This commit enhances the triangulation data structure (TDS) by improving robustness and ensuring atomicity during vertex insertion. - Introduces a `rollback_vertex_insertion` method to ensure consistent TDS state after vertex insertion failures, including topology assignment. Rolls back changes if `assign_neighbors()` or `assign_incident_cells()` fails. - Optimizes vertex insertion, neighbor and incident cell assignments. - Removes doctests from code coverage reports. - Updates regexes for gitleaks allowlist.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (21)
src/geometry/predicates.rs (8)
591-599: Map orientation errors into CellValidationError for type consistencyinsphere_lifted returns CellValidationError, but simplex_orientation returns CoordinateConversionError. Even if length is prechecked, conversions can still fail. Avoid relying on a blanket From; map explicitly for consistency with the rest of the function.
Apply:
- let orientation = simplex_orientation(simplex_points)?; + let orientation = simplex_orientation(simplex_points) + .map_err(|e| CellValidationError::CoordinateConversion { source: e })?;
236-244: Ensure trait bound for Float::abs or avoid itFloat::abs requires T: Float. The where-clause is T: CoordinateScalar + Sum + Zero; unless CoordinateScalar implies Float, this won’t compile.
Options:
- If CoordinateScalar: Float, do nothing.
- Otherwise, add Float bound:
-where - T: CoordinateScalar + Sum + Zero, +where + T: CoordinateScalar + Sum + Zero + Float,Please confirm whether CoordinateScalar already guarantees Float; if not, adopt the bound above.
249-253: Docs: replace outdated “circumsphere_contains” referenceThe preferred distance-based API here is insphere_distance. Update the wording to prevent confusion.
Apply:
-/// over `circumsphere_contains` as it provides better numerical stability by using a matrix +/// over `insphere_distance` as it provides better numerical stability by using a matrix
400-425: De-duplicate orientation branches via normalized determinantLogic is identical modulo sign. Normalize by orientation and compare once.
Apply:
- Orientation::POSITIVE => { - if det > tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det < -tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } - Orientation::NEGATIVE => { - if det < -tolerance_f64 { - Ok(InSphere::INSIDE) - } else if det > tolerance_f64 { - Ok(InSphere::OUTSIDE) - } else { - Ok(InSphere::BOUNDARY) - } - } + Orientation::POSITIVE | Orientation::NEGATIVE => { + let orient_sign = if matches!(orientation, Orientation::POSITIVE) { 1.0 } else { -1.0 }; + let det_norm = det * orient_sign; + if det_norm > tolerance_f64 { + Ok(InSphere::INSIDE) + } else if det_norm < -tolerance_f64 { + Ok(InSphere::OUTSIDE) + } else { + Ok(InSphere::BOUNDARY) + } + }
1281-1355: Tests with only printlns: mark as diagnostic or assert somethingThese two tests are observational and can spam logs.
Options:
- Add #[ignore = "diagnostic test; no assertions"] to both.
- Or convert to doc examples behind cfg(test) with asserts.
- Or gate prints behind a feature flag/logging.
Example:
-#[test] +#[test] +#[ignore = "diagnostic output only"] fn debug_circumsphere_properties() { ... } -#[test] +#[test] +#[ignore = "diagnostic output only"] fn compare_circumsphere_methods() { ... }Also applies to: 1668-1699
627-628: Unnecessary lint allowance?#[allow(unnameable_test_items)] seems unused here; tests are plain functions. If no longer needed, remove it.
256-266: Docs clarity: parity note scoped to lifted methodNice clarification. Consider adding a one-liner linking to the exact section in insphere_lifted explaining the parity flip to aid navigation.
256-271: Reference formattingSmall nit: two Shewchuk references appear; keeping one in “References” section below would reduce duplication.
.gitleaks.toml (1)
25-28: Avoid suggesting path-based allowlists in examplesEven as comments, path-based allowlist examples invite copy/paste regressions. Replace with a literal-based example or add a note discouraging file-level allowlists.
-# [[allowlist]] -# description = "Allow test hex literals in all test files" -# paths = ['''.*test.*\.rs$''', '''.*tests/.*\.rs$'''] -# regexes = ['''0x[0-9A-Fa-f_]+'''] +# [[allowlist]] +# description = "Example: prefer literal-scoped regex over file paths" +# regexes = ['''0xdead_beef_cafe_babe'''] +# # Note: avoid file-level path allowlists; they mask unrelated leaks.src/core/traits/insertion_algorithm.rs (2)
489-493: Avoid const on mutable accessors unless you need const‑context calls
pub const fn ..._mut(&mut self) -> &mut ...provides no practical benefit and can constrain future changes due to const rules. Consider droppingconstfrom the accessors.- pub const fn bad_cells_buffer_mut( + pub fn bad_cells_buffer_mut( &mut self, ) -> &mut SmallBuffer<crate::core::triangulation_data_structure::CellKey, 16> {(Apply similarly to other
*_buffer_mutand non-essentialconstaccessors.)
1139-1141: Avoid Cell clone in hot path by providing indexed Facet constructorThe TODO is spot on. A
Facet::from_cell_and_index(cell_key, facet_index, tds)(or similar) would eliminate a clone and reduce allocations in a hot loop.Would you like a concrete API sketch for this constructor?
src/core/triangulation_data_structure.rs (6)
2064-2165: Avoid triple generation bumps in add()Generation is bumped:
- inside insert_vertex_with_mapping(),
- inside assign_neighbors(),
- again at the end of add().
The final bump is redundant and adds churn to cache invalidation.
Apply this diff to drop the last bump:
- // Increment generation counter to invalidate caches - self.bump_generation(); - Ok(())Also applies to: 2167-2171
2115-2132: Confirm algorithm side-effects on failure; rollback mode might be too lenientOn insert_vertex() error you call rollback_vertex_insertion(..., remove_related_cells = false). If the algorithm partially mutated cells before erroring, this leaves a risk of orphaned cells. Please confirm the insertion contract: either no structural mutations on Err, or switch to remove_related_cells = true here.
2453-2473: Neighbor assignment: good structure; one nit on memory trade-off
- Facet-key map and semantic ordering are correct and robust.
- The two-phase “updates Vec” is a reasonable borrow-checker trade-off. If this becomes hot and large, consider chunked application to reduce peak memory without unsafe. No action required now.
Also applies to: 2482-2568, 2540-2544
2733-2741: Deterministic duplicate reporting in errorsIn validate_no_duplicate_cells(), you sort VertexKey values. That’s fine for detection, but error messages print the keys, which can be non-deterministic across runs. Prefer reporting sorted vertex UUIDs for stable diagnostics.
Apply this diff inside the duplicate error formatting:
- .map(|(cell1, cell2, vertices)| { - format!("cells {cell1:?} and {cell2:?} with vertices {vertices:?}") - }) + .map(|(cell1, cell2, vertices)| { + let mut vertex_uuids: Vec<Uuid> = + vertices.iter().map(|&k| self.vertices[k].uuid()).collect(); + vertex_uuids.sort_unstable(); + format!( + "cells {cell1:?} and {cell2:?} with vertex UUIDs {vertex_uuids:?}" + ) + })Also applies to: 3340-3346, 3351-3363
2173-2270: Rollback helper is pragmatic; consider guarding logsThe stderr logging is useful in bulk flows; consider gating with cfg(debug_assertions) to avoid noisy logs in release/libs.
3913-3951: Test coverage expansion — excellent breadthStrong, focused tests for rollback, error paths, boundary analysis, mapping consistency, and multi-D flows. Minor nit: consider reducing println!/eprintln! in steady-state tests to keep CI logs terse.
Also applies to: 3953-3977, 3980-4024, 4026-4095, 4097-4151, 4383-4478, 5731-5826, 6050-6100, 6450-6470, 6560-6650, 7053-7174, 7177-7278, 7953-8520
src/core/algorithms/robust_bowyer_watson.rs (4)
935-940: Consider trimming trait bounds (DivAssign) here.is_facet_visible_from_vertex_robust doesn’t itself use DivAssign; the fallback method has its own bounds. Keeping bounds minimal reduces API friction for alternative T types.
Apply this diff if you agree:
- where - T: AddAssign<T> - + ComplexField<RealField = T> - + SubAssign<T> - + Sum - + From<f64> - + DivAssign<T>, + where + T: AddAssign<T> + ComplexField<RealField = T> + SubAssign<T> + Sum + From<f64>,
1291-1297: Nice: test-only debug logging macro.Gating noisy logs behind feature "test-debug" keeps CI output clean while preserving local insight.
Consider documenting the feature flag in WARP.md for discoverability.
1537-1540: Reduce println! noise in tests or gate under debug feature.These ad-hoc printouts can flood CI logs. Consider using the debug_println! macro for consistency.
Apply this pattern:
- println!("Getting all boundary facets..."); + debug_println!("Getting all boundary facets...");Repeat similarly for nearby print statements where detailed output isn’t required by assertions.
Also applies to: 1558-1561, 1570-1584
743-776: Facet mapping validation looks solid; minor dedup optimization optional.Dedup via FastHashSet is correct. As a micro-tweak, using retain with reserve_exact on seen can shave rehashing for large facets.
- { - let mut seen = fast_hash_set_with_capacity(cell_keys.len()); - cell_keys.retain(|k| seen.insert(*k)); - } + let mut seen = fast_hash_set_with_capacity(cell_keys.len()); + cell_keys.retain(|k| seen.insert(*k));
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitleaks.toml(1 hunks)src/core/algorithms/robust_bowyer_watson.rs(18 hunks)src/core/traits/insertion_algorithm.rs(29 hunks)src/core/triangulation_data_structure.rs(45 hunks)src/geometry/predicates.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T20:03:49.896Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/robust_bowyer_watson.rssrc/core/triangulation_data_structure.rssrc/core/traits/insertion_algorithm.rssrc/geometry/predicates.rs
🧬 Code graph analysis (4)
src/core/algorithms/robust_bowyer_watson.rs (8)
src/core/traits/insertion_algorithm.rs (2)
new(281-293)new(421-428)src/core/triangulation_data_structure.rs (4)
new(1867-1899)number_of_cells(800-802)vertices(578-580)cells(537-539)src/core/facet.rs (5)
new(324-330)facet_vertices(736-748)vertex(407-409)vertices(474-481)key(492-510)src/core/vertex.rs (1)
point(404-406)src/core/cell.rs (2)
vertices(474-476)facets(1051-1056)src/core/boundary.rs (1)
boundary_facets(88-151)src/geometry/robust_predicates.rs (3)
general_triangulation(774-784)high_precision(800-811)degenerate_robust(827-838)src/core/util.rs (1)
derive_facet_key_from_vertices(369-412)
src/core/triangulation_data_structure.rs (5)
src/core/traits/insertion_algorithm.rs (5)
fmt(38-52)new(281-293)new(421-428)with_capacity(432-439)default(407-409)src/core/cell.rs (11)
cell(2192-2192)cell(2321-2321)cell(2323-2323)cell(2327-2327)cell(2526-2526)cell(2539-2539)uuid(501-503)vertices(474-476)vertex_uuids(582-587)is_valid(905-941)dim(656-658)src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/algorithms/bowyer_watson.rs (2)
new(184-193)default(230-232)src/core/vertex.rs (6)
uuid(459-461)None(1899-1899)is_valid(539-555)point(404-406)from_points(348-353)dim(503-505)
src/core/traits/insertion_algorithm.rs (5)
src/core/algorithms/robust_bowyer_watson.rs (2)
new(113-123)default(77-79)src/core/triangulation_data_structure.rs (5)
new(1867-1899)bowyer_watson(2324-2341)default(216-218)vertices(578-580)number_of_cells(800-802)src/core/algorithms/bowyer_watson.rs (2)
new(184-193)default(230-232)src/core/facet.rs (3)
new(324-330)vertex(407-409)vertices(474-481)src/core/boundary.rs (1)
boundary_facets(88-151)
src/geometry/predicates.rs (2)
src/geometry/util.rs (1)
circumradius(824-830)src/geometry/point.rs (1)
new(78-80)
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: performance-regression
🔇 Additional comments (22)
src/geometry/predicates.rs (8)
633-654: Solid coverage additions across dimensionsGood set of assertions and edge cases; parity handling is exercised well.
Minor nit to keep: ensure doc examples compile under doctest; paths like use delaunay::geometry::InSphere may require re-export. If doctests are enabled, please confirm.
Also applies to: 703-708, 718-739, 742-766, 768-846, 848-856, 858-891, 893-907, 1001-1017, 1166-1182, 1184-1212, 1214-1240, 1242-1278, 1357-1398, 1399-1412, 1414-1450, 1452-1489, 1491-1529, 1531-1571, 1572-1609, 1610-1666
510-524: Error type consistency on arity checkGood switch to CellValidationError::InsufficientVertices; aligns with core/cell validation semantics.
372-395: Matrix construction and normalization look correctStructure matches the standard insphere determinant (coords | squared_norm | 1).
112-161: Orientation predicate: tolerant degeneracy and matrix layout LGTMDeterminant-based orientation with tolerance is fine; docs and example are clear.
216-225: insphere_distance: API shape and error propagation are coherentGood separation vs determinant methods; circumcenter errors flow via CircumcenterError.
580-587: Lifted matrix build is numerically reasonableCentering at p0 and using squared norms in last column is standard and stable.
600-615: Normalize determinant once (parity × orientation) — niceAfter the parity fix above, this provides a clean single-branch decision.
Please add a brief inline comment citing the parity rule source (even D flips sign) to future-proof maintenance.
600-603: Fix compile error: use parity check without unstable/nonexistent APIusize doesn’t have is_multiple_of() on stable (MSRV 1.90). Use a simple modulo check.
Apply:
- let dimension_is_even = D.is_multiple_of(2); + let dimension_is_even = D % 2 == 0;.gitleaks.toml (1)
16-21: Good fix: allowlist scoped to specific literals, not filesSwitching from file path allowlisting to tightly scoped regexes addresses the prior leakage risk. LGTM.
src/core/traits/insertion_algorithm.rs (3)
458-475: Visibility testing now returns Result — good propagationSwitching to
Result<bool, InsertionError>and propagating topology errors keeps callers honest and improves diagnosability. Nice.
421-427: SmallBuffer migration: constructor is clean and const-safeInitialization via
SmallBuffer::new()across all buffers looks correct. Reuse-friendly and zero-alloc in common cases.
863-874: Integer overflow risk in bbox expansion (expanded_max) and asymmetric clampingFor integer T,
max_coords[i] + margincan overflow. Comment claims “saturating addition” but normal+is used. Lower bound also clamps to current value instead of type min, making expansion asymmetric.Clamp using Bounded min/max and guard subtraction/addition.
- // Protect against overflow for integer types during bbox expansion - // For floating-point types, this is equivalent to normal arithmetic - // For integer types, this prevents overflow by clamping to valid ranges - expanded_min[i] = if min_coords[i] >= margin { - min_coords[i] - margin - } else { - min_coords[i] // Saturate at current value to prevent underflow - }; - // Use saturating addition to prevent overflow - // This works for both integer and floating-point types - expanded_max[i] = max_coords[i] + margin; + // Clamp to type bounds for integer T; behaves like normal math for floats + let min_val = <T as num_traits::Bounded>::min_value(); + let max_val = <T as num_traits::Bounded>::max_value(); + expanded_min[i] = if min_coords[i] >= min_val + margin { + min_coords[i] - margin + } else { + min_val + }; + expanded_max[i] = if max_coords[i] <= max_val - margin { + max_coords[i] + margin + } else { + max_val + };Additionally add the import and a minimal bound:
- use num_traits::{NumCast, One, Zero, cast}; + use num_traits::{NumCast, One, Zero, cast, Bounded};And in the where-clause of
is_vertex_likely_exterioradd:- where - T: AddAssign<T> + SubAssign<T> + Sum + NumCast + One + Zero + PartialEq + Div<Output = T>, + where + T: AddAssign<T> + + SubAssign<T> + + Sum + + NumCast + + One + + Zero + + PartialEq + + Div<Output = T> + + num_traits::Bounded + + PartialOrd,src/core/triangulation_data_structure.rs (5)
157-166: Imports and trait bounds updates — LGTMImports align with the new insertion API and small-buffer utilities. No issues spotted.
Also applies to: 170-173, 191-191, 195-197
1076-1139: Key-based vertex-key retrieval is solid; error boundary is appropriateGood use of SmallBuffer and precise InconsistentDataStructure errors on missing mappings. This should remove a lot of UUID→Key overhead in hot paths.
3671-3673: Confirm T::is_finite availability in PartialEq debug assertionsThe debug_assert uses x.is_finite() on generic T. Ensure CoordinateScalar guarantees this method (e.g., Float-like bound). If not guaranteed, this will fail to compile even in debug-only contexts.
Also applies to: 3683-3685, 3675-3677, 3687-3689
3827-3831: Generation reset on deserialization — LGTMResetting generation to 0 and reconstructing maps is correct to invalidate caches across serialization boundaries.
2872-2883: Consistent use of get_cell_vertex_keys — LGTMNice consolidation to the key-based path with proper error propagation throughout boundary/mapping/validation logic.
Also applies to: 2967-2968, 3512-3520
src/core/algorithms/robust_bowyer_watson.rs (5)
976-981: Good fix: dimension-safe SmallBuffer usage.Switching to MAX_PRACTICAL_DIMENSION_SIZE removes the prior capacity-8 hazard and prevents overflow for higher dimensions.
Please confirm MAX_PRACTICAL_DIMENSION_SIZE >= D + 1 for all supported D. If not guaranteed, we should gate with a debug assert.
Also applies to: 982-986
1026-1032: Robust guard for empty facet vertices.Early-returning false avoids centroid math on invalid/empty facets. Sensible defensive check.
1062-1066: Config‑driven visibility threshold is a solid improvement.Using visibility_threshold_multiplier with perturbation_scale makes the heuristic tunable and testable.
Double-check multiplier semantics across presets to ensure intended selectivity (see robust_predicates config presets).
518-561: Bad-cell detection path: good reuse of SmallBuffer and conservative boundary handling.The robust_insphere fallback to standard predicate on error is a safe degradation; set-based merge avoids O(n²).
Confirm that MAX_PRACTICAL_DIMENSION_SIZE safely covers vertex_points capacity (D+1). Add a debug_assert!(vertex_points.capacity() >= D + 1) if helpful.
3302-3315: Fix: algorithm must be mutable to call insert_vertex_fallback(&mut self, ...).This test won’t compile as written.
Apply this diff:
- let algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new(); + let mut algorithm = RobustBoyerWatson::<f64, Option<()>, Option<()>, 3>::new();
Corrects `insphere` tests by considering simplex orientation to interpret determinant sign for more accurate results. This resolves issues with lifted matrix formulations in various dimensions. Adds comprehensive tests for insphere predicates across dimensions 2D to 5D, including error and edge cases, ensuring correctness. Adds debug output for circumradius tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/geometry/predicates.rs (1)
221-222: Compile fix: add Float (or Signed) bound for insphere_distance
Float::abs(circumradius - radius)requiresT: Float. Without this bound, the function does not compile. Easiest fix: add+ Floatto the where‑clause.-where - T: CoordinateScalar + Sum + Zero, +where + T: CoordinateScalar + Sum + Zero + Float,Alternative (if you prefer not to require
Float): switch tonum_traits::Signedand useSigned::abs, but that’s a broader change (import + callsite) and not necessary ifTis always floating-point.
🧹 Nitpick comments (7)
.gitleaks.toml (2)
14-21: Tighten regex allowlist to avoid substring over-matchesAdd word boundaries so only the exact test literals are ignored, not longer strings containing them.
[allowlist] description = "Benign hex literals used for facet key formatting validation in tests" regexes = [ - '''(?i)0x1234_5678_9abc_def0''', # Test constant for facet key formatting in boundary tests - '''(?i)0x1234_5678_90ab_cdef''', # Test constant for facet key formatting in convex hull tests - '''(?i)0xdead_beef_cafe_babe''', # Additional test constant used for comparison tests + '''(?i)\b0x1234_5678_9abc_def0\b''', # Test constant for facet key formatting in boundary tests + '''(?i)\b0x1234_5678_90ab_cdef\b''', # Test constant for facet key formatting in convex hull tests + '''(?i)\b0xdead_beef_cafe_babe\b''', # Additional test constant used for comparison tests ]
1-3: Add CI to enforce Gitleaks on PRsRecommend a lightweight GH Action to block secrets before merge and pin a version for reproducibility.
# .github/workflows/gitleaks.yml name: gitleaks on: pull_request: push: { branches: [ main ] } jobs: scan: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 with: { fetch-depth: 0 } - uses: gitleaks/gitleaks-action@v2 with: version: v8.18.4 args: --no-git --config .gitleaks.toml --verbosebenches/PERFORMANCE_RESULTS.md (3)
96-96: Clarify what these “Performance Summary” numbers represent.State whether these are medians/means across dimensions/tests, or a representative case, to avoid misinterpretation.
Also applies to: 98-98
137-143: Reconcile the “Method Disagreements” narrative with 100% agreement above.Given the updated results show 100% agreement, reword this section to note potential (not observed) disagreements and why they can occur.
Apply this diff:
-### Method Disagreements - -The disagreements between methods are expected due to: +### Numerical considerations and potential disagreements + +While this run shows 100% agreement across methods, disagreements can occur due to:
6-6: Toolchain 1.90.0 confirmed — optional: make commit hash clickablerust-toolchain.toml (line 3: channel = "1.90.0") and Cargo.toml (rust-version = "1.90.0") match; rg search returned no explicit CI matches (rg skipped files).
Apply this optional diff to hyperlink the commit:
-**Git Commit**: 1445afc3d898bb5907f0dffe8c38de445a6a71ed +**Git Commit**: [1445afc3d898bb5907f0dffe8c38de445a6a71ed](https://github.com/acgetchell/delaunay/commit/1445afc3d898bb5907f0dffe8c38de445a6a71ed)src/geometry/predicates.rs (2)
329-347: Doc example import path: consider using predicates::InSphere for consistencyIf
InSphereisn’t re-exported atgeometry, doctests may fail. Prefer importing from the same module as the function:-/// use delaunay::geometry::predicates::insphere; -/// use delaunay::geometry::InSphere; +/// use delaunay::geometry::predicates::{insphere, InSphere};If
InSphereis intentionally re-exported atgeometry, ignore.
964-1136: Optional: reduce duplication in orientation testsThe 2D–5D positive/negative/degenerate suites repeat similar patterns. Consider small helpers/macros to generate point sets per dimension and assert expected orientation to keep maintenance low.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitleaks.toml(1 hunks)benches/PERFORMANCE_RESULTS.md(4 hunks)src/geometry/predicates.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T20:03:49.896Z
Learnt from: acgetchell
PR: acgetchell/delaunay#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/PERFORMANCE_RESULTS.mdsrc/geometry/predicates.rs
🧬 Code graph analysis (1)
src/geometry/predicates.rs (2)
src/geometry/traits/coordinate.rs (3)
default_tolerance(521-521)default_tolerance(551-553)default_tolerance(561-563)src/geometry/util.rs (2)
safe_scalar_to_f64(394-396)circumradius(824-830)
⏰ 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: build (windows-latest)
- GitHub Check: performance-regression
🔇 Additional comments (13)
.gitleaks.toml (3)
1-9: Good shift from file-level allowlists to literal-scoped patternsThis avoids masking unrelated leaks and aligns with best practices.
10-13: Confirmed:[extend] useDefault = trueis supported
Gitleaks supports an [extend] table withuseDefault = trueto load built‑in/default rules (per README/docs); no change required.
14-21: Allowlist verified — literals present
- src/core/boundary.rs:915 — 0x1234_5678_9ABC_DEF0
- src/geometry/algorithms/convex_hull.rs:4213 — 0x1234_5678_90ab_cdef
- src/geometry/algorithms/convex_hull.rs:4273 — 0xdead_beef_cafe_babe
Allowlist is up-to-date.
benches/PERFORMANCE_RESULTS.md (4)
24-26: 2D table updates look consistent.Numbers and winner are coherent with the summary claim.
32-34: 3D table updates look consistent.Ratios align with the “~1.8x slower” statement for insphere_distance.
40-41: Verify 4D Far vertex row wasn’t missed.Basic/Boundary updated, but Far (Line 42) remains at 1.0 µs for insphere_lifted. Confirm that the generator intentionally left it unchanged, or refresh if a new value exists.
48-50: 5D table updates look consistent.Uniform improvements and winner preserved.
src/geometry/predicates.rs (6)
402-409: Determinant sign normalization by orientation looks correctApplying orientation sign to normalize
det(no parity flip in this “standard determinant” path) is consistent with the documented approach above.Please confirm tests comparing
inspherevsinsphere_liftedpass across 2D–5D after the parity fix ininsphere_lifted.
600-608: Parity- and orientation-aware normalization in lifted path is soundThe combined
parity_sign(even dims invert) ×orient_signnormalization is the right way to align lifted determinant results with geometric expectations across dimensions.
251-266: Clear doc note distinguishing determinant vs lifted parity conventionsGood clarification that parity handling is required for the lifted formulation, not the standard determinant path. This reduces future confusion.
648-735: Nice coverage of circumradius across dimensionsSolid assertions and tolerances; the increasing-radius pattern checks are a helpful sanity test.
1591-1597: Great: stress test enforces cross-method agreementThis will guard against future regressions in sign interpretation.
593-596: Fix: usize has no is_multiple_of; use modulo for parity
D.is_multiple_of(2)is not a stable/available method forusize. Use a modulo parity check.- let dimension_is_even = D.is_multiple_of(2); + let dimension_is_even = D % 2 == 0;
| - **insphere vs insphere_lifted**: 100.0% agreement (different algorithms) | ||
| - **insphere_distance vs insphere_lifted**: 100.0% agreement | ||
| - **All three methods agree**: 100.0% (expected due to different numerical approaches) | ||
|
|
There was a problem hiding this comment.
Fix agreement wording to avoid implying agreement is “expected.”
Current parenthetical suggests 100% agreement is expected due to numerical differences, which is backwards. Clarify that 100% agreement was observed in this run but isn’t guaranteed.
Apply this diff:
-- **insphere vs insphere_lifted**: 100.0% agreement (different algorithms)
+- **insphere vs insphere_lifted**: 100.0% agreement (distinct numerical paths)
- **insphere_distance vs insphere_lifted**: 100.0% agreement
-- **All three methods agree**: 100.0% (expected due to different numerical approaches)
+- **All three methods agree**: 100.0% (observed in this run; not guaranteed across all inputs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **insphere vs insphere_lifted**: 100.0% agreement (different algorithms) | |
| - **insphere_distance vs insphere_lifted**: 100.0% agreement | |
| - **All three methods agree**: 100.0% (expected due to different numerical approaches) | |
| - **insphere vs insphere_lifted**: 100.0% agreement (distinct numerical paths) | |
| - **insphere_distance vs insphere_lifted**: 100.0% agreement | |
| - **All three methods agree**: 100.0% (observed in this run; not guaranteed across all inputs) |
🤖 Prompt for AI Agents
In benches/PERFORMANCE_RESULTS.md around lines 65 to 68, the parenthetical
currently implies 100% agreement is "expected due to different numerical
approaches," which is backwards; update the wording to state that 100% agreement
was observed in this run but is not guaranteed across different data sets or
numerical methods (e.g., replace the parenthetical with something like "observed
in this run — not guaranteed across all inputs or numerical approaches"). Ensure
the three bullet lines remain but adjust the final line to clarify observation
vs. expectation.
This pull request introduces several improvements and updates across the codebase, including raising the minimum supported Rust version, updating performance benchmarks and documentation, enhancing test coverage for the Bowyer-Watson algorithm, and optimizing cell validation logic for speed and consistency. The most important changes are summarized below.
Rust version and configuration alignment:
Cargo.toml,clippy.toml, andrust-toolchain.tomlfor consistency across development and tooling. [1] [2] [3]Benchmarking and documentation updates:
benches/PERFORMANCE_RESULTS.mdto reflect the latest run, including new timings and agreement percentages for algorithms, and changed the update utility references in documentation tobenchmark-utils. [1] [2] [3] [4] [5]cargo tarpaulintoWARP.md.README.mdregarding vertex and cell data types.Algorithm and trait test coverage:
src/core/algorithms/bowyer_watson.rscoveringDefaulttrait, insertion strategies, facet cache provider, insertion algorithm trait methods, and error handling, improving code reliability and branch coverage. [1] [2]Cell validation and deserialization optimizations:
src/core/cell.rsto useFastHashSetfor duplicate detection (replacingHashSet), streamlined imports, and improved code clarity and performance. [1] [2] [3] [4] [5] [6] [7] [8]Spell-check dictionary updates:
cspell.jsonto support additional dependencies and code identifiers. [1] [2] [3]