refactor(geometry)!: adopt la-stack 0.4.3 API (#424)#438
Merged
Conversation
- Route stack-matrix dispatch, checked access, determinant filters, and singular tolerances through the local geometry matrix shim. - Preserve typed la-stack solve and factorization errors in geometry error paths instead of stringifying backend diagnostics. - Use rounded exact solve fallback for circumcenters and reject non-finite predicate matrices at construction boundaries. - Make `.toroidal()` the periodic image-point constructor and move wrapping-only construction to `.canonicalized_toroidal()`. - Align pinned just, rumdl, taplo, dprint, and typos setup through cargo installs instead of Homebrew. - Disable la-stack default features explicitly while selecting exact arithmetic. BREAKING CHANGE: `.toroidal()` now builds the periodic image-point quotient. Callers that only want coordinate wrapping must use `.canonicalized_toroidal()`. BREAKING CHANGE: `.toroidal_periodic()` has been removed. BREAKING CHANGE: Error types that can carry la-stack diagnostics now implement PartialEq without Eq because la-stack 0.4.3 diagnostics can contain floating point payloads. BREAKING CHANGE: `geometry::matrix::SINGULARITY_TOLERANCE` has been removed; singular solve tolerances are now owned by the la-stack shim. Closes #424
Contributor
WalkthroughSplit toroidal builder modes into explicit ChangesBuilders, Tests, and Docs (Toroidal modes)
la-stack Boundary, Matrix, and Predicate Changes
Circumsphere, Measures, and Error Plumbing
Eq-derive removals & comment cleanup
CI, justfile, and Tooling Docs
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 7 |
🟢 Coverage 96.74% diff coverage · +0.45% coverage variation
Metric Results Coverage variation ✅ +0.45% coverage variation (-1.00%) Diff coverage ✅ 96.74% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2f310d9) 63058 57083 90.52% Head commit (b1c19fd) 62824 (-234) 57154 (+71) 90.97% (+0.45%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#438) 184 178 96.74% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
rust
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #438 +/- ##
==========================================
+ Coverage 90.51% 90.95% +0.43%
==========================================
Files 71 71
Lines 62848 62595 -253
==========================================
+ Hits 56888 56932 +44
+ Misses 5960 5663 -297
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
239-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify that this example is canonicalized, not periodic.
Line 241 says “periodic boundary conditions,” but the snippet uses
.canonicalized_toroidal([..]), which does not build periodic quotient neighbor rewiring. Please reword the intro (or switch the snippet to.toroidal([..])) so users choose the correct mode.✏️ Suggested doc tweak
-For periodic boundary conditions, use `DelaunayTriangulationBuilder`: +For toroidal wrapping (canonicalized coordinates without periodic quotient rewiring), use `DelaunayTriangulationBuilder`:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 239 - 257, The README currently says "periodic boundary conditions" but the example uses DelaunayTriangulationBuilder::canonicalized_toroidal which is not the periodic (quotient-rewired) mode; either change the explanatory text to clarify this example demonstrates the canonicalized toroidal mode (mentioning canonicalized_toroidal specifically) or swap the example call to DelaunayTriangulationBuilder::toroidal to show true periodic/quotient behavior; update the surrounding sentence to reference the chosen symbol (canonicalized_toroidal or toroidal) so readers pick the correct mode.src/geometry/matrix.rs (1)
208-228:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid NaN fallback in
determinanterror path.Line 227 returns
f64::NANfor backend errors, which silently discards sign/error semantics and can mask predicate failures. Please surface this as a typed error path instead of collapsing to NaN.As per coding guidelines, “No f64 operation may silently lose sign information; unwrap_or(NaN), unwrap_or(f64::INFINITY) … are anti-patterns.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/geometry/matrix.rs` around lines 208 - 228, The determinant function currently swallows backend errors by returning f64::NAN; change it to surface errors as a typed result instead: update pub fn determinant<const D: usize>(m: &Matrix<D>) -> Result<f64, LaError> (or add a new fallible variant like determinant_checked) and propagate m.det()'s Result directly (map Ok(det) and forward Err(e), handling LaError::Singular by returning Ok(0.0) if you want to preserve that behavior), update the function docstring and any callers/tests to handle Result, and remove the NaN fallback branch that returns f64::NAN; keep references to Matrix, determinant, m.det(), and LaError::Singular when making the change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/geometry/util/measures.rs`:
- Around line 271-275: Change gram_determinant_ldlt to return Result<f64,
CircumcenterError> instead of f64; remove the mapping of determinant errors to
f64::NAN and instead propagate the underlying factorization/determinant errors
by converting LaError into CircumcenterError (preserve error context) when
ldlt.det() returns Err. Update the match on ldlt.det() to return Ok(det) on
success and Err(...) with a wrapped CircumcenterError for all failure cases
(including singular), and then update callers of gram_determinant_ldlt to handle
the Result (propagate or map to higher-level errors) rather than expecting NaN
sentinel values.
---
Outside diff comments:
In `@README.md`:
- Around line 239-257: The README currently says "periodic boundary conditions"
but the example uses DelaunayTriangulationBuilder::canonicalized_toroidal which
is not the periodic (quotient-rewired) mode; either change the explanatory text
to clarify this example demonstrates the canonicalized toroidal mode (mentioning
canonicalized_toroidal specifically) or swap the example call to
DelaunayTriangulationBuilder::toroidal to show true periodic/quotient behavior;
update the surrounding sentence to reference the chosen symbol
(canonicalized_toroidal or toroidal) so readers pick the correct mode.
In `@src/geometry/matrix.rs`:
- Around line 208-228: The determinant function currently swallows backend
errors by returning f64::NAN; change it to surface errors as a typed result
instead: update pub fn determinant<const D: usize>(m: &Matrix<D>) -> Result<f64,
LaError> (or add a new fallible variant like determinant_checked) and propagate
m.det()'s Result directly (map Ok(det) and forward Err(e), handling
LaError::Singular by returning Ok(0.0) if you want to preserve that behavior),
update the function docstring and any callers/tests to handle Result, and remove
the NaN fallback branch that returns f64::NAN; keep references to Matrix,
determinant, m.det(), and LaError::Singular when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 98555f1b-5c04-4bd9-95bb-71ace781169f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
.github/workflows/ci.yml.github/workflows/codecov.ymlCargo.tomlREADME.mdbenches/common/flip_workflows.rsdocs/ORIENTATION_SPEC.mddocs/api_design.mddocs/dev/tooling-alignment.mddocs/invariants.mddocs/limitations.mddocs/numerical_robustness_guide.mddocs/topology.mddocs/workflows.mdjustfilesrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/collections/aliases.rssrc/core/collections/helpers.rssrc/core/collections/secondary_maps.rssrc/core/construction.rssrc/core/facet.rssrc/core/query.rssrc/core/simplex.rssrc/core/tds.rssrc/core/util/delaunay_validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/matrix.rssrc/geometry/predicates.rssrc/geometry/quality.rssrc/geometry/sos.rssrc/geometry/traits/coordinate.rssrc/geometry/util/circumsphere.rssrc/geometry/util/measures.rssrc/lib.rssrc/topology/manifold.rssrc/topology/traits/global_topology_model.rssrc/topology/traits/topological_space.rstests/circumsphere_debug_tools.rstests/delaunay_edge_cases.rstests/regressions.rstests/triangulation_builder.rs
- Encapsulate stack-allocated linear algebra behind the geometry matrix shim so the rest of Delaunay avoids depending on la-stack internals. - Preserve typed la-stack backend failures through determinant and Gram-measure paths instead of converting them to NaN sentinels. - Re-export LaError from the focused geometry prelude for callers of fallible matrix helpers. - Refresh README, roadmap, and code-organization docs around focused preludes, feature coverage, topology modes, and current performance limits. BREAKING CHANGE: geometry::matrix::determinant now returns Result<f64, LaError> instead of f64, so callers must handle backend determinant errors explicitly. Closes #424
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
402-402:⚠️ Potential issue | 🟡 MinorFix broken README anchor to the toroidal workflow section
docs/workflows.md#builder-api-toroidal-periodic-triangulationsdoesn’t exist indocs/workflows.md(no matching heading or explicit id/anchor). The toroidal section present is## Builder API: toroidal construction modes(anchor would be#builder-api-toroidal-construction-modes).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 402, The README link target is pointing to a non-existent anchor; update the link target for "[toroidal construction workflow]" to the correct anchor by replacing docs/workflows.md#builder-api-toroidal-periodic-triangulations with docs/workflows.md#builder-api-toroidal-construction-modes so it matches the "## Builder API: toroidal construction modes" heading.Source: Learnings
🧹 Nitpick comments (1)
src/geometry/matrix.rs (1)
375-394: ⚡ Quick winCover
determinantacross D=2 through D=5.The new
Result<f64, LaError>contract is only exercised onMatrix<2>. Since this boundary is dimension-generic, a regression in the 3×3–5×5 paths would currently slip through. Please add macro-based regular, singular, and non-finite cases for D=2 through D=5.As per coding guidelines: “Unit tests must cover known values, error paths, and dimension-generic correctness; dimension-generic tests MUST cover D=2 through D=5 whenever feasible using pastey macros”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/geometry/matrix.rs` around lines 375 - 394, Add dimension-generic tests for determinant across D=2..5 by creating a small macro (using paste or a simple macro_rules!) that generates three tests per dimension: a regular case asserting determinant(&Matrix::<D>::try_from_rows(...)) == expected with assert_relative_eq!, a singular case asserting 0.0, and a non-finite backend error case asserting Err(LaError::NonFinite { .. }) with assert_matches!; reference the determinant function, Matrix::<N>::try_from_rows, LaError::NonFinite, assert_relative_eq!, and assert_matches! in the macro so each generated test covers D=2 through D=5.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@README.md`:
- Line 402: The README link target is pointing to a non-existent anchor; update
the link target for "[toroidal construction workflow]" to the correct anchor by
replacing docs/workflows.md#builder-api-toroidal-periodic-triangulations with
docs/workflows.md#builder-api-toroidal-construction-modes so it matches the "##
Builder API: toroidal construction modes" heading.
---
Nitpick comments:
In `@src/geometry/matrix.rs`:
- Around line 375-394: Add dimension-generic tests for determinant across D=2..5
by creating a small macro (using paste or a simple macro_rules!) that generates
three tests per dimension: a regular case asserting
determinant(&Matrix::<D>::try_from_rows(...)) == expected with
assert_relative_eq!, a singular case asserting 0.0, and a non-finite backend
error case asserting Err(LaError::NonFinite { .. }) with assert_matches!;
reference the determinant function, Matrix::<N>::try_from_rows,
LaError::NonFinite, assert_relative_eq!, and assert_matches! in the macro so
each generated test covers D=2 through D=5.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4f2c1583-a905-4fd2-ba04-f7fc5a7bdfba
📒 Files selected for processing (8)
README.mddocs/code_organization.mddocs/roadmap.mdsrc/geometry/matrix.rssrc/geometry/util/measures.rssrc/lib.rstests/circumsphere_debug_tools.rstests/prelude_exports.rs
✅ Files skipped from review due to trivial changes (2)
- docs/roadmap.md
- docs/code_organization.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
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.
.toroidal()the periodic image-point constructor and move wrapping-only construction to.canonicalized_toroidal().BREAKING CHANGE:
.toroidal()now builds the periodic image-point quotient. Callers that only want coordinate wrapping must use.canonicalized_toroidal().BREAKING CHANGE:
.toroidal_periodic()has been removed.BREAKING CHANGE: Error types that can carry la-stack diagnostics now implement PartialEq without Eq because la-stack 0.4.3 diagnostics can contain floating point payloads.
BREAKING CHANGE:
geometry::matrix::SINGULARITY_TOLERANCEhas been removed; singular solve tolerances are now owned by the la-stack shim.Closes #424