refactor!(tds): canonicalize incidence storage#473
Merged
Conversation
- Split the TDS into focused storage, mutation, validation, incidence, equality, key, error, and snapshot modules. - Make vertex incidence canonical storage and expose AdjacencyIndex as a lifetime-bound borrowed view. - Remove compact summary error exports in favor of typed construction, insertion, repair, and invariant errors. - Add bench-gated PL-manifold repair fixtures and a Criterion benchmark for over-shared facets with orphan cleanup. - Align uv-managed workflow/tooling pins with uv 0.11.23 and guard raw benchmark fixture builders with Semgrep. BREAKING CHANGE: AdjacencyIndex now carries a TDS lifetime and borrowed-view query signatures, compact summary error exports were removed from the root and focused preludes, and TDS internals are organized under src/core/tds/* instead of src/core/tds.rs. Closes #403
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR adds maintained incidence storage and split topology views, refactors construction and insertion errors into typed boxed variants, updates repair and flip paths to traverse maintained incidence, and adds PL-manifold benchmark support with matching docs and tooling. ChangesTDS modularization, typed errors, adjacency views, and benchmark plumbing
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 858 |
🟢 Coverage 89.03% diff coverage · -0.13% coverage variation
Metric Results Coverage variation ✅ -0.13% coverage variation (-1.00%) Diff coverage ✅ 89.03% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (1985e6f) 69013 63272 91.68% Head commit (82b3147) 69935 (+922) 64023 (+751) 91.55% (-0.13%) 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 (#473) 8417 7494 89.03% 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]](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 (3)
docs/code_organization.md (1)
450-468:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
tds/validation.rsfrom architecture inventory.Line 468 references Level 2 validation as residing in
tds/validation.rs, but the architecture overview (lines 450-457) does not include atds/validation.rsentry. The listed modules arestorage,errors,equality,keys,mutation, andsnapshot, butvalidationis missing from the documented inventory.Add
tds/validation.rsto the architecture listing with a brief description matching its role as the Level 2 structural and adjacency consistency validator.Proposed addition (after line 457)
- `tds/snapshot.rs` - TDS persistence boundary: raw codec records parse into a validated UUID snapshot before hydration allocates fresh slotmap keys, preserving vertex and simplex payload data without serializing storage-local handles + - `tds/validation.rs` - TDS Level 2 structural validation and adjacency consistency checksAnd verify that line 468 (the Level mapping comment) correctly reflects the modules documented above.
🤖 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 `@docs/code_organization.md` around lines 450 - 468, The documentation is missing the tds/validation.rs module from the architecture inventory at lines 450-457, even though it is referenced later at line 468 in the validation.rs section. Add a new entry for tds/validation.rs to the TDS modules list after the tds/snapshot.rs entry with a brief description indicating it handles Level 2 structural validation and adjacency consistency checks. Then verify that the Level mapping comment at line 468 correctly reflects all documented modules, ensuring consistency between the architecture overview and the validation orchestration description.src/delaunay/construction.rs (1)
5128-5145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClassify internal typed failures as non-retryable.
OrientationCanonicalizationInternalandInsertionNeighborWiringrepresent deterministic structural failures, but they currently fall through as retryable. With the default shuffled policy, these can be retried and finally reported asShuffledRetryExhausted, obscuring the actionable typed source.Proposed fix
DelaunayConstructionFailure::Tds { reason: TdsConstructionFailure::DuplicateUuid { .. } | TdsConstructionFailure::Validation { .. }, } | DelaunayConstructionFailure::InternalInconsistency { .. } + | DelaunayConstructionFailure::InsertionNeighborWiring { .. } + | DelaunayConstructionFailure::OrientationCanonicalizationInternal { .. } | DelaunayConstructionFailure::UnsupportedPeriodicDimension { .. } | DelaunayConstructionFailure::SpatialIndexConstruction { .. } | DelaunayConstructionFailure::InsertionTopologyValidation { .. }🤖 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/delaunay/construction.rs` around lines 5128 - 5145, The is_non_retryable_construction_error function is missing two deterministic structural failure variants that should be classified as non-retryable. Add OrientationCanonicalizationInternal and InsertionNeighborWiring to the pattern match within the matches! macro by including two additional match arms (OrientationCanonicalizationInternal { .. } and InsertionNeighborWiring { .. }) as DelaunayConstructionFailure variants in the same list as the other non-retryable failure types, ensuring these failures are not retried and their typed source is preserved.src/core/algorithms/flips.rs (1)
430-432:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface stale incidence instead of skipping it.
simplex_keys_containing_vertexnow reads the canonical incidence index, so an unresolved simplex key is an invariant violation. The currentcontinuecan hide a corrupted incidence index and allow the flip to commit after only local cavity validation.Propagate stale-incidence failures
if k_move >= 2 && k_move < D && let Some(existing_simplex) = - find_simplex_containing_simplex(tds, inserted_face_vertices, removed_simplices) + find_simplex_containing_simplex(tds, inserted_face_vertices, removed_simplices)? {fn find_simplex_containing_simplex<U, V, const D: usize>( tds: &Tds<U, V, D>, simplex_vertices: &[VertexKey], removed_simplices: &[SimplexKey], -) -> Option<SimplexKey> +) -> Result<Option<SimplexKey>, FlipError> where U: DataType, V: DataType, { - let first = *simplex_vertices.first()?; + let Some(&first) = simplex_vertices.first() else { + return Ok(None); + }; for simplex_key in tds.simplex_keys_containing_vertex(first) { if removed_simplices.contains(&simplex_key) { continue; } - let Some(simplex) = tds.simplex(simplex_key) else { - continue; - }; + let simplex = tds + .simplex(simplex_key) + .ok_or(FlipError::MissingSimplex { simplex_key })?; if simplex_vertices .iter() .copied() .all(|vk| simplex.contains_vertex(vk)) { - return Some(simplex_key); + return Ok(Some(simplex_key)); } } - None + Ok(None) }As per coding guidelines, “Every mutating operation MUST preserve invariants checked by Tds::is_valid (Level 1–3) and DelaunayTriangulation::is_valid (Level 4); operations that cannot preserve them must fail explicitly.”
Also applies to: 1150-1157
🤖 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 430 - 432, The current code uses continue to skip when find_simplex_containing_simplex returns None, which can hide corrupted incidence index violations. Instead of continuing, explicitly surface the stale incidence error by returning an appropriate error result. This ensures the operation fails when simplex_keys_containing_vertex encounters an unresolved simplex key, preserving the invariant that all incidence indices must be valid. Apply the same fix to the similar pattern mentioned at lines 1150-1157.Source: Coding guidelines
🧹 Nitpick comments (1)
src/core/tds/equality.rs (1)
42-46: Consider usingtotal_cmpfor consistency with other coordinate comparisons in the codebaseThe current implementation is safe due to NaN rejection at construction time (documented at line 56–57), making the
Nonebranch unreachable. However, other coordinate comparisons elsewhere (Point::cmp, Simplex::cmp, construction ordering) usetotal_cmpwith explicit iteration. Adopting that pattern would improve consistency and align with established practice.The claim about sign loss is incorrect: IEEE 754 specifies that ±0.0 are equal, which is the correct behavior for equality comparison. The panic risk from malformed coordinates is also overstated, since construction validates all coordinates.
🤖 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/equality.rs` around lines 42 - 46, The compare_coords function currently uses partial_cmp with a fallback to CmpOrdering::Equal for the None case, but other coordinate comparisons in the codebase (such as Point::cmp and Simplex::cmp) use total_cmp instead. Refactor the compare_coords function to use total_cmp on the left and right coordinate arrays for consistency with the established pattern elsewhere in the codebase. This aligns with how construction ordering and other coordinate comparisons handle the same comparison logic.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 `@docs/numerical_robustness_guide.md`:
- Line 335: The markdown documentation file has an incorrect file path reference
on line 335. The link currently points to `src/core/tds/storage.rs` but the
function `validate_simplex_coordinate_uniqueness` is actually defined in
`src/core/tds/validation.rs`. Update the link in the documentation to reference
the correct file path `src/core/tds/validation.rs` instead of the storage
module.
In `@docs/ORIENTATION_SPEC.md`:
- Line 235: In the documentation at line 235 of docs/ORIENTATION_SPEC.md, remove
the references to storage.rs and triangulation.rs from the list of files
containing orientation unit tests. Add src/core/tds/validation.rs to the list
instead, as this is where the actual coherent orientation tests are located,
including test_facet_permutation_parity_derives_coherent_and_incoherent_cases()
and test_orientation_validation_* functions.
- Line 27: The documentation file contains incorrect module references for the
method `Tds::is_coherently_oriented`. The method is actually located in
`src/core/tds/validation.rs`, not in `src/core/tds/storage.rs` as currently
documented. Update all references to this method in the ORIENTATION_SPEC.md file
to point to the correct validation module instead of the storage module. There
are two locations that need correction where the link references
`src/core/tds/storage.rs` - replace these with `src/core/tds/validation.rs` to
ensure the documentation links to the actual implementation.
---
Outside diff comments:
In `@docs/code_organization.md`:
- Around line 450-468: The documentation is missing the tds/validation.rs module
from the architecture inventory at lines 450-457, even though it is referenced
later at line 468 in the validation.rs section. Add a new entry for
tds/validation.rs to the TDS modules list after the tds/snapshot.rs entry with a
brief description indicating it handles Level 2 structural validation and
adjacency consistency checks. Then verify that the Level mapping comment at line
468 correctly reflects all documented modules, ensuring consistency between the
architecture overview and the validation orchestration description.
In `@src/core/algorithms/flips.rs`:
- Around line 430-432: The current code uses continue to skip when
find_simplex_containing_simplex returns None, which can hide corrupted incidence
index violations. Instead of continuing, explicitly surface the stale incidence
error by returning an appropriate error result. This ensures the operation fails
when simplex_keys_containing_vertex encounters an unresolved simplex key,
preserving the invariant that all incidence indices must be valid. Apply the
same fix to the similar pattern mentioned at lines 1150-1157.
In `@src/delaunay/construction.rs`:
- Around line 5128-5145: The is_non_retryable_construction_error function is
missing two deterministic structural failure variants that should be classified
as non-retryable. Add OrientationCanonicalizationInternal and
InsertionNeighborWiring to the pattern match within the matches! macro by
including two additional match arms (OrientationCanonicalizationInternal { .. }
and InsertionNeighborWiring { .. }) as DelaunayConstructionFailure variants in
the same list as the other non-retryable failure types, ensuring these failures
are not retried and their typed source is preserved.
---
Nitpick comments:
In `@src/core/tds/equality.rs`:
- Around line 42-46: The compare_coords function currently uses partial_cmp with
a fallback to CmpOrdering::Equal for the None case, but other coordinate
comparisons in the codebase (such as Point::cmp and Simplex::cmp) use total_cmp
instead. Refactor the compare_coords function to use total_cmp on the left and
right coordinate arrays for consistency with the established pattern elsewhere
in the codebase. This aligns with how construction ordering and other coordinate
comparisons handle the same comparison logic.
🪄 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: e8ce8901-0bea-4fa0-896e-2ccbab233dc5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/generate-baseline.yml.github/workflows/release-benchmarks.yml.github/workflows/semgrep-sarif.ymlAGENTS.mdCargo.tomlbenches/README.mdbenches/pl_manifold_repair.rsdocs/ORIENTATION_SPEC.mddocs/code_organization.mddocs/dev/commands.mddocs/dev/rust.mddocs/dev/testing.mddocs/dev/tooling-alignment.mddocs/invariants.mddocs/numerical_robustness_guide.mddocs/production_review_remediation_checklist.mdjustfilepyproject.tomlsemgrep.yamlsrc/bench_fixtures.rssrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/construction.rssrc/core/insertion.rssrc/core/orientation.rssrc/core/query.rssrc/core/repair.rssrc/core/simplex.rssrc/core/tds.rssrc/core/tds/equality.rssrc/core/tds/errors.rssrc/core/tds/incidence.rssrc/core/tds/keys.rssrc/core/tds/mutation.rssrc/core/tds/snapshot.rssrc/core/tds/storage.rssrc/core/tds/validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/insertion.rssrc/delaunay/query.rssrc/lib.rssrc/topology/manifold.rstests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/large_scale_debug.rstests/prelude_exports.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
- Coverage 91.65% 91.52% -0.14%
==========================================
Files 72 77 +5
Lines 68792 69714 +922
==========================================
+ Hits 63053 63804 +751
- Misses 5739 5910 +171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Maintain vertex-to-simplices incidence as an explicit TDS index used by adjacency traversal, local repair, and stale-incidence detection. - Add lifetime-bound `TriangulationAdjacency` views for repeated topology queries while keeping detached adjacency indexes checked. - Surface dangling vertex incidence as typed flip and removal errors instead of silently skipping corrupted index entries. - Refresh TDS module docs, prelude exports, and allocation benchmarks for the split incidence, mutation, and validation layout. BREAKING CHANGE: `AdjacencyIndex` now borrows TDS incidence, and `*_with_index` query helpers require an index tied to the queried triangulation lifetime. Callers with repeated adjacency queries should use `adjacency()` / `TriangulationAdjacency` or rebuild an index from the same triangulation snapshot.
Contributor
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 (2)
src/delaunay/construction.rs (1)
3019-3021:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop retrying non-geometric Delaunay-validation failures.
candidate.is_delaunay_via_flips()errors are always stored asDelaunayValidation, but the non-retryable repair classification is only applied when the sameDelaunayRepairErroris wrapped byDelaunayRepair/InsertionDelaunayRepair. That means deterministic failures such as invalid topology or hard verification errors can still burn every shuffled retry and surface asShuffledRetryExhaustedinstead of the immediate typed failure. Mirroris_geometric_repair_errorin both retry paths before assigninglast_failure.Possible direction
+ fn map_delaunay_validation_retry_failure( + source: DelaunayRepairError, + ) -> Result<DelaunayConstructionRetryFailure, DelaunayTriangulationConstructionError> { + if is_geometric_repair_error(&source) { + Ok(DelaunayConstructionRetryFailure::DelaunayValidation { + source: Box::new(source), + }) + } else { + Err(DelaunayTriangulationConstructionError::Triangulation( + DelaunayConstructionFailure::DelaunayRepair { + phase: DelaunayConstructionRepairPhase::Completion, + source: Box::new(source), + }, + )) + } + }Then use it at each
candidate.is_delaunay_via_flips()error branch, wrapping the returned error withDelaunayTriangulationConstructionErrorWithStatisticsin the statistics variant.Also applies to: 3102-3107, 3236-3242, 3347-3354, 5155-5157
🤖 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/delaunay/construction.rs` around lines 3019 - 3021, The issue is that deterministic Delaunay-validation failures from candidate.is_delaunay_via_flips() are being retried unnecessarily, burning all shuffled retries before surfacing as ShuffledRetryExhausted instead of the actual typed failure. You need to create a helper function that mirrors the is_geometric_repair_error logic to classify whether a DelaunayRepairError is a geometric error or a non-retryable deterministic failure. Then apply this classification check at each candidate.is_delaunay_via_flips() error branch (appearing at lines around 3019, 3102, 3236, 3347, and 5155) before assigning to last_failure. When the error is non-geometric and non-retryable, wrap it with DelaunayTriangulationConstructionErrorWithStatistics in the statistics variant to ensure immediate failure instead of continued retry attempts.src/core/tds/equality.rs (1)
95-124:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCompare canonical vertex incidence before returning equality.
This equality path can return
trueafter matching vertices and simplex vertex sets, but it never checks the now-authoritative vertex→simplices incidence index. A TDS with stale or missing incidence entries can therefore compare equal to a valid TDS, masking corruption in the canonical representation. Normalize and compare each vertex’s incident simplex UUID set, or otherwise validate incidence consistency, before the finaltrue.Based on PR objectives, vertex incidence is now the canonical storage mechanism within the TDS.
🤖 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/equality.rs` around lines 95 - 124, After the simplex comparison loop in the triangulation equality check completes successfully (after iterating through all simplices with eq_by_vertices), add validation of the vertex incidence index before returning true. For each vertex in the triangulation, normalize and compare the incident simplex UUID sets between self and other to ensure the canonical vertex→simplices incidence representation is consistent between both TDS instances. If any vertex's incident simplex set differs or is missing, return false instead of allowing the equality check to proceed.
🧹 Nitpick comments (1)
src/delaunay/construction.rs (1)
5186-5196: ⚡ Quick winRemove or preserve the dropped completion-repair message.
map_completion_repair_errornow ignores_message, but both callers still allocate formatted context strings before calling it. Either remove the parameter and call-siteformat!work, or add typed context if the seeded/global failure detail is still needed.Minimal cleanup if the typed source is sufficient
- pub(crate) fn map_completion_repair_error( - _message: String, - repair_error: DelaunayRepairError, - ) -> DelaunayTriangulationConstructionError { + pub(crate) fn map_completion_repair_error( + repair_error: DelaunayRepairError, + ) -> DelaunayTriangulationConstructionError { DelaunayTriangulationConstructionError::Triangulation( DelaunayConstructionFailure::DelaunayRepair { phase: DelaunayConstructionRepairPhase::Completion, source: Box::new(repair_error), }, ) }Call sites can then pass only the repair error instead of constructing an unused message.
Also applies to: 4526-4550
🤖 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/delaunay/construction.rs` around lines 5186 - 5196, The `map_completion_repair_error` function has an unused `_message` parameter (indicated by the underscore prefix), yet callers are still allocating and formatting context strings before calling it, which is wasteful. Remove the `_message` parameter from the `map_completion_repair_error` function signature entirely, and update all call sites (at lines 4526-4550 and others) to stop constructing formatted messages and pass only the repair_error parameter to this function.
🤖 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/adjacency.rs`:
- Around line 186-190: Add the #[must_use] attribute to the
TriangulationAdjacency struct definition. Since this is a public wrapper type
that represents a built adjacency view, it should have the #[must_use] macro
attribute placed alongside the existing #[derive(Debug)] and #[non_exhaustive]
attributes to ensure instances are not accidentally dropped without being used.
In `@src/core/algorithms/flips.rs`:
- Around line 15705-15754: Replace the individual dimension-specific test
functions find_simplex_containing_simplex_rejects_stale_incidence_key,
build_k1_inverse_context_rejects_stale_incidence_before_multiplicity,
build_k2_inverse_context_rejects_stale_incidence_key, and
build_k3_inverse_context_rejects_stale_incidence_key with pastey macro-generated
parametrized tests that cover the feasible dimension ranges: D=2 through D=5 for
the first two functions, D=3 through D=5 for the build_k2_flip_context_from_edge
test, and D=4 through D=5 for the build_k3_flip_context_from_triangle test. Use
the pastey macro syntax to iterate over these dimension ranges and generate test
variants for each dimension, ensuring the generic parameter in Tds and related
types reflects the current dimension being tested.
- Around line 1164-1174: The order of validation checks in the loop iterating
over simplex_keys_containing_vertex is incorrect. Currently, the code checks if
simplex_key is in removed_simplices before validating that the simplex actually
exists via tds.simplex(simplex_key). This allows stale incidence entries that
are also in the removal set to be silently skipped instead of triggering
FlipError::DanglingVertexIncidence. Reorder the checks so that
tds.simplex(simplex_key) is called and validated first to ensure dangling vertex
incidences are always detected, and only then check if the simplex_key should be
skipped because it exists in removed_simplices.
In `@src/core/tds/equality.rs`:
- Around line 157-210: The three new test functions
test_tds_partial_eq_different_counts_not_equal,
test_tds_partial_eq_rejects_dangling_simplex_vertex_key, and
test_tds_partial_eq_rejects_misaligned_periodic_offsets are currently hardcoded
to dimension D=2. Refactor these tests to be dimension-generic by wrapping them
with pastey macros to generate parameterized versions that cover dimensions D=2
through D=5, following the project's coding guidelines for dimension-generic
testing.
---
Outside diff comments:
In `@src/core/tds/equality.rs`:
- Around line 95-124: After the simplex comparison loop in the triangulation
equality check completes successfully (after iterating through all simplices
with eq_by_vertices), add validation of the vertex incidence index before
returning true. For each vertex in the triangulation, normalize and compare the
incident simplex UUID sets between self and other to ensure the canonical
vertex→simplices incidence representation is consistent between both TDS
instances. If any vertex's incident simplex set differs or is missing, return
false instead of allowing the equality check to proceed.
In `@src/delaunay/construction.rs`:
- Around line 3019-3021: The issue is that deterministic Delaunay-validation
failures from candidate.is_delaunay_via_flips() are being retried unnecessarily,
burning all shuffled retries before surfacing as ShuffledRetryExhausted instead
of the actual typed failure. You need to create a helper function that mirrors
the is_geometric_repair_error logic to classify whether a DelaunayRepairError is
a geometric error or a non-retryable deterministic failure. Then apply this
classification check at each candidate.is_delaunay_via_flips() error branch
(appearing at lines around 3019, 3102, 3236, 3347, and 5155) before assigning to
last_failure. When the error is non-geometric and non-retryable, wrap it with
DelaunayTriangulationConstructionErrorWithStatistics in the statistics variant
to ensure immediate failure instead of continued retry attempts.
---
Nitpick comments:
In `@src/delaunay/construction.rs`:
- Around line 5186-5196: The `map_completion_repair_error` function has an
unused `_message` parameter (indicated by the underscore prefix), yet callers
are still allocating and formatting context strings before calling it, which is
wasteful. Remove the `_message` parameter from the `map_completion_repair_error`
function signature entirely, and update all call sites (at lines 4526-4550 and
others) to stop constructing formatted messages and pass only the repair_error
parameter to this function.
🪄 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: cb76ec31-f3b7-4e9a-9bf9-205d093cc4bb
📒 Files selected for processing (23)
benches/allocation_hot_paths.rsdocs/ORIENTATION_SPEC.mddocs/code_organization.mddocs/dev/rust.mddocs/invariants.mddocs/numerical_robustness_guide.mddocs/production_review_remediation_checklist.mdsrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/insertion.rssrc/core/query.rssrc/core/repair.rssrc/core/tds/equality.rssrc/core/tds/errors.rssrc/core/tds/incidence.rssrc/core/tds/mutation.rssrc/core/tds/storage.rssrc/delaunay/construction.rssrc/delaunay/flips.rssrc/delaunay/query.rssrc/lib.rstests/prelude_exports.rs
✅ Files skipped from review due to trivial changes (7)
- src/delaunay/flips.rs
- docs/invariants.md
- docs/production_review_remediation_checklist.md
- docs/ORIENTATION_SPEC.md
- docs/numerical_robustness_guide.md
- docs/code_organization.md
- docs/dev/rust.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/core/tds/incidence.rs
- tests/prelude_exports.rs
- src/lib.rs
- src/core/tds/errors.rs
- src/core/algorithms/incremental_insertion.rs
- src/core/query.rs
- Replace detached adjacency indexes with IncidenceView, EdgeIndex, SimplexNeighborIndex, and the composite TriangulationAdjacency view. - Tie query indexes to the triangulation lifetime and remove stale *_with_index query helpers in favor of direct split topology queries. - Canonicalize vertex incidence mutation with transactional removal, rollback reuse, typed stale-incidence errors, and incidence-aware equality. - Preserve deterministic construction and Delaunay validation failures as typed errors instead of retry-exhaustion wrappers. BREAKING CHANGE: `AdjacencyIndex`, `AdjacencyIndexBuildError`, `build_adjacency_index()`, and the `*_with_index` query helpers have been removed. Use `incidence()`, `build_edge_index()`, `build_simplex_neighbor_index()`, or `adjacency()` with `TopologyIndexBuildError` instead.
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 (1)
src/core/tds/equality.rs (1)
169-174:⚠️ Potential issue | 🟡 MinorDon't mark
TdsasEqwhilePartialEqcan be non-reflexive.The
eqmethod returnsfalsewhensimplex_uuid_sort_entriesreturnsNone(line 173). The test-only helperspush_first_simplex_vertex_key_storage_only_for_testandset_first_simplex_periodic_offsets_storage_only_for_testdeliberately create malformed TDS states with mismatched periodic offset counts, triggering this path. This violates theEqtrait's reflexivity contract (x == x must always be true), even though the malformed states are test-only constructions.Either remove the
Eqmarker impl or ensure the sort entry path cannot fail for any reachable state, including intentionally corrupted test values.🤖 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/equality.rs` around lines 169 - 174, The Tds type is marked with the Eq trait, but the PartialEq implementation in the eq method can violate reflexivity by returning false when simplex_uuid_sort_entries returns None. This occurs because test-only helpers like push_first_simplex_vertex_key_storage_only_for_test and set_first_simplex_periodic_offsets_storage_only_for_test create malformed TDS states. Fix this by either removing the Eq marker from the Tds impl block entirely, or alternatively by modifying the logic to ensure simplex_uuid_sort_entries cannot fail for any reachable state, including the test-corrupted values.
🤖 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/query.rs`:
- Around line 956-986: The test function
split_topology_indexes_cover_independent_query_families() currently only covers
dimension D=3, but the tested functions (incidence, build_edge_index,
build_simplex_neighbor_index) are const-generic across dimensions. Wrap this
test with pastey macros to generate dimension-generic test coverage for D=2
through D=5, following the same pattern used for accessor tests in the same
file. Apply the same treatment to the other split-index topology test cases
referenced in lines 1056-1098 and 1336-1428.
---
Outside diff comments:
In `@src/core/tds/equality.rs`:
- Around line 169-174: The Tds type is marked with the Eq trait, but the
PartialEq implementation in the eq method can violate reflexivity by returning
false when simplex_uuid_sort_entries returns None. This occurs because test-only
helpers like push_first_simplex_vertex_key_storage_only_for_test and
set_first_simplex_periodic_offsets_storage_only_for_test create malformed TDS
states. Fix this by either removing the Eq marker from the Tds impl block
entirely, or alternatively by modifying the logic to ensure
simplex_uuid_sort_entries cannot fail for any reachable state, including the
test-corrupted values.
🪄 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: 86c7ef49-7ea6-4ea9-af29-bec798754838
📒 Files selected for processing (19)
AGENTS.mddocs/code_organization.mddocs/dev/rust.mdexamples/triangulation_and_hull.rssrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/query.rssrc/core/tds/equality.rssrc/core/tds/errors.rssrc/core/tds/mutation.rssrc/core/tds/storage.rssrc/delaunay/construction.rssrc/delaunay/query.rssrc/lib.rstests/example_workflows.rstests/prelude_exports.rstests/public_topology_api.rstests/trait_bound_ergonomics.rs
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- docs/dev/rust.md
- docs/code_organization.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/prelude_exports.rs
- src/core/tds/errors.rs
- src/lib.rs
- src/delaunay/query.rs
- src/delaunay/construction.rs
- Remove the Eq marker from Tds because malformed internal states can make semantic PartialEq comparisons non-reflexive. - Expand split topology index coverage across D=2 through D=5 with a generic shared-facet fixture independent of Delaunay construction tie-breaking. BREAKING CHANGE: `Tds<U, V, D>` no longer implements `Eq`. Use `PartialEq` for semantic triangulation comparison; identity/provenance comparisons should use an explicitly named API if needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/query.rs`:
- Around line 789-792: The fixture currently only sets the neighbor relationship
in one direction (positive_simplex_key to negative_simplex_key) but the tests
expect bidirectional neighbor relationships. After the existing
tds.set_neighbors_by_key call for positive_simplex_key, add a reciprocal call
that sets the neighbor relationship from negative_simplex_key back to
positive_simplex_key using the same pattern, ensuring both simplices have
symmetric neighbor assignments without relying on undocumented setter side
effects.
🪄 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: 6a387d3f-3eb7-4bf7-9c3e-07a51a22a39c
📒 Files selected for processing (2)
src/core/query.rssrc/core/tds/equality.rs
💤 Files with no reviewable changes (1)
- src/core/tds/equality.rs
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: AdjacencyIndex now carries a TDS lifetime and borrowed-view query signatures, compact summary error exports were removed from the root and focused preludes, and TDS internals are organized under src/core/tds/* instead of src/core/tds.rs.
Closes #403