refactor!: harden typed validation and exact predicate APIs - #375
Conversation
- Replace stringly validation and repair contexts with typed summaries and failure categories across insertion, flips, TDS, builder, and Delaunay validation paths. - Tighten kernel, coordinate, and data trait contracts so exact predicates are dimension-scoped and payload bounds apply only where needed. - Split explicit-construction validation into orthogonal typed errors and preserve structured source details through fallback and repair paths. - Narrow focused prelude exports, add repository style rules, and document the 10,000-vertex 3D acceptance envelope. BREAKING CHANGE: ExactPredicates now takes a const dimension and inherits Kernel<D>; Kernel no longer requires Default; several public error variants, payloads, prelude exports, and trait bounds are renamed, retyped, or tightened.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
Sorry — I can’t produce the required hidden review stack artifact because it must include every provided rangeId exactly once and the PR contains too many ranges for me to reliably assemble here. If you want, I can:
Which would you prefer? ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
🟢 Coverage 86.23% diff coverage · +0.65% coverage variation
Metric Results Coverage variation ✅ +0.65% coverage variation (-1.00%) Diff coverage ✅ 86.23% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (efc2bb7) 57156 51119 89.44% Head commit (dc83c83) 58933 (+1777) 53091 (+1972) 90.09% (+0.65%) 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 (#375) 2890 2492 86.23% 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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)
290-370:⚠️ Potential issue | 🟠 MajorMirror
NeighborValidationErrortype onTdsValidationFailure::InvalidNeighborsinstead of collapsing toString.
TdsError::InvalidNeighborscarries a typedreason: NeighborValidationErrorenum with 16+ discriminated variants (e.g.,LengthMismatch,NonPeriodicSelfNeighbor,MissingNeighborCell,SharedVertexCountMismatch,MirrorFacetMissing,BackReferenceMismatch). TheFromimpl at lines 297–299 converts this viareason.to_string(), collapsing the type toStringand preventing downstream pattern matching on the specific failure variant.Compare other variants in the same impl:
InvalidVertexandInvalidCellpreserveVertexValidationErrorandCellValidationErrorvia#[source];OrientationViolationpreserves all structured fields;GeometricandFacetwrap their source types. CollapsingNeighborValidationErrortoStringcontradicts both the stated PR goal (preserve structured source details) and the pattern established by neighboring error variants, preventing retryability classification and error diagnostics.Change
TdsValidationFailure::InvalidNeighbors { message: String }toTdsValidationFailure::InvalidNeighbors { reason: NeighborValidationError }and update theFromimpl to passreasonthrough unchanged.🤖 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 290 - 370, The InvalidNeighbors branch in the From<TdsError> for TdsValidationFailure is collapsing NeighborValidationError into a String; update the TdsValidationFailure enum so InvalidNeighbors stores reason: NeighborValidationError (not message: String), and modify the From implementation's TdsError::InvalidNeighbors arm to forward the original reason through unchanged (preserve type NeighborValidationError) instead of calling reason.to_string(); update any pattern matches/constructors that expect message to use the new reason field.src/core/cell.rs (1)
639-651:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn a typed error here instead of panicking on offset-length mismatches.
set_periodic_vertex_offsetsis enforcing a real runtime invariant on mutable library state, butassert_eq!turns bad input into a panic. This should fail explicitly so builders/repair paths can surface the problem without aborting the process.Suggested direction
- pub(crate) fn set_periodic_vertex_offsets( + pub(crate) fn set_periodic_vertex_offsets( &mut self, offsets: impl Into<PeriodicOffsetBuffer<D>>, - ) { + ) -> Result<(), CellValidationError> { let offsets = offsets.into(); - assert_eq!( - offsets.len(), - self.vertices.len(), - "set_periodic_vertex_offsets: offsets.len() ({}) must match self.vertices.len() ({}); refusing to update self.periodic_vertex_offsets", - offsets.len(), - self.vertices.len(), - ); + if offsets.len() != self.vertices.len() { + return Err(CellValidationError::InvalidPeriodicOffsetsLength { + actual: offsets.len(), + expected: self.vertices.len(), + dimension: D, + }); + } self.periodic_vertex_offsets = Some(offsets); + Ok(()) }As per coding guidelines, "Every mutating operation must preserve invariants checked by Tds::is_valid (Levels 1-3) and DelaunayTriangulation::is_valid (Level 4); operations that cannot preserve them must fail explicitly rather than leave the triangulation inconsistent" and "Use Result<_, Error> for every fallible operation; panics are reserved for documented, debug-only precondition violations; library code in src/ must not panic on user input".
🤖 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/cell.rs` around lines 639 - 651, Replace the panic-causing assert_eq! in set_periodic_vertex_offsets with a fallible API: change fn set_periodic_vertex_offsets(&mut self, offsets: impl Into<PeriodicOffsetBuffer<D>>) to return Result<(), ErrorType> (create or use the crate's Error enum, e.g., Error::PeriodicOffsetLengthMismatch) and validate offsets.len() against self.vertices.len(); on mismatch return Err(Error::PeriodicOffsetLengthMismatch { expected: self.vertices.len(), found: offsets.len() }) and on success set self.periodic_vertex_offsets = Some(offsets); update all call sites to propagate or handle the Result and preserve invariants checked by Tds::is_valid/DelaunayTriangulation::is_valid.src/geometry/algorithms/convex_hull.rs (1)
1051-1061:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDifferent-TDS provenance is being collapsed into
StaleHull.With the new identity check, a hull built from
dt1and used withdt2can now returnErr(ConvexHullConstructionError::StaleHull { hull_generation: g, tds_generation: g })even when neither TDS was mutated. That makes the current payload/message factually wrong and drops the new provenance detail this refactor is introducing. Please surface identity mismatch distinctly here, and mirror that in the validation path as well.Also applies to: 1125-1130
🤖 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/algorithms/convex_hull.rs` around lines 1051 - 1061, The check that compares creation_generation and creation_identity is collapsing an identity mismatch into ConvexHullConstructionError::StaleHull; change the logic so an identity mismatch is reported distinctly (e.g., return a new ConvexHullConstructionError variant like IdentityMismatch { hull_identity, tds_identity } or use an existing distinct variant) instead of StaleHull in the block around creation_generation/creation_identity (the code using creation_generation.get(), creation_identity.get(), tds.generation(), and Arc::ptr_eq). Make the same change in the validation path later (the analogous block around lines ~1125-1130) so provenance mismatches are surfaced separately from stale-generation errors and include both hull and tds identity info in the error payload.src/triangulation/builder.rs (1)
2016-2037:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn a construction error here instead of panicking on scalar conversion.
build_periodicis on the genericT: CoordinateScalarpath, butto_f64().expect(...)will abort for any caller whose scalar cannot be converted tof64. That turns ordinary user input on a public API into a panic insidesrc/. Either constrain the periodic path to supported scalar types or map this failure intoDelaunayTriangulationConstructionError.Minimal direction
- let canonical_f64: Vec<[f64; D]> = canonical_vertices + let canonical_f64: Vec<[f64; D]> = canonical_vertices .iter() .enumerate() - .map(|(canon_idx, v)| { + .map(|(canon_idx, v)| -> Result<[f64; D], DelaunayTriangulationConstructionError> { let orig_coords = v.point().coords(); let mut coords = [0_f64; D]; for i in 0..D { let domain_i = domain[i]; - let orig = orig_coords[i] - .to_f64() - .expect("canonical coordinate is finite and convertible"); + let orig = orig_coords[i].to_f64().ok_or_else(|| { + TriangulationConstructionError::GeometricDegeneracy { + message: format!( + "Periodic image-point construction requires coordinates convertible to f64 (vertex {canon_idx}, axis {i})", + ), + } + .into() + })?; let normalized = (orig / domain_i).clamp(0.0, 1.0 - f64::EPSILON); // ... } - coords + Ok(coords) }) - .collect(); + .collect::<Result<_, _>>()?;As per coding guidelines, "library code in src/ must not panic on user input".
🧹 Nitpick comments (4)
src/core/algorithms/incremental_insertion.rs (2)
608-624: ⚡ Quick win
PartialEqon summary types compares fullDisplaystrings.Both
DelaunayRepairErrorSummaryandInsertionErrorSummaryderivePartialEqover amessage: Stringpopulated viasource.to_string(). Equality therefore depends on exactDisplayformatting of every wrapped error variant. Tests liketest_delaunay_repair_error_summary_covers_all_kindsandtest_insertion_error_summary_preserves_nested_source_kindalready assert onsummary.message == source.to_string(), so futureDisplaytweaks ofTriangulationValidationError,DelaunayRepairError, etc. will silently break callers that match summaries for equality.Consider implementing
PartialEqmanually based onkind(+source_kindforInsertionErrorSummary) and excludingmessage, or document this brittleness explicitly. The current design also makesEqsemantically meaningful only whenDisplayis stable — worth being explicit about.Also applies to: 709-761
🤖 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 608 - 624, DelaunayRepairErrorSummary (and similarly InsertionErrorSummary) derives PartialEq/Eq which compares the message String produced by source.to_string(), making equality brittle to Display changes; remove the derived PartialEq/Eq and implement PartialEq (and Eq if needed) manually to compare only the structured discriminants (use DelaunayRepairErrorSummary.kind for DelaunayRepairErrorSummary, and for InsertionErrorSummary compare kind and source_kind) and ignore the message field, or alternatively keep derives but add an explicit doc comment explaining that equality depends on Display stability—update the impl From<&DelaunayRepairError> (and the equivalent InsertionErrorSummary From impl) only to populate message but not rely on it for equality.
763-796: 💤 Low value
Box<DelaunayRepairErrorSummary>inLocalRepairRobustFallbackis likely unnecessary.
DelaunayRepairErrorSummaryis small (aDelaunayRepairErrorKinddiscriminant plus aString), so the heap indirection adds an allocation and a deref without meaningfully shrinkingDelaunayRepairFailureContext. The other variants in this enum are unit-sized, so the enum's max size is already governed by this variant either way. Storing the summary by value keeps the cold-path API allocation-free and aligns with the surrounding "compact, by-value summary" goal.If the boxing is intentional for a forward-looking reason (e.g. anticipated growth of the summary), a short comment would help; otherwise consider unboxing.
🤖 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 763 - 796, The LocalRepairRobustFallback variant currently stores initial: Box<DelaunayRepairErrorSummary>, which introduces an unnecessary heap allocation; change the variant to store initial: DelaunayRepairErrorSummary by value in the DelaunayRepairFailureContext enum and update all uses accordingly (notably the fmt::Display impl match arm for Self::LocalRepairRobustFallback { initial } should call initial.fmt(f)? directly without dereferencing a Box). Ensure any code constructing this variant passes the value (not a Box) and remove any boxing sites; if boxing was intentional, add a brief comment explaining the rationale instead.src/geometry/algorithms/convex_hull.rs (1)
372-404: ⚡ Quick winMark
ConvexHullas#[must_use].This public wrapper type is in the changed surface and still lacks
#[must_use], so callers can silently drop a freshly built hull even though its methods already opt into that signal.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/algorithms/convex_hull.rs` around lines 372 - 404, The ConvexHull public wrapper type should be annotated with #[must_use]; add the #[must_use] attribute immediately above the pub struct ConvexHull<K, U, V, const D: usize> declaration so the compiler warns when a freshly built hull is dropped without use (no other code changes required; modify the ConvexHull struct declaration in this file).tests/proptest_tds.rs (1)
578-588: ⚖️ Poor tradeoffRejection threshold may be too strict for high-dimensional smoke tests.
The assertion
stats.rejected_construction_failed <= max_allowed_construction_rejectionsuses a fixed bound (target_cases.max(1), which equals 6) regardless of how many inputs proptest actually generated. If construction repeatedly fails in 4D/5D (common with random points near degeneracy), proptest will generate many more than 6 inputs to satisfy thecases: 6requirement. For example, if proptest generates 100 inputs and 95 fail construction but 6 eventually succeed, the test will fail because95 > 6, even though proptest's built-in rejection mechanism already handled the situation.Consider either:
- Removing the absolute bound and relying on proptest's rejection tracking, or
- Making the bound relative to
stats.generated(e.g.,rejected_construction_failed <= generated * 9 / 10) to allow a high but bounded rejection rate.🤖 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 `@tests/proptest_tds.rs` around lines 578 - 588, The current assertion uses a fixed max_allowed_construction_rejections derived from target_cases (via target_cases.max(1)) which is too strict when proptest generates many inputs; change the check on stats.rejected_construction_failed to be relative to how many inputs were actually generated (stats.generated) or remove the absolute bound: for example compute let max_allowed_construction_rejections = (stats.generated.max(1) * 9) / 10 to allow up to a 90% rejection rate, then assert stats.rejected_construction_failed <= max_allowed_construction_rejections (keeping the existing assert message), or simply delete the assert if you prefer to rely entirely on proptest's rejection tracking; update the code around max_allowed_construction_rejections, stats.rejected_construction_failed, target_cases and stats.generated accordingly.
🤖 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 608-624: Add the #[must_use] attribute to the public summary
wrapper structs so callers cannot ignore them: annotate
DelaunayRepairErrorSummary and the analogous InsertionErrorSummary with
#[must_use] (placed above the struct/derive) so the compiler warns when the
returned summaries are discarded; update both definitions (e.g., the impl
From<&DelaunayRepairError> remains unchanged) to ensure the attribute applies to
the public types.
In `@src/core/cell.rs`:
- Around line 165-171: The current comparator uses
left.partial_cmp(right).map_or_else(...) and returns cmp::Ordering::Equal when
partial_cmp is None, which makes CoordinateScalar ordering non-total and breaks
eq_by_vertices()/sort_by; instead implement a deterministic total order for the
incomparable case (e.g. treat NaN as greater/less consistently or compare
bit-patterns) or explicitly propagate an error/panic rather than collapsing to
Equal; update the comparator used in eq_by_vertices and any other locations
(also at the similar code around lines referenced 1455-1456) to call a helper
(e.g., total_cmp_for_coordinate) that converts Option<Ordering> into a stable
Ordering or returns Err/panic so sorting remains stable and total.
In `@src/core/tds.rs`:
- Around line 1248-1257: The public wrapper struct InvariantErrorSummary should
be annotated with #[must_use] so callers cannot accidentally discard validation
summaries; update the struct declaration for InvariantErrorSummary by adding the
#[must_use] attribute above its derive attributes (matching the pattern used for
TdsMutationError) while keeping existing derives and doc comments intact.
In `@src/core/vertex.rs`:
- Around line 13-15: The docs for Vertex and VertexBuilder still state "U:
DataType" which is misleading after the refactor; update the documentation
comments on the Vertex and VertexBuilder types to explain that generic U no
longer must implement DataType for general use, but that serialization requires
DataSerialize/DataDeserialize (or the crate's equivalent) when
serializing/deserializing; reference the symbols Vertex and VertexBuilder and
replace or augment the lines that assert "U: DataType" with wording that
clarifies the looser runtime bounds and the separate serde/data trait
requirements for serialization.
In `@src/triangulation/builder.rs`:
- Around line 357-364: Add the #[must_use] attribute to the four public wrapper
error structs so callers cannot accidentally drop them: annotate
ExplicitTdsError, ExplicitInsertionError, ExplicitInvariantError, and
ExplicitDelaunayValidationError with #[must_use]; update the struct declarations
(the derive lines and/or immediately above each pub struct) to include
#[must_use] while keeping the existing derives and attributes intact.
In `@src/triangulation/delaunay.rs`:
- Around line 7348-7353: The code currently wraps the inverse k=1 wiring failure
into NeighborValidationError::Other with a formatted string, which discards the
original NeighborWiringError type; modify the remove_vertex path that constructs
NeighborValidationError to preserve the typed failure instead of stringifying
it: if NeighborWiringError already implements From<NeighborWiringError> for
NeighborValidationError or Into conversion exists, return/propagate that
conversion directly (e.g., return Err(NeighborWiringError.into())); otherwise
add a dedicated variant on NeighborValidationError (e.g.,
NeighborValidationError::NeighborWiring(NeighborWiringError)) and return that
variant so callers can distinguish the specific neighbor wiring failure during
the inverse k=1 flip.
---
Outside diff comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 290-370: The InvalidNeighbors branch in the From<TdsError> for
TdsValidationFailure is collapsing NeighborValidationError into a String; update
the TdsValidationFailure enum so InvalidNeighbors stores reason:
NeighborValidationError (not message: String), and modify the From
implementation's TdsError::InvalidNeighbors arm to forward the original reason
through unchanged (preserve type NeighborValidationError) instead of calling
reason.to_string(); update any pattern matches/constructors that expect message
to use the new reason field.
In `@src/core/cell.rs`:
- Around line 639-651: Replace the panic-causing assert_eq! in
set_periodic_vertex_offsets with a fallible API: change fn
set_periodic_vertex_offsets(&mut self, offsets: impl
Into<PeriodicOffsetBuffer<D>>) to return Result<(), ErrorType> (create or use
the crate's Error enum, e.g., Error::PeriodicOffsetLengthMismatch) and validate
offsets.len() against self.vertices.len(); on mismatch return
Err(Error::PeriodicOffsetLengthMismatch { expected: self.vertices.len(), found:
offsets.len() }) and on success set self.periodic_vertex_offsets =
Some(offsets); update all call sites to propagate or handle the Result and
preserve invariants checked by Tds::is_valid/DelaunayTriangulation::is_valid.
In `@src/geometry/algorithms/convex_hull.rs`:
- Around line 1051-1061: The check that compares creation_generation and
creation_identity is collapsing an identity mismatch into
ConvexHullConstructionError::StaleHull; change the logic so an identity mismatch
is reported distinctly (e.g., return a new ConvexHullConstructionError variant
like IdentityMismatch { hull_identity, tds_identity } or use an existing
distinct variant) instead of StaleHull in the block around
creation_generation/creation_identity (the code using creation_generation.get(),
creation_identity.get(), tds.generation(), and Arc::ptr_eq). Make the same
change in the validation path later (the analogous block around lines
~1125-1130) so provenance mismatches are surfaced separately from
stale-generation errors and include both hull and tds identity info in the error
payload.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 608-624: DelaunayRepairErrorSummary (and similarly
InsertionErrorSummary) derives PartialEq/Eq which compares the message String
produced by source.to_string(), making equality brittle to Display changes;
remove the derived PartialEq/Eq and implement PartialEq (and Eq if needed)
manually to compare only the structured discriminants (use
DelaunayRepairErrorSummary.kind for DelaunayRepairErrorSummary, and for
InsertionErrorSummary compare kind and source_kind) and ignore the message
field, or alternatively keep derives but add an explicit doc comment explaining
that equality depends on Display stability—update the impl
From<&DelaunayRepairError> (and the equivalent InsertionErrorSummary From impl)
only to populate message but not rely on it for equality.
- Around line 763-796: The LocalRepairRobustFallback variant currently stores
initial: Box<DelaunayRepairErrorSummary>, which introduces an unnecessary heap
allocation; change the variant to store initial: DelaunayRepairErrorSummary by
value in the DelaunayRepairFailureContext enum and update all uses accordingly
(notably the fmt::Display impl match arm for Self::LocalRepairRobustFallback {
initial } should call initial.fmt(f)? directly without dereferencing a Box).
Ensure any code constructing this variant passes the value (not a Box) and
remove any boxing sites; if boxing was intentional, add a brief comment
explaining the rationale instead.
In `@src/geometry/algorithms/convex_hull.rs`:
- Around line 372-404: The ConvexHull public wrapper type should be annotated
with #[must_use]; add the #[must_use] attribute immediately above the pub struct
ConvexHull<K, U, V, const D: usize> declaration so the compiler warns when a
freshly built hull is dropped without use (no other code changes required;
modify the ConvexHull struct declaration in this file).
In `@tests/proptest_tds.rs`:
- Around line 578-588: The current assertion uses a fixed
max_allowed_construction_rejections derived from target_cases (via
target_cases.max(1)) which is too strict when proptest generates many inputs;
change the check on stats.rejected_construction_failed to be relative to how
many inputs were actually generated (stats.generated) or remove the absolute
bound: for example compute let max_allowed_construction_rejections =
(stats.generated.max(1) * 9) / 10 to allow up to a 90% rejection rate, then
assert stats.rejected_construction_failed <= max_allowed_construction_rejections
(keeping the existing assert message), or simply delete the assert if you prefer
to rely entirely on proptest's rejection tracking; update the code around
max_allowed_construction_rejections, stats.rejected_construction_failed,
target_cases and stats.generated accordingly.
🪄 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: 02108ddb-2f9f-4bfd-ae36-1c657f3d4f5f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.github/workflows/ci.yml.github/workflows/codecov.ymlCargo.tomlREADME.mdREFERENCES.mdbenches/README.mdbenches/large_scale_performance.rsbenches/profiling_suite.rsdocs/dev/tooling-alignment.mddocs/limitations.mddocs/roadmap.mddocs/validation.mddocs/workflows.mdrust-toolchain.tomlsemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/cell.rssrc/core/collections/buffers.rssrc/core/facet.rssrc/core/tds.rssrc/core/traits/data_type.rssrc/core/triangulation.rssrc/core/util/deduplication.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/jaccard.rssrc/core/vertex.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/predicates.rssrc/geometry/quality.rssrc/geometry/robust_predicates.rssrc/geometry/traits/coordinate.rssrc/geometry/util/circumsphere.rssrc/geometry/util/measures.rssrc/geometry/util/point_generation.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/traits/topological_space.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rstests/README.mdtests/delaunay_repair_fallback.rstests/euler_characteristic.rstests/large_scale_debug.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/proptest_tds.rstests/proptest_triangulation.rstests/semgrep/src/project_rules/rust_style.rstests/triangulation_builder.rs
| let cell_uuid = { | ||
| let cell = self | ||
| .cell_mut(cell_key) | ||
| .ok_or_else(|| TdsError::CellNotFound { | ||
| cell_key, | ||
| context: "set_neighbors_by_key".to_string(), | ||
| })?; | ||
| let cell_uuid = cell.uuid(); | ||
| Self::set_cell_neighbors_normalized(cell, neighbors); | ||
| cell_uuid | ||
| }; | ||
|
|
||
| // Phase 3A: Store neighbor keys directly in SmallBuffer | ||
| // Normalize: if all neighbors are None, set cell.neighbors to None | ||
| if neighbors_vec.iter().all(Option::is_none) { | ||
| cell.neighbors = None; | ||
| } else { | ||
| let mut neighbor_buffer = SmallBuffer::new(); | ||
| neighbor_buffer.extend(neighbors_vec.iter().copied()); | ||
| cell.neighbors = Some(neighbor_buffer); | ||
| for (neighbor_key, mirror_idx, back_reference) in reciprocal_updates { | ||
| let neighbor_cell = | ||
| self.cells | ||
| .get_mut(neighbor_key) | ||
| .ok_or_else(|| TdsError::InvalidNeighbors { | ||
| reason: NeighborValidationError::MissingNeighborCell { | ||
| cell_key, | ||
| cell_uuid, | ||
| facet_index: mirror_idx, | ||
| neighbor_key, | ||
| context: "applying reciprocal neighbor update".to_string(), | ||
| }, | ||
| })?; | ||
| Self::set_neighbor_slot(neighbor_cell, mirror_idx, back_reference)?; | ||
| } |
There was a problem hiding this comment.
Don't commit the source neighbor buffer before reciprocal writes are guaranteed to succeed.
set_neighbors_by_key() updates cell_key first, then applies reciprocal writes. If one of those later writes fails (for example, an already-corrupted reciprocal neighbor buffer trips LengthMismatch inside set_neighbor_slot()), the method returns Err with a one-sided adjacency already committed. This leaves the TDS less consistent after a failed mutation than before the call.
One way to harden this
let reciprocal_updates =
self.reciprocal_neighbor_updates_for_neighbor_update(cell_key, neighbors)?;
+ self.validate_reciprocal_update_targets(&reciprocal_updates)?;
let cell_uuid = {
let cell = self
.cell_mut(cell_key)
.ok_or_else(|| TdsError::CellNotFound {The preflight should validate any existing reciprocal buffers and mirror indices without mutating them, so the source write only happens once every reciprocal target is known-good.
As per coding guidelines, every mutating operation must preserve invariants checked by Tds::is_valid and fail explicitly rather than leave the triangulation inconsistent.
- Preserve typed neighbor and flip-wiring failures through TDS and Delaunay validation instead of collapsing them into display strings. - Report convex hull TDS identity mismatches distinctly from stale-generation hulls. - Make periodic cell-offset updates fallible and use deterministic coordinate ordering for cell and Delaunay vertex comparisons. - Mark public validation summaries and convex hull wrappers as must-use, and compare repair/insertion summaries by structured kind. - Force Codecov coverage tooling reinstall when the pinned cargo-llvm-cov version changes. BREAKING CHANGE: DelaunayTriangulationValidationError variants now store boxed typed sources, and DelaunayRepairFailureContext::LocalRepairRobustFallback stores its summary by value. Callers matching or constructing those variants must update accordingly.
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)
src/core/algorithms/incremental_insertion.rs (1)
1483-1490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve perturbation retryability for summarized neighbor-rebuild failures.
NeighborRebuildError::Unexpectednow carriesInsertionErrorSummary, but this branch still hard-codes it tofalse. That makes rolled-back perturbation retries stop on geometry-sensitive failures once they pass through neighbor rebuild — e.g.TopologyValidationFailed(IsolatedVertex)orTopologyValidation(Geometric/OrientationViolation)become non-retryable even thoughsource_kindis preserved specifically to distinguish them.♻️ Suggested fix
CavityFillingError::NeighborRebuild { reason } => match reason { NeighborRebuildError::NonManifoldTopology { .. } => true, NeighborRebuildError::TopologyValidation { reason } => { Self::is_tds_validation_failure_retryable(reason) } - NeighborRebuildError::Wiring { .. } | NeighborRebuildError::Unexpected { .. } => { - false - } + NeighborRebuildError::Wiring { .. } => false, + NeighborRebuildError::Unexpected { source } => match source.source_kind { + Some(InsertionErrorSourceKind::Tds( + TdsErrorKind::Geometric | TdsErrorKind::OrientationViolation, + )) + | Some(InsertionErrorSourceKind::Triangulation( + TriangulationValidationErrorKind::ManifoldFacetMultiplicity + | TriangulationValidationErrorKind::BoundaryRidgeMultiplicity + | TriangulationValidationErrorKind::RidgeLinkNotManifold + | TriangulationValidationErrorKind::VertexLinkNotManifold + | TriangulationValidationErrorKind::IsolatedVertex, + )) => true, + _ => false, + }, },Based on learnings: perturbation retry (
is_retryable()) runs after rollback, so geometrically-sensitive errors — includingIsolatedVertex— should remain retryable there.🤖 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 1483 - 1490, The match arm for CavityFillingError::NeighborRebuild currently returns false for NeighborRebuildError::Unexpected, but Unexpected now carries an InsertionErrorSummary whose source_kind encodes whether the failure is geometry-sensitive and thus should remain retryable after rollback; update the NeighborRebuildError::Unexpected arm to destructure the summary (e.g., NeighborRebuildError::Unexpected(summary)) and return the summary's retryability (call summary.is_retryable() or inspect summary.source_kind and delegate to Self::is_tds_validation_failure_retryable(...) for TDS validation kinds) instead of hard-coding false so perturbation retries are preserved for geometry-sensitive failures.
🤖 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 `@src/core/algorithms/incremental_insertion.rs`:
- Around line 1483-1490: The match arm for CavityFillingError::NeighborRebuild
currently returns false for NeighborRebuildError::Unexpected, but Unexpected now
carries an InsertionErrorSummary whose source_kind encodes whether the failure
is geometry-sensitive and thus should remain retryable after rollback; update
the NeighborRebuildError::Unexpected arm to destructure the summary (e.g.,
NeighborRebuildError::Unexpected(summary)) and return the summary's retryability
(call summary.is_retryable() or inspect summary.source_kind and delegate to
Self::is_tds_validation_failure_retryable(...) for TDS validation kinds) instead
of hard-coding false so perturbation retries are preserved for
geometry-sensitive failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 30ddabba-abda-45da-b91b-2ded78db02a2
📒 Files selected for processing (16)
.github/workflows/codecov.ymlbenches/ci_performance_suite.rsbenches/profiling_suite.rsexamples/triangulation_3d_100_points.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/cell.rssrc/core/tds.rssrc/core/triangulation.rssrc/core/vertex.rssrc/geometry/algorithms/convex_hull.rssrc/topology/manifold.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rstests/prelude_exports.rstests/proptest_tds.rs
✅ Files skipped from review due to trivial changes (1)
- benches/ci_performance_suite.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/proptest_tds.rs
- benches/profiling_suite.rs
- tests/prelude_exports.rs
- src/core/vertex.rs
- src/geometry/algorithms/convex_hull.rs
- src/core/cell.rs
- src/core/algorithms/flips.rs
- src/core/tds.rs
- src/core/triangulation.rs
- src/triangulation/delaunay.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
+ Coverage 89.41% 90.06% +0.65%
==========================================
Files 61 61
Lines 56965 58742 +1777
==========================================
+ Hits 50935 52907 +1972
+ Misses 6030 5835 -195
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Remove unnecessary DataType and CoordinateScalar bounds from read-only topology, adjacency, quality, and conversion APIs. - Keep payload bounds on repair and mutation paths that need cloneable stored data. - Preserve typed validation, insertion, Delaunay, and explicit builder error discriminants through compact summaries and retry decisions. - Distinguish convex-hull TDS identity mismatches from stale-generation errors. - Surface facet-index overflow and quality lookup failures through structured errors.
coderabbitai
Bot
added
bug
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/tds.rs (1)
3656-3725:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake neighbor topology checks periodic-offset aware.
This still derives shared-facet membership from raw
VertexKeys. For lifted periodic cells, two valid neighbors can reuse the same key set with different offsets, which makesshared_countbecomeD + 1and rejects the adjacency asSharedVertexCountMismatch/OppositeVertexMismatch. The later mirror-facet cross-checks have the same blind spot. Use the lifted(VertexKey, offset)identity here instead of plain keys.🤖 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/tds.rs` around lines 3656 - 3725, The neighbor topology check currently compares raw VertexKey sets (cell_vertices / neighbor_vertices) which fails for lifted periodic cells; instead pair each vertex key with its periodic offset and compare the lifted identity (VertexKey, Offset). Replace the loop that computes shared_count and missing_vertex_idx so it iterates over lifted vertices for both the cell and neighbor (using whatever API supplies per-vertex offsets or a lifted_vertices() helper), count matches by (vkey, offset) equality, and compute the missing_vertex_idx from those lifted identities; update the ensuing checks that raise NeighborValidationError::SharedVertexCountMismatch and NeighborValidationError::OppositeVertexMismatch (and any later mirror-facet cross-checks) to use the lifted comparisons so periodic-offset-distinct vertices are not mistaken as duplicates.src/core/util/facet_keys.rs (1)
141-168: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
SmallBufferfor these per-simplex scratch buffers.Both
lifted_facetandpacked_signatureare bounded by simplex arity, so the newVecallocations are avoidable here. Switching them toSmallBufferkeeps this utility aligned with the repo’s stack-allocation rule and removes two heap allocations from the facet-key path.As per coding guidelines: "Per-simplex data must be stack-allocated ([T; D] coordinates, SmallBuffer<VertexKey, MAX_PRACTICAL_DIMENSION_SIZE>); triangulation topology is stored in DenseSlotMap (heap-backed by necessity, not accident)".
🤖 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/util/facet_keys.rs` around lines 141 - 168, Replace the per-simplex heap Vecs with stack-allocated SmallBuffer instances: change lifted_facet: Vec<(u64, [i8; D])> to SmallBuffer<(u64, [i8; D]), MAX_PRACTICAL_DIMENSION_SIZE> and packed_signature: Vec<u64> to SmallBuffer<u64, MAX_PRACTICAL_DIMENSION_SIZE * (D + 1)> (or equivalent compile-time bound), import SmallBuffer, and adjust code to use SmallBuffer::new()/push() instead of Vec::with_capacity/ push; keep the same sorting/iteration semantics (call .as_mut_slice() or .as_slice() to sort_unstable_by when needed) and preserve the existing error-return logic (PeriodicFacetKeyDerivationError and shifted validation) so behavior is unchanged except eliminating the two heap allocations for lifted_facet and packed_signature.src/core/algorithms/flips.rs (1)
824-879: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftThese local neighbor-validation failures are still stringly typed.
All of these newly-touched branches end up as
NeighborValidationError::Other { message }, so callers still have to parse strings to distinguish concrete cases like wrong neighbor arity, removed-neighbor references, non-periodic self-neighbors, missing neighbors, mirror-facet mismatch, mutual-pointer mismatch, and facet-order failures. That undercuts the typed-validation direction of this PR.Please give these cases dedicated discriminants, or introduce a flip-trial-specific typed reason enum and wrap that instead of formatting messages inline.
Also applies to: 947-955, 975-991
🤖 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/flips.rs` around lines 824 - 879, Replace the stringly-typed NeighborValidationError::Other branches in the flip-neighbor checks with concrete, typed variants: add new discriminants to NeighborValidationError (or create a FlipTrialNeighborReason enum) for each case (WrongArity, ReferencedRemovedNeighbor { cell: CellKey, neighbor: CellKey }, NonPeriodicSelfNeighbor { cell: Uuid, facet_idx: usize }, NeighborNotFound { neighbor: CellKey }, MirrorFacetMismatch { cell: Uuid, facet_idx: usize, neighbor: Uuid }, MutualPointerMismatch, FacetOrderMismatch, etc.) and return TdsValidationFailure::InvalidNeighbors with those variants instead of formatted messages; update all sites shown (the branches around neighbors.len() check, removed_cells check, self-neighbor check using cell_allows_periodic_self_neighbor, missing neighbor lookup tds.cell(...), and mirror_facet_index(...) failure) to use the new typed variants so callers can match on specific failures rather than parsing strings.
♻️ Duplicate comments (1)
src/core/tds.rs (1)
4089-4118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreflight reciprocal targets before committing the source write.
reciprocal_neighbor_updates_for_neighbor_update()proves existence and mirror indices, but it still doesn't prove that each reciprocal buffer can actually accept the write. If one target already has a malformed neighbor buffer,set_neighbor_slot()can fail afterset_cell_neighbors_normalized(cell, neighbors)has already committedcell_key, leaving a one-sided adjacency behind and skipping the generation bump on the error path.Possible hardening
self.validate_neighbor_topology(cell_key, neighbors)?; self.validate_neighbor_update_matches_facet_incidence(cell_key, neighbors)?; let reciprocal_updates = self.reciprocal_neighbor_updates_for_neighbor_update(cell_key, neighbors)?; + self.validate_reciprocal_update_targets(&reciprocal_updates)?; let cell_uuid = { let cell = self .cell_mut(cell_key) .ok_or_else(|| TdsError::CellNotFound {As per coding guidelines, every mutating operation must preserve invariants checked by
Tds::is_validand fail explicitly rather than leave the triangulation inconsistent.
🧹 Nitpick comments (2)
src/geometry/util/triangulation_generation.rs (1)
773-777: ⚡ Quick winReplace
println!test diagnostics with feature-gatedtracing::debug!.This new diagnostic path should follow repo diagnostics conventions (tracing + feature gate) rather than
println!.Suggested change
- if let Err(e) = &valid_different_seed { - println!( - "test_generate_random_triangulation_basic (second seeded 2D): TDS invalid: {e}" - ); - } + if let Err(e) = &valid_different_seed { + #[cfg(feature = "diagnostics")] + tracing::debug!( + error = %e, + "test_generate_random_triangulation_basic (second seeded 2D): TDS invalid" + ); + }As per coding guidelines, “Use tracing::{debug,info,warn,error}! for committed diagnostics across production code, tests, and benchmarks” and “Gate non-essential test/benchmark diagnostics behind feature flags.”
🤖 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 773 - 777, Replace the println! diagnostic in the triangulation validity check with a feature-gated tracing::debug! call: detect the block that evaluates triangulation_different_seed and the Err branch that currently prints (refer to triangulation_different_seed and valid_different_seed), change the println! to tracing::debug! and wrap that debug emission in a cfg(feature = "diagnostics") gate (or the repo's diagnostics feature name) so the tracing call is only compiled when diagnostics are enabled; ensure tracing::debug is available under that same cfg (e.g., import tracing::debug or qualify it inside the gated block) so the test diagnostics follow the repo convention.src/core/algorithms/incremental_insertion.rs (1)
2433-2443: ⚡ Quick winAssert the sorted-input contract in
facet_hash_from_sorted_vertices().This helper is now the single entry point for facet/ridge hashing, but a missed
sort_unstable()at any future call site will silently change facet keys and mis-wire neighbors. A debug-only sortedness assertion here would make that failure mode obvious during tests.🤖 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 2433 - 2443, Add a debug-only assertion to facet_hash_from_sorted_vertices() to ensure the input slice is sorted: inside the function (before hashing) use a debug_assert that every adjacent pair in sorted_vkeys is non-decreasing (e.g., windows(2).all(|w| w[0] <= w[1])) and include a short message like "facet_hash_from_sorted_vertices: input must be sorted" so test builds fail fast if a caller forgets sort_unstable().
🤖 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 717-835: The summary drops top-level retryability; add a boolean
field (e.g. retryable: bool) to InsertionErrorSummary, set it in the
From<InsertionError> implementation by delegating to source.is_retryable() (call
InsertionError::is_retryable on the original error), update
InsertionErrorSummary::is_retryable() to simply return that field (or return
self.retryable || existing discriminant logic), and update PartialEq/Eq behavior
if you need summaries with different retryability to compare as different
(include retryable in eq) so NeighborRebuildError::Unexpected and
CavityFillingError::NeighborRebuild that only hold summaries preserve the
original retryability.
In `@src/core/util/facet_keys.rs`:
- Around line 269-281: cell_facet_key currently accepts any vertices length as
long as omit_idx is in range and will hash malformed cells instead of failing;
update cell_facet_key to validate the cell has simplex arity (vertices.len() ==
D + 1) before building the facet key or route the call through
checked_facet_key_from_vertex_keys::<D>() so malformed arity returns an Err
(propagating a facet index/arity error) instead of producing a facet via
facet_key_from_vertices; ensure verify_facet_index_consistency then receives
only checked/validated facet keys.
In `@src/topology/characteristics/validation.rs`:
- Around line 130-131: The doc comment in validation.rs contains a dangling
fragment: remove or merge the trailing phrase "if the underlying operations
fail." so the documentation reads as a single coherent sentence (e.g., keep
"Returns [`TopologyError`] if topology validation support data cannot be built."
and delete the stray fragment) for the function/module that currently has that
comment to avoid the non-sequitur.
---
Outside diff comments:
In `@src/core/algorithms/flips.rs`:
- Around line 824-879: Replace the stringly-typed NeighborValidationError::Other
branches in the flip-neighbor checks with concrete, typed variants: add new
discriminants to NeighborValidationError (or create a FlipTrialNeighborReason
enum) for each case (WrongArity, ReferencedRemovedNeighbor { cell: CellKey,
neighbor: CellKey }, NonPeriodicSelfNeighbor { cell: Uuid, facet_idx: usize },
NeighborNotFound { neighbor: CellKey }, MirrorFacetMismatch { cell: Uuid,
facet_idx: usize, neighbor: Uuid }, MutualPointerMismatch, FacetOrderMismatch,
etc.) and return TdsValidationFailure::InvalidNeighbors with those variants
instead of formatted messages; update all sites shown (the branches around
neighbors.len() check, removed_cells check, self-neighbor check using
cell_allows_periodic_self_neighbor, missing neighbor lookup tds.cell(...), and
mirror_facet_index(...) failure) to use the new typed variants so callers can
match on specific failures rather than parsing strings.
In `@src/core/tds.rs`:
- Around line 3656-3725: The neighbor topology check currently compares raw
VertexKey sets (cell_vertices / neighbor_vertices) which fails for lifted
periodic cells; instead pair each vertex key with its periodic offset and
compare the lifted identity (VertexKey, Offset). Replace the loop that computes
shared_count and missing_vertex_idx so it iterates over lifted vertices for both
the cell and neighbor (using whatever API supplies per-vertex offsets or a
lifted_vertices() helper), count matches by (vkey, offset) equality, and compute
the missing_vertex_idx from those lifted identities; update the ensuing checks
that raise NeighborValidationError::SharedVertexCountMismatch and
NeighborValidationError::OppositeVertexMismatch (and any later mirror-facet
cross-checks) to use the lifted comparisons so periodic-offset-distinct vertices
are not mistaken as duplicates.
In `@src/core/util/facet_keys.rs`:
- Around line 141-168: Replace the per-simplex heap Vecs with stack-allocated
SmallBuffer instances: change lifted_facet: Vec<(u64, [i8; D])> to
SmallBuffer<(u64, [i8; D]), MAX_PRACTICAL_DIMENSION_SIZE> and packed_signature:
Vec<u64> to SmallBuffer<u64, MAX_PRACTICAL_DIMENSION_SIZE * (D + 1)> (or
equivalent compile-time bound), import SmallBuffer, and adjust code to use
SmallBuffer::new()/push() instead of Vec::with_capacity/ push; keep the same
sorting/iteration semantics (call .as_mut_slice() or .as_slice() to
sort_unstable_by when needed) and preserve the existing error-return logic
(PeriodicFacetKeyDerivationError and shifted validation) so behavior is
unchanged except eliminating the two heap allocations for lifted_facet and
packed_signature.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 2433-2443: Add a debug-only assertion to
facet_hash_from_sorted_vertices() to ensure the input slice is sorted: inside
the function (before hashing) use a debug_assert that every adjacent pair in
sorted_vkeys is non-decreasing (e.g., windows(2).all(|w| w[0] <= w[1])) and
include a short message like "facet_hash_from_sorted_vertices: input must be
sorted" so test builds fail fast if a caller forgets sort_unstable().
In `@src/geometry/util/triangulation_generation.rs`:
- Around line 773-777: Replace the println! diagnostic in the triangulation
validity check with a feature-gated tracing::debug! call: detect the block that
evaluates triangulation_different_seed and the Err branch that currently prints
(refer to triangulation_different_seed and valid_different_seed), change the
println! to tracing::debug! and wrap that debug emission in a cfg(feature =
"diagnostics") gate (or the repo's diagnostics feature name) so the tracing call
is only compiled when diagnostics are enabled; ensure tracing::debug is
available under that same cfg (e.g., import tracing::debug or qualify it inside
the gated block) so the test diagnostics follow the repo convention.
🪄 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: ef332ea9-89fa-4e86-8fed-5d3fa82d4233
📒 Files selected for processing (38)
src/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/boundary.rssrc/core/cell.rssrc/core/collections/spatial_hash_grid.rssrc/core/edge.rssrc/core/facet.rssrc/core/tds.rssrc/core/traits/boundary_analysis.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/facet_keys.rssrc/core/util/measurement.rssrc/core/vertex.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/point.rssrc/geometry/quality.rssrc/geometry/traits/coordinate.rssrc/geometry/util/measures.rssrc/geometry/util/triangulation_generation.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rstests/README.mdtests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/euler_characteristic.rstests/proptest_delaunay_triangulation.rstests/proptest_point.rstests/proptest_tds.rstests/proptest_vertex.rstests/public_topology_api.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (1)
- tests/delaunay_edge_cases.rs
✅ Files skipped from review due to trivial changes (8)
- tests/trait_bound_ergonomics.rs
- src/core/edge.rs
- tests/proptest_point.rs
- tests/public_topology_api.rs
- src/core/util/measurement.rs
- src/core/adjacency.rs
- tests/README.md
- tests/delaunay_incremental_insertion.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- src/core/algorithms/locate.rs
- src/core/boundary.rs
- src/geometry/traits/coordinate.rs
- tests/proptest_tds.rs
- tests/proptest_delaunay_triangulation.rs
- tests/euler_characteristic.rs
- src/core/facet.rs
- tests/triangulation_builder.rs
- src/geometry/quality.rs
- src/triangulation/builder.rs
- src/core/cell.rs
- src/core/triangulation.rs
- src/geometry/algorithms/convex_hull.rs
- src/triangulation/delaunay.rs
- Preserve top-level retryability in insertion summaries and compare summaries by structured fields instead of display text. - Surface flip-trial neighbor failures as typed NeighborValidationError variants rather than string payloads. - Validate facet-key simplex arity before hashing and compare periodic neighbor facets through lifted identities. BREAKING CHANGE: InsertionErrorSummary now includes a retryable field that direct struct literals must set, and flip-trial neighbor validation returns specific NeighborValidationError variants instead of Other strings for those failure modes.
1 participant
BREAKING CHANGE: ExactPredicates now takes a const dimension and inherits Kernel; Kernel no longer requires Default; several public error variants, payloads, prelude exports, and trait bounds are renamed, retyped, or tightened.