Closed
Conversation
…ure (#228) - Soften post-condition in normalize_and_promote_positive_orientation: add canonicalize_global_orientation_sign before the promote loop to prevent oscillation; demote the residual negative-orientation check from a hard error to a diagnostic log so near-degenerate but structurally valid simplices no longer abort insertion. - Replace enlarged local repair fallback with global repair_delaunay_with_flips_k2_k3 when D<4 per-insertion local repair cycles on co-spherical FP configurations. The multi-attempt global repair uses robust predicates and alternate queue orders to break cycling. - Gate global repair fallback on a new ConstructionOptions field (use_global_repair_fallback, default true) threaded through the build chain via DelaunayInsertionState. The periodic builder disables it (.without_global_repair_fallback()) so global repair cannot disrupt the image-point topology; the existing 24-attempt shuffle retry finds a working vertex ordering instead.
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
bug
![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
🧹 Nitpick comments (3)
src/core/triangulation.rs (1)
2307-2311: Add quantitative context to the residual-negative diagnostic.Please include a count (and ideally a small sample of
CellKeys) in this debug log so it is actionable during production triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/triangulation.rs` around lines 2307 - 2311, In normalize_and_promote_positive_orientation, update the tracing::debug call to include quantitative context by computing the number of cells that remain negative after promotion and a small sample (e.g., up to 5) of their CellKey values; collect these into a Vec or iterator (e.g., residual_negative_cells and sampled_keys) and log both the count and the sample (using Debug/Display for CellKey) in the debug message so the diagnostic reads like "X cells remain negative; sample CellKeys: [...]". Ensure sampling is cheap (take first N or use .iter().take(N).cloned()) and handle empty case gracefully.src/core/delaunay_triangulation.rs (2)
463-468: Add regression coverage for the new opt-out toggle.The toggle is behavior-critical; a focused test would guard against future wiring regressions.
✅ Suggested test addition
+ #[test] + fn test_without_global_repair_fallback_threads_to_insertion_state() { + init_tracing(); + let vertices: Vec<Vertex<f64, (), 3>> = vec![ + vertex!([0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0]), + vertex!([0.0, 1.0, 0.0]), + vertex!([0.0, 0.0, 1.0]), + vertex!([0.25, 0.25, 0.25]), + ]; + + let opts = ConstructionOptions::default() + .with_retry_policy(RetryPolicy::Disabled) + .without_global_repair_fallback(); + + let dt: DelaunayTriangulation<FastKernel<f64>, (), (), 3> = + DelaunayTriangulation::new_with_options(&vertices, opts).unwrap(); + + assert!(!dt.insertion_state.use_global_repair_fallback); + }🤖 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 463 - 468, Add a focused unit test that verifies the new opt-out toggle works by constructing the triangulation/config using the without_global_repair_fallback(...) builder method and asserting that the internal flag use_global_repair_fallback is set to false and that behavior changes accordingly (e.g., run a minimal triangulation scenario that would normally trigger the D<4 global repair fallback and assert the fallback is not invoked or that the alternative behavior occurs). Place the test in the same module (cfg(test)) near other tests in core/delaunay_triangulation.rs, use the public constructor/build flow that exposes without_global_repair_fallback, and assert either the internal flag or an observable behavioral difference to prevent future wiring regressions.
3012-3048: Consider extracting the duplicated D<4 fallback block into one helper.Both stats and non-stats branches now carry near-identical local-failure/global-fallback handling; a shared helper would reduce drift risk.
♻️ Refactor sketch
+ fn handle_d_lt4_local_repair_failure( + &mut self, + index: usize, + topology: TopologyGuarantee, + repair_err: &DelaunayRepairError, + ) -> Result<(), DelaunayTriangulationConstructionError> + where + K::Scalar: ScalarSummable, + { + if self.insertion_state.use_global_repair_fallback { + let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); + repair_delaunay_with_flips_k2_k3(tds, kernel, None, topology) + .map(|_| ()) + .map_err(|global_err| TriangulationConstructionError::GeometricDegeneracy { + message: format!( + "per-insertion Delaunay repair failed at index {index}: {global_err}" + ), + } + .into()) + } else { + Err(TriangulationConstructionError::GeometricDegeneracy { + message: format!( + "per-insertion Delaunay repair failed at index {index}: {repair_err}" + ), + } + .into()) + } + }- if D < 4 { - // duplicated block... - } + if D < 4 { + self.handle_d_lt4_local_repair_failure( + index, + topology, + &repair_err, + )?; + continue; + }Also applies to: 3201-3237
🤖 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 3012 - 3048, The duplicated "D<4 local repair cycling => global repair fallback" block should be extracted into a single helper to avoid drift between the stats and non-stats branches: add a private helper (e.g., handle_d_lt_4_fallback or try_local_then_global_repair) that takes the contextual values (index, repair_err for logging, topology, and mutable access to self.tri or its parts) and performs the tracing::debug logging, calls repair_delaunay_with_flips_k2_k3, logs the global_err on failure, and returns a Result<(), Box<dyn Error>> or a bool/enum indicating "continue loop" vs "propagate TriangulationConstructionError::GeometricDegeneracy"; then replace the duplicated if self.insertion_state.use_global_repair_fallback { ... } blocks with a single call to that helper and handle its return to either continue the loop or return the constructed GeometricDegeneracy error.
🤖 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/triangulation.rs`:
- Around line 2306-2313: The post-condition softening in
normalize_and_promote_positive_orientation() is neutralized because callers
(specifically the caller that currently invokes
normalize_and_promote_positive_orientation()? and then
validate_geometric_cell_orientation() ) still enforce strict validation; change
the API so normalize_and_promote_positive_orientation returns an explicit result
flag (e.g., enum or bool like promoted_all: bool or AcceptedResidualNegatives)
instead of only Result<(), Error>, update
normalize_and_promote_positive_orientation (and internal helpers
cells_require_positive_orientation_promotion and
canonicalize_global_orientation_sign if needed) to set that flag when it logs
"accepting coherent orientation", and modify the caller to check the returned
flag and skip or relax validate_geometric_cell_orientation() when residual
negatives were accepted. Ensure the new return type is propagated and handled
where normalize_and_promote_positive_orientation is called.
---
Nitpick comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 463-468: Add a focused unit test that verifies the new opt-out
toggle works by constructing the triangulation/config using the
without_global_repair_fallback(...) builder method and asserting that the
internal flag use_global_repair_fallback is set to false and that behavior
changes accordingly (e.g., run a minimal triangulation scenario that would
normally trigger the D<4 global repair fallback and assert the fallback is not
invoked or that the alternative behavior occurs). Place the test in the same
module (cfg(test)) near other tests in core/delaunay_triangulation.rs, use the
public constructor/build flow that exposes without_global_repair_fallback, and
assert either the internal flag or an observable behavioral difference to
prevent future wiring regressions.
- Around line 3012-3048: The duplicated "D<4 local repair cycling => global
repair fallback" block should be extracted into a single helper to avoid drift
between the stats and non-stats branches: add a private helper (e.g.,
handle_d_lt_4_fallback or try_local_then_global_repair) that takes the
contextual values (index, repair_err for logging, topology, and mutable access
to self.tri or its parts) and performs the tracing::debug logging, calls
repair_delaunay_with_flips_k2_k3, logs the global_err on failure, and returns a
Result<(), Box<dyn Error>> or a bool/enum indicating "continue loop" vs
"propagate TriangulationConstructionError::GeometricDegeneracy"; then replace
the duplicated if self.insertion_state.use_global_repair_fallback { ... } blocks
with a single call to that helper and handle its return to either continue the
loop or return the constructed GeometricDegeneracy error.
In `@src/core/triangulation.rs`:
- Around line 2307-2311: In normalize_and_promote_positive_orientation, update
the tracing::debug call to include quantitative context by computing the number
of cells that remain negative after promotion and a small sample (e.g., up to 5)
of their CellKey values; collect these into a Vec or iterator (e.g.,
residual_negative_cells and sampled_keys) and log both the count and the sample
(using Debug/Display for CellKey) in the debug message so the diagnostic reads
like "X cells remain negative; sample CellKeys: [...]". Ensure sampling is cheap
(take first N or use .iter().take(N).cloned()) and handle empty case gracefully.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 799e4543-859c-4e98-ae1c-f2bcf5d386b0
📒 Files selected for processing (4)
src/core/builder.rssrc/core/delaunay_triangulation.rssrc/core/operations.rssrc/core/triangulation.rs
Comment on lines
2306
to
2313
| if self.cells_require_positive_orientation_promotion()? { | ||
| return Err(InsertionError::TopologyValidation( | ||
| TdsValidationError::InconsistentDataStructure { | ||
| message: | ||
| "Failed to converge to positive geometric orientation after bounded promotion passes" | ||
| .to_string(), | ||
| }, | ||
| )); | ||
| tracing::debug!( | ||
| "normalize_and_promote_positive_orientation: \ | ||
| some cells still appear negative after bounded promotion passes \ | ||
| (likely near-degenerate FP noise); accepting coherent orientation" | ||
| ); | ||
| } | ||
| self.canonicalize_global_orientation_sign()?; |
There was a problem hiding this comment.
Softened post-condition is currently neutralized in the repair path.
This function now accepts residual negatives, but the caller in src/core/delaunay_triangulation.rs still does normalize_and_promote_positive_orientation()? at Line 5106 and then validate_geometric_cell_orientation()? at Line 5130. So near-degenerate cases still abort insertion, only later in the flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/triangulation.rs` around lines 2306 - 2313, The post-condition
softening in normalize_and_promote_positive_orientation() is neutralized because
callers (specifically the caller that currently invokes
normalize_and_promote_positive_orientation()? and then
validate_geometric_cell_orientation() ) still enforce strict validation; change
the API so normalize_and_promote_positive_orientation returns an explicit result
flag (e.g., enum or bool like promoted_all: bool or AcceptedResidualNegatives)
instead of only Result<(), Error>, update
normalize_and_promote_positive_orientation (and internal helpers
cells_require_positive_orientation_promotion and
canonicalize_global_orientation_sign if needed) to set that flag when it logs
"accepting coherent orientation", and modify the caller to check the returned
flag and skip or relax validate_geometric_cell_orientation() when residual
negatives were accepted. Ensure the new return type is propagated and handled
where normalize_and_promote_positive_orientation is called.
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 #231 +/- ##
==========================================
+ Coverage 61.43% 61.59% +0.15%
==========================================
Files 49 49
Lines 13189 13261 +72
==========================================
+ Hits 8103 8168 +65
- Misses 5086 5093 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
CodeRabbit review request