refactor(api)!: parse coordinate inputs into validated types (#440)#444
Merged
Conversation
- Add CoordinateRange and route generator and Hilbert range inputs through typed boundary parsing before internal use. - Replace stringly numeric diagnostics with typed coordinate, count, range, and error-reason payloads across geometry and generator APIs. - Restrict the public coordinate scalar contract to f64 while documenting future exact-coordinate support as an explicit API addition. - Move geometry and generator error types into their owning modules and update prelude exports, docs, examples, and semgrep guardrails accordingly. BREAKING CHANGE: generator, coordinate, geometry diagnostic, and prelude APIs now expose CoordinateRange and typed error payloads instead of several raw tuple, f32, and stringly diagnostic surfaces. Closes #440
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCentralizes coordinate handling on a validated CoordinateRange, narrows caller-visible coordinate scalar to f64, replaces stringly error payloads with typed diagnostics, makes HashGridIndex construction fallible, and threads these changes through generators, insertion/construction/error plumbing, prelude exports, docs, tests, and CI/tooling. ChangesTyped coordinate model overhaul
Estimated code review effort Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 157 |
🟢 Coverage 90.73% diff coverage · -0.02% coverage variation
Metric Results Coverage variation ✅ -0.02% coverage variation (-1.00%) Diff coverage ✅ 90.73% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (ca95380) 63031 57447 91.14% Head commit (27b5f0e) 63739 (+708) 58077 (+630) 91.12% (-0.02%) 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 (#444) 1878 1704 90.73% 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
documentation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
- Coverage 91.11% 91.09% -0.03%
==========================================
Files 71 72 +1
Lines 62802 63527 +725
==========================================
+ Hits 57225 57867 +642
- Misses 5577 5660 +83
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: 3
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/incremental_insertion.rs (1)
649-734:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve retryability for wrapped insertion-stage bootstrap failures.
Lines 649-734 now funnel several insertion-layer failures into
InitialSimplexConstructionError::UnexpectedInsertionStage, but Lines 1752-1765 classify that entire bucket as non-retryable. That suppresses perturbation retry for cases that are already retryable at the insertion layer—e.g.InsertionNonManifoldTopology, retryableConflictErrorvariants, retryableHullExtensionReasonvariants, and retryable Level 3 topology failures—so geometrically sensitive bootstrap failures will hard-fail instead of retrying from the rolled-back snapshot.Suggested direction
CavityFillingError::InitialSimplexConstruction { reason } => match reason { InitialSimplexConstructionError::TdsValidation { source } => { Self::is_tds_validation_failure_retryable(source) } InitialSimplexConstructionError::GeometricDegeneracy { .. } => true, + InitialSimplexConstructionError::UnexpectedInsertionStage { reason } => { + match reason.as_ref() { + InitialSimplexUnexpectedInsertionStage::CavityFilling { source } => { + Self::is_cavity_filling_error_retryable(source) + } + InitialSimplexUnexpectedInsertionStage::ConflictRegion { source } => { + matches!( + source, + ConflictError::NonManifoldFacet { .. } + | ConflictError::RidgeFan { .. } + | ConflictError::DisconnectedBoundary { .. } + | ConflictError::OpenBoundary { .. } + ) + } + InitialSimplexUnexpectedInsertionStage::NonManifoldTopology { .. } => true, + InitialSimplexUnexpectedInsertionStage::HullExtension { reason } => { + matches!( + reason, + HullExtensionReason::NoVisibleFacets + | HullExtensionReason::InvalidPatch { .. } + ) + } + InitialSimplexUnexpectedInsertionStage::TopologyValidation { source } => { + Self::is_level3_error_retryable(source) + } + InitialSimplexUnexpectedInsertionStage::Location { .. } + | InitialSimplexUnexpectedInsertionStage::DelaunayValidation { .. } + | InitialSimplexUnexpectedInsertionStage::FinalTopologyValidation { .. } => false, + } + } InitialSimplexConstructionError::DuplicateUuid { .. } | InitialSimplexConstructionError::FailedToCreateSimplex { .. } | InitialSimplexConstructionError::InsufficientVertices { .. } | InitialSimplexConstructionError::InternalInconsistency { .. } | InitialSimplexConstructionError::DuplicateCoordinates { .. } | InitialSimplexConstructionError::LocalRepairBudgetExceeded { .. } - | InitialSimplexConstructionError::UnsupportedPeriodicDimension { .. } - | InitialSimplexConstructionError::UnexpectedInsertionStage { .. } => false, + | InitialSimplexConstructionError::UnsupportedPeriodicDimension { .. } => false, },Based on learnings: perturbation retry in this file operates on a rolled-back snapshot, so geometrically sensitive wrapped errors must keep their per-variant retryability instead of inheriting the stricter in-place fallback rules.
Also applies to: 1752-1765
🤖 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/core/algorithms/incremental_insertion.rs` around lines 649 - 734, The mapping from TriangulationConstructionError into InitialSimplexConstructionError currently collapses many insertion-layer errors into UnexpectedInsertionStage, losing per-variant retryability; update the From<TriangulationConstructionError> (the match arms creating InitialSimplexConstructionError::UnexpectedInsertionStage) to preserve the original retryable variants instead of flattening them — specifically special-case and forward the retryable TriangulationConstructionError variants (e.g., InsertionNonManifoldTopology, InsertionConflictRegion / ConflictError variants, InsertionHullExtension (and its retryable HullExtensionReason variants), InsertionTopologyValidation, FinalTopologyValidation and any retryable Level‑3 topology or Delaunay/validation errors) into corresponding InitialSimplexUnexpectedInsertionStage variants that retain the inner source/reason (box the source where needed) or include the original TriangulationConstructionError variant so the later retryability-check (the logic around the 1752-1765 classification) can inspect and preserve their retryable status.Source: Learnings
src/geometry/util/measures.rs (1)
164-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject overflowed intermediates before classifying these fast paths as degenerate.
These branches do raw floating-point products before any finiteness check. With large but finite coordinates, the intermediate length/cross/triple terms can overflow to
inf; then the tolerance path seesinf <= infand reports a typed degeneracy, and the 1D/2D length paths can even returnOk(inf)unchecked. That means valid but large inputs get either the wrong error class or a non-finite measure instead of an explicit numeric failure.Also applies to: 193-209, 229-251, 580-597, 616-637
🤖 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/util/measures.rs` around lines 164 - 181, The fast-paths in measures.rs (e.g., the 1D length branch computing diff/length using points[..].coords(), the 2D cross/triple routines referenced around lines 193-209, 229-251, and the other branches noted at 580-597 and 616-637) compute raw floating intermediates without finiteness checks, so large-but-finite inputs can produce inf/NaN and then be misclassified as degenerate or return Ok(inf); before classifying degeneracy or returning a numeric measure, explicitly validate that all intermediate results (diff components, cross products, triple products, and computed length/cross/triple scalars) are finite (not NaN/inf) and if any are not finite, return an appropriate numeric-failure error (rather than DegenerateSimplex or Ok(inf)), e.g., propagate a CircumcenterError indicating numeric overflow/invalid numeric result; apply this finite-check pattern to the 1D branch (uses Float::abs, T::zero(), points[..].coords()), the 2D cross/area branches and the higher-dimension branches named above.src/geometry/util/conversions.rs (1)
359-379:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAllow
2^53through theusizeprecision guard.
f64exactly represents every integer up to and including2^53, butmax_precise_u128is currently computed as(1 << 53) - 1. That makessafe_usize_to_scalar(9_007_199_254_740_992)fail even though the value round-trips throughf64exactly.Suggested fix
- let max_precise_u128: u128 = (1u128 << max_precise_bits) - 1; + let max_precise_u128: u128 = 1u128 << max_precise_bits;🤖 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/util/conversions.rs` around lines 359 - 379, The precision guard currently sets max_precise_u128 = (1u128 << max_precise_bits) - 1 which disallows 2^53 even though f64 represents integers up to and including 2^53; change the computation to max_precise_u128 = 1u128 << max_precise_bits (remove the -1) so the existing comparison if u128::from(value_u64) > max_precise_u128 correctly permits values equal to 2^53; update the constant/variable in the same block (F64_MANTISSA_BITS, t_mantissa_bits, max_precise_bits, max_precise_u128, value_u64/safe_usize_to_scalar) accordingly.src/geometry/util/point_generation.rs (1)
845-890:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerived grid coordinates still need finiteness checks.
Only the raw
spacingandoffsetinputs are checked for finiteness. Large finite values can still makeoffset[d] + index_as_scalar * spacingoverflow to±∞, and this path will currently returnOk(...)with invalidPoints instead of a typed error.🤖 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/util/point_generation.rs` around lines 845 - 890, The computed per-dimension coordinate (offset[d] + index_as_scalar * spacing) can overflow to ±∞ even when inputs are finite; after computing coords (or each coords[d]) in the loop that calls grid_index_as_scalar::<T, D>, check that the resulting scalar is finite (use the existing is_finite_generic() on the computed value) and if not return a typed RandomPointGenerationError (e.g., an InvalidGeneratedGridCoordinate/InvalidGridCoordinate variant) carrying axis and InvalidCoordinateValue::from_debug(&coords[d])—update the point-generation loop that uses spacing, offset, grid_index_as_scalar, coords, and RandomPointGenerationError to perform this validation before pushing to points.
🧹 Nitpick comments (3)
semgrep.yaml (2)
417-420: ⚡ Quick winBroaden generator guardrail coverage to all generator APIs.
delaunay.rust.no-stringly-generator-error-payloadscurrently only scanssrc/geometry/util/point_generation.rs(Line 418), but this PR’s scope includes typed payload enforcement across generator surfaces (including triangulation generation/builder boundaries). Please includesrc/geometry/util/triangulation_generation.rs(and any generator-facing builder module) so regressions can’t slip through unscanned paths.🤖 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 `@semgrep.yaml` around lines 417 - 420, Update the semgrep rule scope for delaunay.rust.no-stringly-generator-error-payloads by adding the triangulation generator files to the include list so all generator APIs are scanned; specifically modify the include array that currently lists "/src/geometry/util/point_generation.rs" to also include "/src/geometry/util/triangulation_generation.rs" and any generator-facing builder modules (e.g., "/src/geometry/util/triangulation_builder.rs" or similar), ensuring the pattern-either block remains unchanged so the rule will catch stringly-typed error payload usages across both point and triangulation generator surfaces.
421-426: ⚡ Quick winScope the
Stringfield-name regex to error variants to avoid false positives.At Line 423, the regex matches bare field names (
min_distance|bounds_*|bounds) withStringtype regardless of enclosing enum/variant, which can flag unrelated structs inpoint_generation.rs. Constrain this pattern with variant context (as done in Line 422/425 style) to keep rule signal high.🤖 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 `@semgrep.yaml` around lines 421 - 426, The current pattern-regex '^\s*(?:min_distance|bounds_min|bounds_max|bounds)\s*:\s*String\s*,' is too broad and must be constrained to enum/variant bodies; replace it with a variant-scoped regex like (?s)\b[A-Za-z0-9_]+\s*\{[^}]*\b(?:min_distance|bounds_min|bounds_max|bounds)\s*:\s*String\s*, so the rule only matches when those field names appear inside an enum/variant body (matching the style used by the surrounding rules such as the patterns for InvalidPositiveScalar and CoordinateConversionFailed).src/geometry/util/triangulation_generation.rs (1)
741-747: ⚡ Quick winMark
RandomTriangulationBuilderas#[must_use].This is a public wrapper type, and dropping it accidentally in a chained setup call currently goes unnoticed.
♻️ Suggested fix
+#[must_use] pub struct RandomTriangulationBuilder<T> { n_points: NonZeroUsize, bounds: CoordinateRange<T>, seed: Option<u64>, topology_guarantee: TopologyGuarantee,As per coding guidelines, "public wrapper types must be #[must_use]".
🤖 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/util/triangulation_generation.rs` around lines 741 - 747, Add the #[must_use] attribute to the public struct RandomTriangulationBuilder to prevent accidental drops in chained builder calls; locate the struct declaration named RandomTriangulationBuilder<T> and place #[must_use] directly above it (preserving the generic parameter and visibility) so the compiler will warn when instances are created and not used.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/delaunay/insertion.rs`:
- Around line 83-87: The code silently ignores errors from
HashGridIndex::try_new by using an early return; change the enclosing function
(e.g., the insertion function in src/delaunay/insertion.rs) to return a Result
type and propagate the error instead of returning void. Replace the let Ok(...)
else { return; } pattern for HashGridIndex::try_new(duplicate_tolerance) with a
?-style or match that maps the error into the function's Result (preserving the
original error type or converting to your crate's error enum), and update call
sites to handle the returned Result accordingly so the spatial-index
construction failure is surfaced to callers.
In `@src/geometry/coordinate_range.rs`:
- Around line 155-161: The current try_new uses `min >= max` which treats
incomparable `PartialOrd` as OK; change `CoordinateRange::try_new` to use
`min.partial_cmp(&max)` and match on the result, returning
`Err(CoordinateRangeError::NonIncreasing { ordering:
CoordinateRangeOrdering::Equal } )` for `Some(Equal)`, `Decreasing` for
`Some(Less/Greater)` as appropriate, and explicitly reject `None` with a new
`NonIncreasing` variant or an appropriate error case so incomparable bounds
fail; update any code paths expecting total ordering and add a regression test
that constructs a finite scalar with incomparable `PartialOrd` to assert
`try_new` returns Err.
In `@src/geometry/util/point_generation.rs`:
- Around line 667-675: Compute radius_sq and if it is not finite, do not proceed
silently — detect the overflow and bail out (or clamp) before the sampling loop
so you don't turn the ball sampler into a cube sampler; specifically, after let
radius_sq = radius * radius; check radius_sq.is_finite() (or equivalent for T)
and if false return an Err or panic with a clear message (or reduce radius to a
safe finite value) so the subsequent norm_sq <= radius_sq comparison cannot
accept every point from the bounding cube; update the error path where
radius/radius_sq are used (the variables radius, radius_sq and the while loop
that uses norm_sq) accordingly.
---
Outside diff comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 649-734: The mapping from TriangulationConstructionError into
InitialSimplexConstructionError currently collapses many insertion-layer errors
into UnexpectedInsertionStage, losing per-variant retryability; update the
From<TriangulationConstructionError> (the match arms creating
InitialSimplexConstructionError::UnexpectedInsertionStage) to preserve the
original retryable variants instead of flattening them — specifically
special-case and forward the retryable TriangulationConstructionError variants
(e.g., InsertionNonManifoldTopology, InsertionConflictRegion / ConflictError
variants, InsertionHullExtension (and its retryable HullExtensionReason
variants), InsertionTopologyValidation, FinalTopologyValidation and any
retryable Level‑3 topology or Delaunay/validation errors) into corresponding
InitialSimplexUnexpectedInsertionStage variants that retain the inner
source/reason (box the source where needed) or include the original
TriangulationConstructionError variant so the later retryability-check (the
logic around the 1752-1765 classification) can inspect and preserve their
retryable status.
In `@src/geometry/util/conversions.rs`:
- Around line 359-379: The precision guard currently sets max_precise_u128 =
(1u128 << max_precise_bits) - 1 which disallows 2^53 even though f64 represents
integers up to and including 2^53; change the computation to max_precise_u128 =
1u128 << max_precise_bits (remove the -1) so the existing comparison if
u128::from(value_u64) > max_precise_u128 correctly permits values equal to 2^53;
update the constant/variable in the same block (F64_MANTISSA_BITS,
t_mantissa_bits, max_precise_bits, max_precise_u128,
value_u64/safe_usize_to_scalar) accordingly.
In `@src/geometry/util/measures.rs`:
- Around line 164-181: The fast-paths in measures.rs (e.g., the 1D length branch
computing diff/length using points[..].coords(), the 2D cross/triple routines
referenced around lines 193-209, 229-251, and the other branches noted at
580-597 and 616-637) compute raw floating intermediates without finiteness
checks, so large-but-finite inputs can produce inf/NaN and then be misclassified
as degenerate or return Ok(inf); before classifying degeneracy or returning a
numeric measure, explicitly validate that all intermediate results (diff
components, cross products, triple products, and computed length/cross/triple
scalars) are finite (not NaN/inf) and if any are not finite, return an
appropriate numeric-failure error (rather than DegenerateSimplex or Ok(inf)),
e.g., propagate a CircumcenterError indicating numeric overflow/invalid numeric
result; apply this finite-check pattern to the 1D branch (uses Float::abs,
T::zero(), points[..].coords()), the 2D cross/area branches and the
higher-dimension branches named above.
In `@src/geometry/util/point_generation.rs`:
- Around line 845-890: The computed per-dimension coordinate (offset[d] +
index_as_scalar * spacing) can overflow to ±∞ even when inputs are finite; after
computing coords (or each coords[d]) in the loop that calls
grid_index_as_scalar::<T, D>, check that the resulting scalar is finite (use the
existing is_finite_generic() on the computed value) and if not return a typed
RandomPointGenerationError (e.g., an
InvalidGeneratedGridCoordinate/InvalidGridCoordinate variant) carrying axis and
InvalidCoordinateValue::from_debug(&coords[d])—update the point-generation loop
that uses spacing, offset, grid_index_as_scalar, coords, and
RandomPointGenerationError to perform this validation before pushing to points.
---
Nitpick comments:
In `@semgrep.yaml`:
- Around line 417-420: Update the semgrep rule scope for
delaunay.rust.no-stringly-generator-error-payloads by adding the triangulation
generator files to the include list so all generator APIs are scanned;
specifically modify the include array that currently lists
"/src/geometry/util/point_generation.rs" to also include
"/src/geometry/util/triangulation_generation.rs" and any generator-facing
builder modules (e.g., "/src/geometry/util/triangulation_builder.rs" or
similar), ensuring the pattern-either block remains unchanged so the rule will
catch stringly-typed error payload usages across both point and triangulation
generator surfaces.
- Around line 421-426: The current pattern-regex
'^\s*(?:min_distance|bounds_min|bounds_max|bounds)\s*:\s*String\s*,' is too
broad and must be constrained to enum/variant bodies; replace it with a
variant-scoped regex like
(?s)\b[A-Za-z0-9_]+\s*\{[^}]*\b(?:min_distance|bounds_min|bounds_max|bounds)\s*:\s*String\s*,
so the rule only matches when those field names appear inside an enum/variant
body (matching the style used by the surrounding rules such as the patterns for
InvalidPositiveScalar and CoordinateConversionFailed).
In `@src/geometry/util/triangulation_generation.rs`:
- Around line 741-747: Add the #[must_use] attribute to the public struct
RandomTriangulationBuilder to prevent accidental drops in chained builder calls;
locate the struct declaration named RandomTriangulationBuilder<T> and place
#[must_use] directly above it (preserving the generic parameter and visibility)
so the compiler will warn when instances are created and not used.
🪄 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: 8142b360-e183-4b77-86e4-777766f951be
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
Cargo.tomlREADME.mdbenches/common/flip_workflows.rsclippy.tomldocs/code_organization.mddocs/numerical_robustness_guide.mddocs/topology.mdexamples/point_comparison_and_hashing.rssemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/collections/spatial_hash_grid.rssrc/core/construction.rssrc/core/insertion.rssrc/core/operations.rssrc/core/repair.rssrc/core/simplex.rssrc/core/util/deduplication.rssrc/core/util/delaunay_validation.rssrc/core/util/hilbert.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/coordinate_range.rssrc/geometry/kernel.rssrc/geometry/point.rssrc/geometry/quality.rssrc/geometry/sos.rssrc/geometry/traits/coordinate.rssrc/geometry/util/circumsphere.rssrc/geometry/util/conversions.rssrc/geometry/util/measures.rssrc/geometry/util/point_generation.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/spaces/toroidal.rssrc/topology/traits/global_topology_model.rstests/benchmark_flip_fixtures.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/insert_with_statistics.rstests/prelude_exports.rstests/proptest_euler_characteristic.rstests/proptest_safe_conversions.rstests/semgrep/src/project_rules/rust_style.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (2)
- tests/triangulation_builder.rs
- tests/delaunay_edge_cases.rs
- Preserve spatial hash-grid construction failures as typed insertion, construction, and flip-neighbor wiring errors. - Reject invalid generator inputs at parse boundaries, including non-positive grid spacing, overflowing squared ball radii, non-finite generated grid coordinates, and incomparable coordinate ranges. - Report non-finite simplex and facet measure intermediates through structured geometry error payloads. - Update prelude exports, documentation, Semgrep coverage, uv workflow pins, and the routine 4D large-scale smoke scale. BREAKING CHANGE: generator and construction APIs now expose more precise typed error variants and payloads. `RandomPointGenerationError::InvalidGridSpacing` carries `InvalidPositiveScalar`, zero or negative grid spacing is rejected, coordinate ranges may report `CoordinateRangeOrdering::Incomparable`, and insertion/construction paths can return `SpatialIndexConstructionFailure`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/algorithms/incremental_insertion.rs`:
- Around line 5157-5166: The assert_matches! invocation currently moves err,
causing InsertionErrorSummary::from(err) to fail; change the assertion to borrow
err (e.g., assert_matches!(&err, ...)) or pass a clone (err.clone()) so err
remains available for InsertionErrorSummary::from; update the pattern to match a
borrowed InsertionError (e.g., InsertionError::SpatialIndexConstruction {
reason: SpatialIndexConstructionFailure::NonPositiveCellSize { value:
CoordinateConversionValue::Scalar(value) } } ) against &err or use err.clone()
before the assertion.
In `@src/delaunay/construction.rs`:
- Around line 4644-4646: The SpatialIndexConstruction variant is being mapped to
TriangulationConstructionError::SpatialIndexConstruction but is not treated as
non-retryable, so failures get retried and lose their structured reason; update
the non-retryable predicate (is_non_retryable_construction_error or equivalent)
to include TriangulationConstructionError::SpatialIndexConstruction (and/or
InsertionError::SpatialIndexConstruction depending on where you check) so that
SpatialIndexConstruction is classified as non-retryable and preserves its
structured reason across attempts.
- Around line 1964-1972: This loop in order_vertices_hilbert (iterating keyed,
using prev_q and pushing into deduped) is silently deduplicating quantized
buckets and must not drop entries when deduplication is disabled; either remove
the prev_q check or guard it behind the actual dedup policy so that when
DedupPolicy::Off (as used by order_vertices_by_strategy for
InsertionOrderStrategy::Hilbert) you always push every vertex into deduped
(preserving construction cardinality and allowing ConstructionStatistics to
account for duplicates), i.e., change the loop to always push vertex unless an
explicit dedup flag is set, referencing the keyed iterator, prev_q, and deduped
vector.
🪄 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: 73d0f150-2fcd-4ffe-9346-14cfde0f4fbd
📒 Files selected for processing (30)
.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/generate-baseline.yml.github/workflows/release-benchmarks.yml.github/workflows/semgrep-sarif.ymlREADME.mdbenches/README.mddocs/dev/tooling-alignment.mddocs/limitations.mddocs/roadmap.mdjustfilesemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/construction.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/insertion.rssrc/geometry/coordinate_range.rssrc/geometry/traits/coordinate.rssrc/geometry/util/circumsphere.rssrc/geometry/util/conversions.rssrc/geometry/util/measures.rssrc/geometry/util/point_generation.rssrc/geometry/util/triangulation_generation.rssrc/lib.rstests/README.mdtests/prelude_exports.rstests/proptest_safe_conversions.rstests/semgrep/src/project_rules/rust_style.rs
✅ Files skipped from review due to trivial changes (7)
- tests/README.md
- docs/roadmap.md
- docs/dev/tooling-alignment.md
- benches/README.md
- justfile
- docs/limitations.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- src/delaunay/insertion.rs
- src/core/construction.rs
- src/delaunay/builder.rs
- semgrep.yaml
- tests/semgrep/src/project_rules/rust_style.rs
- tests/prelude_exports.rs
- tests/proptest_safe_conversions.rs
- src/geometry/traits/coordinate.rs
- src/geometry/util/measures.rs
- src/geometry/util/conversions.rs
- src/geometry/util/point_generation.rs
- src/lib.rs
- src/geometry/util/triangulation_generation.rs
- Honor `DedupPolicy::Off` when applying Hilbert insertion order so quantized duplicate inputs remain visible to construction. - Treat spatial-index construction failures as non-retryable so callers keep the structured failure reason.
- Assert default batch construction does not hide duplicate inputs behind preprocessing dedup. - Cover explicit exact dedup for duplicate collapse, all-identical insufficiency, and Hilbert quantized collisions.
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.
BREAKING CHANGE: generator, coordinate, geometry diagnostic, and prelude APIs now expose CoordinateRange and typed error payloads instead of several raw tuple, f32, and stringly diagnostic surfaces.
Closes #440