refactor: deduplicate D<4 repair fallback and improve diagnostics#232
Merged
acgetchell merged 7 commits intomainfrom Mar 7, 2026
Merged
refactor: deduplicate D<4 repair fallback and improve diagnostics#232acgetchell merged 7 commits intomainfrom
acgetchell merged 7 commits intomainfrom
Conversation
- Extract try_d_lt4_global_repair_fallback helper to eliminate duplicated repair-or-abort logic between the stats and non-stats branches of insert_remaining_vertices_seeded. - Enrich the soft post-condition diagnostic in normalize_and_promote_positive_orientation with the count of residual negative cells and a sample of up to 5 CellKeys. - Add test_construction_options_global_repair_fallback_toggle unit test verifying the without_global_repair_fallback builder toggle.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralizes D<4 repair fallback into a new helper, makes RobustKernel the default across constructors/serde/generation/benches/examples/tests, adds a topological admissibility check in flip repairs, and logs sampled negatively-oriented cell keys during orientation normalization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
rust
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 preferences |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
==========================================
+ Coverage 61.60% 61.84% +0.24%
==========================================
Files 49 49
Lines 13261 13238 -23
==========================================
+ Hits 8169 8187 +18
+ Misses 5092 5051 -41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Fixes #228 — 3D bulk Delaunay triangulation failures due to FP co-spherical misclassification with FastKernel. - Change all convenience constructors (new, new_with_topology_guarantee, new_with_options, empty, etc.) to use RobustKernel<f64> - Change builder build() default to RobustKernel<T> - Add RobustKernel to prelude::query exports - Update type annotations across tests, benches, and doc tests - Preserve FastKernel in tests that explicitly test it via with_kernel()
coderabbitai
bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/builder.rs (1)
786-789: Documentation is now slightly misleading.Since
build()now defaults toRobustKernel, the comment "Use this when you needRobustKernel..." is inaccurate—users getRobustKernelautomatically withbuild(). Consider updating to:- /// Use this when you need [`RobustKernel`] or - /// a custom kernel implementation. + /// Use this when you need [`FastKernel`] (for performance-critical paths) or + /// a custom kernel implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/builder.rs` around lines 786 - 789, The doc comment on the Builder::build() API is misleading: it states "Use this when you need `RobustKernel`" even though `build()` now defaults to `RobustKernel`; update the documentation for the builder method (the comment block above `build()` in builder.rs) to reflect that `build()` returns a triangulation using `RobustKernel` by default and clarify when callers should use the alternative kernel-taking API (reference the method name `build_with_kernel` or similar if present) for custom kernels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 2915-2918: The GeometricDegeneracy error message currently only
includes the global fallback error (global_err) and loses the original
per-insertion/local failure; update the
TriangulationConstructionError::GeometricDegeneracy construction to include both
the local/per-insertion error and the global_err in the formatted message (e.g.
"per-insertion Delaunay repair failed at index {index}: local error:
{local_err}; global fallback: {global_err}"), ensuring you reference the same
local error variable used where the fallback is invoked and keep the existing
index and global_err identifiers.
---
Nitpick comments:
In `@src/core/builder.rs`:
- Around line 786-789: The doc comment on the Builder::build() API is
misleading: it states "Use this when you need `RobustKernel`" even though
`build()` now defaults to `RobustKernel`; update the documentation for the
builder method (the comment block above `build()` in builder.rs) to reflect that
`build()` returns a triangulation using `RobustKernel` by default and clarify
when callers should use the alternative kernel-taking API (reference the method
name `build_with_kernel` or similar if present) for custom kernels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: cd5c08ee-8d82-4e47-856d-be6660b531e7
📒 Files selected for processing (13)
benches/large_scale_performance.rssrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/builder.rssrc/core/cell.rssrc/core/delaunay_triangulation.rssrc/core/traits/facet_cache.rssrc/geometry/algorithms/convex_hull.rssrc/lib.rstests/allocation_api.rstests/euler_characteristic.rstests/proptest_cell.rstests/proptest_delaunay_triangulation.rs
✅ Files skipped from review due to trivial changes (1)
- src/core/algorithms/flips.rs
Update examples and random triangulation utilities to use RobustKernel, aligning them with the core library's default. FastKernel is now explicitly documented as unreliable for 3D workloads due to floating- point precision issues in near-degenerate configurations. This change also adds topological admissibility checks for flip-based repairs and improves error diagnostics for per-insertion failures. Refs: issue-228
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/geometry/util/triangulation_generation.rs (1)
130-132: Consider unifyingRobustKernelconfiguration across generation paths.
random_triangulation_try_with_verticesusesdegenerate_robustconfig, butRandomTriangulationBuilder::build_with_vertex_datausesRobustKernel::new(). A shared kernel factory would reduce behavior drift between APIs.♻️ Suggested consolidation
+fn default_random_kernel<T: ScalarAccumulative>() -> RobustKernel<T> { + RobustKernel::with_config(config_presets::degenerate_robust::<T>()) +} ... - let robust_config = config_presets::degenerate_robust::<T>(); - let robust_kernel = RobustKernel::with_config(robust_config); + let robust_kernel = default_random_kernel::<T>(); ... - let kernel = RobustKernel::new(); + let kernel = default_random_kernel::<T>(); ... - let dt = DelaunayTriangulation::with_topology_guarantee_and_options( - &RobustKernel::new(), + let kernel = default_random_kernel::<T>(); + let dt = DelaunayTriangulation::with_topology_guarantee_and_options( + &kernel, &vertices, self.topology_guarantee, self.construction_options, )?;Also applies to: 651-652, 703-705
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/geometry/util/triangulation_generation.rs` around lines 130 - 132, Unify RobustKernel creation by introducing and using a shared factory so both random_triangulation_try_with_vertices and RandomTriangulationBuilder::build_with_vertex_data create kernels with the same config; replace direct calls to RobustKernel::new() with a call to a new helper (e.g., make_robust_kernel or robust_kernel_from_preset) that uses config_presets::degenerate_robust::<T>() and returns RobustKernel::with_config(...); update all occurrences noted (including the other spots around the referenced lines) to call this factory so behavior is consistent across APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/builder.rs`:
- Around line 492-495: Docs currently imply that calling build_with_kernel
guarantees reliable construction; instead update the wording to state that
reliability depends on the kernel type passed (e.g., RobustKernel) and not on
the method name. Modify the comment around
FastKernel/RobustKernel/build_with_kernel in builder.rs to recommend using
RobustKernel (or calling build_with_kernel with RobustKernel) for reliable
results and explicitly note that build_with_kernel alone does not make
FastKernel reliable.
---
Nitpick comments:
In `@src/geometry/util/triangulation_generation.rs`:
- Around line 130-132: Unify RobustKernel creation by introducing and using a
shared factory so both random_triangulation_try_with_vertices and
RandomTriangulationBuilder::build_with_vertex_data create kernels with the same
config; replace direct calls to RobustKernel::new() with a call to a new helper
(e.g., make_robust_kernel or robust_kernel_from_preset) that uses
config_presets::degenerate_robust::<T>() and returns
RobustKernel::with_config(...); update all occurrences noted (including the
other spots around the referenced lines) to call this factory so behavior is
consistent across APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 98a49c64-d005-440e-a3bd-2b433d7b64d3
📒 Files selected for processing (7)
examples/convex_hull_3d_100_points.rsexamples/triangulation_3d_100_points.rssrc/core/algorithms/flips.rssrc/core/builder.rssrc/core/delaunay_triangulation.rssrc/geometry/kernel.rssrc/geometry/util/triangulation_generation.rs
- Add make_robust_kernel<T>() factory in triangulation_generation.rs that uses degenerate_robust config preset; replace all direct RobustKernel::new() calls to ensure consistent predicate configuration across code paths - Change ValidationPolicy::Always → OnSuspicion during batch construction in both build_with_kernel_inner_seeded variants to avoid O(n²) full validation on every insertion - Clarify toroidal_periodic docs in builder.rs: reliability depends on the kernel type passed to build_with_kernel, not the method itself Related: #228
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/builder.rs (1)
774-785: Consider aligning defaultbuild()with the shared robust preset.
build()still usesRobustKernel::new(). Elsewhere in this PR, robust construction is centralized around a degenerate-safe preset. Using one default config across entry points would reduce behavior drift on near-degenerate inputs.Diff suggestion
+use crate::geometry::robust_predicates::config_presets; @@ pub fn build<V>( self, @@ { - self.build_with_kernel(&RobustKernel::new()) + let kernel = RobustKernel::with_config(config_presets::degenerate_robust::<T>()); + self.build_with_kernel(&kernel) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/builder.rs` around lines 774 - 785, The default build() currently calls build_with_kernel(&RobustKernel::new()), which diverges from the PR-wide degenerate-safe preset; change build() to call build_with_kernel with the shared robust preset used elsewhere in this PR (replace RobustKernel::new() with the centralized degenerate-safe RobustKernel configuration/function used by other entry points) so build(), build_with_kernel(), and other constructors use the same preset for near-degenerate inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 2749-2756: The PLManifoldStrict batch path is incorrectly using
ValidationPolicy::OnSuspicion even when
topology_guarantee.requires_vertex_links_during_insertion() is true; update the
logic that sets dt.tri.validation_policy so that if
topology_guarantee.requires_vertex_links_during_insertion() (or the combination
that demands per-insertion checks) is true you set ValidationPolicy::Strict (or
the per-insertion policy used elsewhere) instead of
ValidationPolicy::OnSuspicion; make the same change in the other seeded
constructor variant so both constructors respect
requires_vertex_links_during_insertion() and requires_ridge_links() and preserve
strict per-insertion validation.
---
Nitpick comments:
In `@src/core/builder.rs`:
- Around line 774-785: The default build() currently calls
build_with_kernel(&RobustKernel::new()), which diverges from the PR-wide
degenerate-safe preset; change build() to call build_with_kernel with the shared
robust preset used elsewhere in this PR (replace RobustKernel::new() with the
centralized degenerate-safe RobustKernel configuration/function used by other
entry points) so build(), build_with_kernel(), and other constructors use the
same preset for near-degenerate inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ce1d9195-6936-465a-acf1-bc61b0119b59
📒 Files selected for processing (3)
src/core/builder.rssrc/core/delaunay_triangulation.rssrc/geometry/util/triangulation_generation.rs
Correct the validation policy during batch construction so that topologies requiring vertex links maintain Always validation. This ensures structural guarantees are preserved during bulk operations. Additionally updates MSRV to 1.94, refreshes dependencies, and expands contributor guidelines in AGENTS.md. Refs: issue-228
Contributor
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 (1)
src/core/delaunay_triangulation.rs (1)
1493-1533:⚠️ Potential issue | 🟡 MinorUpdate these constructor docs to say
RobustKernel, not “fast-kernel”.The implementations now build with
RobustKernel<f64>, but these doc blocks still describe the old fast-kernel convenience path. That will misstate the default predicate/performance tradeoff for users.Also applies to: 1536-1572, 1604-1627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/delaunay_triangulation.rs` around lines 1493 - 1533, The doc comment for new_with_options incorrectly mentions a “fast-kernel” convenience path; update the docs to reference the actual kernel used (RobustKernel<f64>) and reword any statements about kernel/performance tradeoffs accordingly. Search for and edit the affected doc blocks around the new_with_options function (and the other similar blocks noted) to replace "fast-kernel" or "fast kernel" wording with "RobustKernel" and adjust phrases that imply a different default predicate/performance characteristic so the documentation accurately reflects that RobustKernel<f64> is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 48-53: Update the "Error handling" guideline to require returning
Result only for fallible public functions (e.g., constructors, I/O, parsing),
not for inherently infallible public APIs such as accessors or methods like
len/is_empty, iterators, or builder setters; state that infallible functions
should return values directly (use Option for fallible lookups) and that builder
setters should return self with fallibility reported by the final build()
method; keep the rule that private helper functions should return Result if they
propagate errors into public fallible functions and retain the instruction to
define module-local error types at the top of the relevant module/file.
In `@Cargo.toml`:
- Line 22: The dependency version for la-stack is incorrect; replace the
non-existent "0.2.0" with a published 0.1.x release (e.g., "0.1.3") in the
Cargo.toml so the crate resolves; ensure the change preserves compatibility with
existing usages like lu(), ldlt(), det(), solve_vec(), LaError::Singular,
LaError::NonFinite, and LaVector::new().
---
Outside diff comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 1493-1533: The doc comment for new_with_options incorrectly
mentions a “fast-kernel” convenience path; update the docs to reference the
actual kernel used (RobustKernel<f64>) and reword any statements about
kernel/performance tradeoffs accordingly. Search for and edit the affected doc
blocks around the new_with_options function (and the other similar blocks noted)
to replace "fast-kernel" or "fast kernel" wording with "RobustKernel" and adjust
phrases that imply a different default predicate/performance characteristic so
the documentation accurately reflects that RobustKernel<f64> is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 30f82fdc-bb0a-4138-bfd4-c2e42f26053f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
AGENTS.mdCargo.tomlclippy.tomlexamples/memory_analysis.rsrust-toolchain.tomlsrc/core/delaunay_triangulation.rs
Establish clearer distinctions between fallible and infallible public APIs. Recommend using Option for lookups and deferring builder errors to the final build method to ensure consistent error propagation across the library. Refs: issue-228
This was referenced Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extract try_d_lt4_global_repair_fallback helper to eliminate duplicated repair-or-abort logic between the stats and non-stats branches of insert_remaining_vertices_seeded.
Enrich the soft post-condition diagnostic in normalize_and_promote_positive_orientation with the count of residual negative cells and a sample of up to 5 CellKeys.
Add test_construction_options_global_repair_fallback_toggle unit test verifying the without_global_repair_fallback builder toggle.