refactor(api)!: make topology views and boundaries owner-aware - #476
Merged
Conversation
- Replace raw TDS boundary queries with one-sided facet-incidence APIs and keep true boundary classification topology-aware. - Add borrowed EdgeView, RidgeQuery, RidgeView, and RidgeLinkView surfaces around detached EdgeKey and RidgeCandidate values. - Interpret boundary facets through GlobalTopology so periodic quotient self-identifications remain closed topology. - Align Euler validation, focused preludes, docs, examples, benchmarks, and Semgrep rules with the new view/candidate/incidence model. BREAKING CHANGE: BoundaryAnalysis and TDS boundary helpers are replaced by FacetIncidenceAnalysis and one-sided facet-incidence APIs. Use Triangulation::boundary_facets or DelaunayTriangulation::boundary_facets for semantic boundary queries. BREAKING CHANGE: FacetView construction now validates and caches live-TDS access, making accessors such as vertices, simplex, opposite_vertex, and key infallible; Triangulation::facets now returns its iterator directly. BREAKING CHANGE: Ridge topology now uses RidgeCandidate, RidgeQuery, RidgeView, and RidgeLinkView instead of detached RidgeVertices-style APIs, and topology-aware Euler/boundary helpers require GlobalTopology where classification matters. Closes #461
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)
WalkthroughAdds borrowed edge and ridge views, replaces boundary-analysis with one-sided facet incidence, threads ChangesFacet Incidence, Borrowed Topology Views, and Validation
Possibly related PRs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 | 817 |
🟢 Coverage 92.48% diff coverage · -0.05% coverage variation
Metric Results Coverage variation ✅ -0.05% coverage variation (-1.00%) Diff coverage ✅ 92.48% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (3b6d2d4) 70021 64115 91.57% Head commit (28faae2) 72534 (+2513) 66383 (+2268) 91.52% (-0.05%) 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 (#476) 4520 4180 92.48% 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
==========================================
- Coverage 91.54% 91.49% -0.05%
==========================================
Files 77 78 +1
Lines 69799 72313 +2514
==========================================
+ Hits 63895 66164 +2269
- Misses 5904 6149 +245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/core/tds/errors.rs (1)
1331-1354:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the other two new validation kinds to this “all variants” table.
The production mapping now covers
BoundaryFacetInClosedTopologyandPeriodicIdentificationInNonPeriodicTopology, but this test only addsRidgeNotFound, so two new error paths can regress unnoticed. As per coding guidelines, "Unit tests must cover known values, error paths, and dimension-generic correctness."🤖 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/errors.rs` around lines 1331 - 1354, The test `triangulation_validation_error_kind_from_error_preserves_all_variants` is incomplete and only includes a test case for the `RidgeNotFound` variant, but the production code now includes two additional variants: `BoundaryFacetInClosedTopology` and `PeriodicIdentificationInNonPeriodicTopology`. Add two more entries to the test cases array, following the same pattern as the existing cases (construct each variant with appropriate test values and specify the corresponding TriangulationValidationErrorKind variant). This ensures all error paths are covered by the test and regressions can be caught.Source: Coding guidelines
src/delaunay/query.rs (2)
928-979:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the stale outer
QueryErrordocs fromfacets().
facets()now returnsAllFacetsIterdirectly, so the# Errorssection claimingQueryError::TriangulationCorruptedcan be returned is no longer accurate. Keep only the per-itemFacetErrornote.Docs fix sketch
- /// # Errors - /// - /// Returns [`QueryError::TriangulationCorrupted`] if the facet iterator cannot - /// represent facet indices for this dimension. Individual iterator items - /// return [`FacetError`](crate::prelude::tds::FacetError) if a facet view - /// cannot be constructed from the current TDS state. + /// Individual iterator items return [`FacetError`](crate::prelude::tds::FacetError) + /// if a facet view cannot be constructed from the current TDS state.🤖 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/query.rs` around lines 928 - 979, The documentation for the `facets()` method contains stale error documentation that is no longer accurate. The `# Errors` section currently mentions that `QueryError::TriangulationCorrupted` can be returned, but since the method now returns `AllFacetsIter` directly without wrapping errors at that level, this claim is incorrect. Remove the part of the `# Errors` section that references `QueryError::TriangulationCorrupted`, keeping only the accurate documentation about how individual iterator items can return `FacetError` if a facet view cannot be constructed.
502-510:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
TopologyInvalidin the wrapper error docs.This wrapper forwards
Triangulation::boundary_facets(), which can now map topology-aware boundary classification failures toQueryError::TopologyInvalid; the docs only mention corrupted facet-index construction and still say “facet-map”.Docs fix sketch
- /// Returns [`QueryError::TriangulationCorrupted`] if facet-map construction + /// Returns [`QueryError::TriangulationCorrupted`] if facet-index construction /// detects invalid simplex or facet bookkeeping. The variant preserves the /// lower-level [`TdsError`] for diagnostics. + /// Returns [`QueryError::TopologyInvalid`] if topology-aware boundary + /// classification rejects one-sided facets for the triangulation's + /// `GlobalTopology`. /// Individual iterator items return [`FacetError`](crate::prelude::tds::FacetError) /// if a boundary facet cannot be created or keyed from the simplices.🤖 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/query.rs` around lines 502 - 510, The documentation comment for the `boundary_facets` method is incomplete and does not reflect all possible error conditions. The method now returns `QueryError::TopologyInvalid` in addition to `QueryError::TriangulationCorrupted`, but the docs only mention the latter. Update the Errors section in the doc comment to include both error variants: add documentation for `QueryError::TopologyInvalid` to describe when topology-aware boundary classification failures occur, while keeping the existing documentation for `QueryError::TriangulationCorrupted`. Ensure the docs accurately describe both error paths that can be returned from the underlying `self.tri.boundary_facets()` call.src/delaunay/repair.rs (1)
1160-1182:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a non-default topology or rename this regression test.
Line 1170 sets
global_topologyto the defaultEuclideanvalue, so a heuristic rebuild that accidentally resets metadata to the default would still pass. To keep this as a preservation regression, use a topology-compatible non-default fixture/value; otherwise rename the test to reflect that it only covers the default topology path.As per coding guidelines, “Unit tests must cover known values, error paths, and dimension-generic correctness.”
🤖 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/repair.rs` around lines 1160 - 1182, In the test function test_heuristic_rebuild_preserves_global_topology, the global_topology variable is set to GlobalTopology::Euclidean which is the default value. To properly test that the heuristic rebuild preserves the topology metadata, either change global_topology to use a non-default GlobalTopology variant (such as Spherical if available) before calling set_global_topology, or alternatively rename the test to accurately describe that it only validates the default topology scenario.Source: Coding guidelines
src/delaunay/builder.rs (1)
1368-1384:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate or reject non-Euclidean metadata before returning canonicalized builds.
Line 1383 attaches
self.global_topologyafter the canonicalized-only build has already been validated as an unre-wired Euclidean triangulation. If a caller explicitly set a closed toroidalglobal_topology, this can return aDelaunayTriangulationwhose topology-aware validation/boundary queries reject its raw open facets. Please either reject incompatibleself.global_topologyin this branch or set it before a final topology/Delaunay validation pass.As per coding guidelines, “Every mutating operation must preserve invariants checked by
Tds::is_valid(Level 1–3) andDelaunayTriangulation::is_valid(Level 4), or fail explicitly rather than leave triangulation in inconsistent state.”🤖 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/builder.rs` around lines 1368 - 1384, In the BuilderTopology::Canonicalized branch, the code calls dt.set_global_topology(self.global_topology) after DelaunayTriangulation::try_with_topology_guarantee_and_options has already validated the triangulation as Euclidean, breaking the invariant that topology and triangulation must be consistent. Either reject incompatible self.global_topology values before the DelaunayTriangulation construction (ensuring it matches the canonicalized toroidal setup), or pass the global_topology configuration into the try_with_topology_guarantee_and_options call itself so validation occurs with the correct topology context in place. This ensures that either the configuration is rejected early or the final triangulation undergoes validation with its intended topology already applied.Source: Coding guidelines
src/core/query.rs (1)
379-396:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new topology-invalid boundary query error.
boundary_facet_keys_from_index(...)can now fail withManifoldError, which is mapped toQueryError::TopologyInvalid, but the# Errorssection only mentions corrupted facet-index construction and still says “facet-map”. Please update the public docs so callers know closed-topology boundary rejection is an expected error path.Docs fix sketch
- /// Returns [`QueryError::TriangulationCorrupted`] if facet-map construction + /// Returns [`QueryError::TriangulationCorrupted`] if facet-index construction /// detects invalid simplex or facet bookkeeping. The variant preserves the /// underlying [`TdsError`] so callers can inspect the structural failure. + /// Returns [`QueryError::TopologyInvalid`] if topology-aware boundary + /// classification rejects one-sided facets for the triangulation's + /// `GlobalTopology`. /// Individual iterator items return [`FacetError`](crate::prelude::tds::FacetError) /// if a boundary facet cannot be created or keyed from the simplices.🤖 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/query.rs` around lines 379 - 396, The documentation for the boundary_facets() method needs to be updated to document the new error path. The # Errors section currently only mentions QueryError::TriangulationCorrupted but the method can also return QueryError::TopologyInvalid when boundary_facet_keys_from_index() fails with ManifoldError. Update the doc comment to include documentation for this new error variant, explaining that it occurs when the boundary query detects closed-topology manifolds that cannot have boundaries, and correct the reference from "facet-map" to be more accurate to what the code actually does.src/core/facet.rs (1)
220-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one-sided/two-sided terminology in this diagnostic.
This error still equates multiplicity
1with “boundary” and2with “internal”, but the new API separates TDS incidence from topology-aware boundary classification. Update the message and the matching test assertion to avoid reintroducing the old boundary semantics.Proposed wording update
/// Facet has invalid multiplicity (should be 1 for boundary or 2 for internal). #[error( - "Facet with key {facet_key:016x} has invalid multiplicity {found}, expected 1 (boundary) or 2 (internal)" + "Facet with key {facet_key:016x} has invalid multiplicity {found}, expected 1 (one-sided) or 2 (two-sided)" )]Also update the assertion in
src/core/facet_incidence.rs:- error_string.contains("expected 1 (boundary) or 2 (internal)"), + error_string.contains("expected 1 (one-sided) or 2 (two-sided)"),🤖 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/facet.rs` around lines 220 - 223, The InvalidMultiplicity error message uses outdated terminology equating multiplicity 1 with "boundary" and 2 with "internal", but the new API uses one-sided and two-sided terminology instead. Update the error string in the #[error(...)] attribute for the InvalidMultiplicity variant to replace "boundary" with "one-sided" and "internal" with "two-sided". Additionally, update any corresponding test assertion in src/core/facet_incidence.rs that matches against this error message to use the new terminology.
🧹 Nitpick comments (2)
src/geometry/algorithms/convex_hull.rs (1)
666-679: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDecouple the hull borrow from the returned
FacetViewlifetime.
FacetViewvalues are backed bytri, but the current signature uses one'tdslifetime for both&selfandtri, so collected views remain unnecessarily tied to the hull borrow after the iterator is consumed.♻️ Proposed lifetime split
- pub fn try_facets<'tds, K>( - &'tds self, - tri: &'tds Triangulation<K, U, V, D>, + pub fn try_facets<'hull, 'tds, K>( + &'hull self, + tri: &'tds Triangulation<K, U, V, D>, ) -> Result< - impl Iterator<Item = Result<FacetView<'tds, U, V, D>, FacetError>> + 'tds, + impl Iterator<Item = Result<FacetView<'tds, U, V, D>, FacetError>> + 'hull, ConvexHullConstructionError, - > { + > + where + 'tds: 'hull, + { self.ensure_current_for_construction(tri)?; let tds = &tri.tds; Ok(selfPlease verify with a compile test that collects facet views inside a scope where the hull is dropped while the triangulation remains alive.
🤖 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 666 - 679, The try_facets method uses a single lifetime 'tds for both the hull reference (&self) and the triangulation reference (&tri), which unnecessarily couples the FacetView lifetime to the hull's borrow. Introduce a separate lifetime parameter for the tri parameter (for example, 'a) that is distinct from 'tds used for &self, then update the return type's Iterator to use this new lifetime parameter instead of 'tds. This allows FacetView values to remain valid as long as the triangulation is alive, independent of the hull's borrow lifetime. After making this change, verify it compiles and test that facet views can be collected in a scope where the hull reference is dropped while the triangulation reference remains in scope.src/core/facet.rs (1)
1419-1425: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
#[must_use]to the public iterator wrappers.
SimplexFacetsIter,BoundaryFacetsIter, andOneSidedFacetsIterare public wrapper types; ignored iterator values should be caught consistently with the rest of the borrowed-view API.As per coding guidelines, “public wrapper types must be
#[must_use].”Proposed attributes
#[derive(Clone)] +#[must_use] pub struct SimplexFacetsIter<'tds, U, V, const D: usize> { tds: &'tds Tds<U, V, D>, simplex_key: SimplexKey, next_facet_index: u16, facet_count: u16, }+#[must_use] pub struct BoundaryFacetsIter<'tds, U, V, const D: usize> { all_facets: AllFacetsIter<'tds, U, V, D>, facet_to_simplices_index: FacetToSimplicesIndex<'tds, U, V, D>, boundary_facet_keys: FastHashSet<u64>, }#[derive(Clone)] +#[must_use] pub struct OneSidedFacetsIter<'tds, U, V, const D: usize> { all_facets: AllFacetsIter<'tds, U, V, D>, facet_to_simplices_index: FacetToSimplicesIndex<'tds, U, V, D>, }Also applies to: 1825-1829, 1915-1919
🤖 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/facet.rs` around lines 1419 - 1425, Add the `#[must_use]` attribute to the three public iterator wrapper structs SimplexFacetsIter, BoundaryFacetsIter, and OneSidedFacetsIter by placing it directly above each struct definition. This ensures that creating these iterators without using them will trigger compiler warnings, maintaining consistency with the rest of the borrowed-view API.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 `@semgrep.yaml`:
- Line 835: The pattern-regex in the struct detection rule contains an incorrect
negative lookahead assertion `(?!'')` that fails to properly exclude Rust
lifetime parameters (which use a single quote like 'tds). Change the negative
lookahead from `(?!'')` to `(?!')` in the pattern-regex on line 835 to correctly
reject lifetime-bound view types. Additionally, update
docs/dev/tooling-alignment.md to document this semgrep rule change with a
comparison of the old versus new regex pattern and the rationale for fixing the
lifetime parameter detection.
In `@src/core/algorithms/flips.rs`:
- Around line 15912-15988: The three test functions
build_k2_inverse_context_rejects_missing_endpoint_incidence_as_adjacency,
build_k2_inverse_context_rejects_missing_edge_incidence_as_adjacency, and
build_k2_inverse_context_rejects_vertex_incidence_mismatch_as_adjacency only
test dimension D=3 by hardcoding Tds<(), (), 3>, but
build_k2_flip_context_from_edge is const-generic and supports D≥3. Create a
macro that parameterizes these three test functions by dimension and generates
the same test cases for D=3, D=4, and D=5 to comply with the coding guidelines
requiring dimension-generic tests to cover D=2 through D=5 whenever feasible.
In `@src/core/edge.rs`:
- Around line 527-554: The validation loop only iterates through v0_star to
build incident_simplices, which means if a simplex exists in v1_star but is
missing from v0_star, that inconsistency is silently dropped. After the existing
loop that processes v0_star completes, add a second validation loop that
iterates through v1_star and ensures every simplex in v1_star is already
included in incident_simplices. If any simplex from v1_star is not found in
incident_simplices, return an appropriate error (similar to
EdgeKeyError::MissingVertexIncidence) to ensure the edge incidence is validated
bidirectionally and no missing entries are silently dropped.
In `@src/core/validation.rs`:
- Around line 446-454: The TopologyError::EulerMismatch match arm is incorrectly
mapping to InvariantError::Tds(TdsError::InconsistentDataStructure) when it
should route to InvariantError::Triangulation instead to maintain consistent
error provenance. Locate the TopologyError::EulerMismatch arm in the match
statement and change the error mapping from
InvariantError::Tds(TdsError::InconsistentDataStructure) to
InvariantError::Triangulation, ensuring the format and message remain
descriptive so callers see the same provenance as the explicit
EulerCharacteristicMismatch path elsewhere in the code.
In `@src/topology/ridge.rs`:
- Around line 1-9: The module documentation for the ridge module is missing two
required elements. First, add a reference citation pointing to the relevant
entry in REFERENCES.md for the ridge-star/link validation algorithm being
implemented. Second, add a note clarifying that the ridge-link implementation is
combinatorial in nature and does not introduce any new f64 floating-point
conditioning behavior. These additions should be incorporated into the existing
module-level documentation comment that begins with the module description about
ridges being codimension-2 simplices.
- Around line 909-920: The code uses `.position(...)` in multiple locations
(around lines 1087, 1090, 1456, and 1463) to find lifted vertex occurrences,
which only returns the first match. This breaks the contract that one
RidgeCandidate may correspond to multiple lifted ridge images, potentially
dropping periodic self-identification links. Replace the `.position(...)` calls
with iterator methods that enumerate all matching lifted positions (not just the
first), and process each matching occurrence/combination accordingly. Ensure
that normalization and anchoring are performed against the actual lifted target
image for each occurrence, rather than hardcoding against only the first
quotient-key match.
- Around line 1219-1224: The function build_ridge_star_map should return an
empty ridge map when the dimension D is less than 2, as RidgeCandidate rejects D
< 2 cases and enumerating empty ridges for 1D simplices can feed impossible
codimension-2 entries into validation. Add an early return check for D < 2 in
the build_ridge_star_map function that returns Ok(FastHashMap::default())
immediately, similar to the existing check for empty simplex_count.
In `@src/topology/spaces/toroidal.rs`:
- Around line 82-86: The struct LiftedVertexId is missing the #[must_use]
attribute. Add #[must_use] as an attribute above the existing #[derive(Clone,
Debug, Eq, PartialEq)] attribute on the LiftedVertexId struct definition to
ensure that values of this public wrapper type cannot be accidentally ignored by
callers, consistent with the project's coding guidelines and similar to how
LiftedLinkEdge is already annotated.
In `@tests/proptest_convex_hull.rs`:
- Line 116: The calls to number_of_one_sided_facets() at lines 116, 215, and 236
use unwrap_or(0) which silently converts query failures into zero values that
pass through prop_assume! filters, masking real regressions. Replace
unwrap_or(0) with unwrap() on the number_of_one_sided_facets() calls to fail
fast on actual errors instead of silently skipping them, allowing the degeneracy
checks via prop_assume! to only filter based on legitimate zero/empty results
rather than hidden failures.
---
Outside diff comments:
In `@src/core/facet.rs`:
- Around line 220-223: The InvalidMultiplicity error message uses outdated
terminology equating multiplicity 1 with "boundary" and 2 with "internal", but
the new API uses one-sided and two-sided terminology instead. Update the error
string in the #[error(...)] attribute for the InvalidMultiplicity variant to
replace "boundary" with "one-sided" and "internal" with "two-sided".
Additionally, update any corresponding test assertion in
src/core/facet_incidence.rs that matches against this error message to use the
new terminology.
In `@src/core/query.rs`:
- Around line 379-396: The documentation for the boundary_facets() method needs
to be updated to document the new error path. The # Errors section currently
only mentions QueryError::TriangulationCorrupted but the method can also return
QueryError::TopologyInvalid when boundary_facet_keys_from_index() fails with
ManifoldError. Update the doc comment to include documentation for this new
error variant, explaining that it occurs when the boundary query detects
closed-topology manifolds that cannot have boundaries, and correct the reference
from "facet-map" to be more accurate to what the code actually does.
In `@src/core/tds/errors.rs`:
- Around line 1331-1354: The test
`triangulation_validation_error_kind_from_error_preserves_all_variants` is
incomplete and only includes a test case for the `RidgeNotFound` variant, but
the production code now includes two additional variants:
`BoundaryFacetInClosedTopology` and
`PeriodicIdentificationInNonPeriodicTopology`. Add two more entries to the test
cases array, following the same pattern as the existing cases (construct each
variant with appropriate test values and specify the corresponding
TriangulationValidationErrorKind variant). This ensures all error paths are
covered by the test and regressions can be caught.
In `@src/delaunay/builder.rs`:
- Around line 1368-1384: In the BuilderTopology::Canonicalized branch, the code
calls dt.set_global_topology(self.global_topology) after
DelaunayTriangulation::try_with_topology_guarantee_and_options has already
validated the triangulation as Euclidean, breaking the invariant that topology
and triangulation must be consistent. Either reject incompatible
self.global_topology values before the DelaunayTriangulation construction
(ensuring it matches the canonicalized toroidal setup), or pass the
global_topology configuration into the try_with_topology_guarantee_and_options
call itself so validation occurs with the correct topology context in place.
This ensures that either the configuration is rejected early or the final
triangulation undergoes validation with its intended topology already applied.
In `@src/delaunay/query.rs`:
- Around line 928-979: The documentation for the `facets()` method contains
stale error documentation that is no longer accurate. The `# Errors` section
currently mentions that `QueryError::TriangulationCorrupted` can be returned,
but since the method now returns `AllFacetsIter` directly without wrapping
errors at that level, this claim is incorrect. Remove the part of the `# Errors`
section that references `QueryError::TriangulationCorrupted`, keeping only the
accurate documentation about how individual iterator items can return
`FacetError` if a facet view cannot be constructed.
- Around line 502-510: The documentation comment for the `boundary_facets`
method is incomplete and does not reflect all possible error conditions. The
method now returns `QueryError::TopologyInvalid` in addition to
`QueryError::TriangulationCorrupted`, but the docs only mention the latter.
Update the Errors section in the doc comment to include both error variants: add
documentation for `QueryError::TopologyInvalid` to describe when topology-aware
boundary classification failures occur, while keeping the existing documentation
for `QueryError::TriangulationCorrupted`. Ensure the docs accurately describe
both error paths that can be returned from the underlying
`self.tri.boundary_facets()` call.
In `@src/delaunay/repair.rs`:
- Around line 1160-1182: In the test function
test_heuristic_rebuild_preserves_global_topology, the global_topology variable
is set to GlobalTopology::Euclidean which is the default value. To properly test
that the heuristic rebuild preserves the topology metadata, either change
global_topology to use a non-default GlobalTopology variant (such as Spherical
if available) before calling set_global_topology, or alternatively rename the
test to accurately describe that it only validates the default topology
scenario.
---
Nitpick comments:
In `@src/core/facet.rs`:
- Around line 1419-1425: Add the `#[must_use]` attribute to the three public
iterator wrapper structs SimplexFacetsIter, BoundaryFacetsIter, and
OneSidedFacetsIter by placing it directly above each struct definition. This
ensures that creating these iterators without using them will trigger compiler
warnings, maintaining consistency with the rest of the borrowed-view API.
In `@src/geometry/algorithms/convex_hull.rs`:
- Around line 666-679: The try_facets method uses a single lifetime 'tds for
both the hull reference (&self) and the triangulation reference (&tri), which
unnecessarily couples the FacetView lifetime to the hull's borrow. Introduce a
separate lifetime parameter for the tri parameter (for example, 'a) that is
distinct from 'tds used for &self, then update the return type's Iterator to use
this new lifetime parameter instead of 'tds. This allows FacetView values to
remain valid as long as the triangulation is alive, independent of the hull's
borrow lifetime. After making this change, verify it compiles and test that
facet views can be collected in a scope where the hull reference is dropped
while the triangulation reference remains in scope.
🪄 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: 64341cc9-c67b-4c26-839b-f81a6114d4fe
📒 Files selected for processing (53)
benches/boundary_uuid_iter.rsbenches/common/flip_workflows.rsbenches/profiling_suite.rsdocs/api_design.mddocs/code_organization.mddocs/dev/rust.mddocs/dev/tooling-alignment.mddocs/invariants.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/triangulation_and_hull.rssemgrep.yamlsrc/bench_fixtures.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/collections/triangulation_maps.rssrc/core/edge.rssrc/core/facet.rssrc/core/facet_incidence.rssrc/core/query.rssrc/core/simplex.rssrc/core/tds/errors.rssrc/core/tds/storage.rssrc/core/tds/validation.rssrc/core/traits/boundary_analysis.rssrc/core/traits/facet_cache.rssrc/core/traits/facet_incidence_analysis.rssrc/core/util/facet_utils.rssrc/core/util/jaccard.rssrc/core/validation.rssrc/delaunay/builder.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/util/measures.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rssrc/topology/ridge.rssrc/topology/spaces/toroidal.rssrc/topology/traits/topological_space.rstests/euler_characteristic.rstests/example_workflows.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_euler_characteristic.rstests/proptest_facet.rstests/semgrep/src/project_rules/rust_style.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (1)
- src/core/traits/boundary_analysis.rs
- Distinguish one-sided facet incidence from semantic boundary facets, with boundary iterators driven by topology-approved facet handles. - Validate facet-degree maps once as `ValidatedFacetDegreeMap` and reuse that proof through manifold, Euler, vertex-link, and boundary checks. - Make construction paths derive or reject `GlobalTopology` metadata so Euclidean, canonicalized toroidal, and periodic image-point builds cannot silently describe different quotient topology. - Tighten ridge/view/prelude docs and semgrep coverage around borrowed views, parse-don't-validate boundaries, and topology organization. BREAKING CHANGE: `Triangulation::set_global_topology` is replaced by `Triangulation::try_set_global_topology`; `GlobalTopologyModel::periodic_domain` returns `Option<ToroidalDomain<D>>` instead of `Option<&[f64; D]>`; `TopologyError::EulerMismatch` is removed; and builders now reject unsupported or conflicting global topology metadata instead of accepting metadata that does not match constructed connectivity.
acgetchell
enabled auto-merge (squash)
June 23, 2026 01:29
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/topology/traits/global_topology_model.rs (1)
247-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate coordinates before early-returning from
lift_for_orientation.With the current ordering,
lift_for_orientation([f64::NAN, …], None)or canonicalized mode with any offset returnsOk(coords)without enforcing the method’s non-finite-coordinate error contract.🛡️ Proposed fix
fn lift_for_orientation( &self, mut coords: [f64; D], periodic_offset: Option<[i8; D]>, ) -> Result<[f64; D], GlobalTopologyModelError> { + // Validate finiteness before returning or performing arithmetic. + for (axis, coord) in coords.iter().copied().enumerate() { + if !coord.is_finite() { + return Err(GlobalTopologyModelError::NonFiniteCoordinate { axis, value: coord }); + } + } + // Canonicalized toroidal mode intentionally accepts optional periodic offsets but // does not apply them. This differs from `EuclideanModel`, which treats any // provided periodic offset as unsupported and returns an error. if !self.supports_periodic_orientation_offsets() { return Ok(coords); } let Some(offset) = periodic_offset else { return Ok(coords); }; - - // Validate finiteness before performing arithmetic - for (axis, coord) in coords.iter().copied().enumerate() { - if !coord.is_finite() { - return Err(GlobalTopologyModelError::NonFiniteCoordinate { axis, value: coord }); - } - } for axis in 0..D {🤖 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/topology/traits/global_topology_model.rs` around lines 247 - 260, The `lift_for_orientation` method has early return statements that skip coordinate validation when `periodic_offset` is `None` or when `!self.supports_periodic_orientation_offsets()` returns true. This allows non-finite coordinate values (like NaN or infinity) to be returned without error, violating the method's contract. Move the coordinate validation logic to execute before these early returns, or add validation at the beginning of the function to ensure all coordinate values are checked for finiteness regardless of which return path is taken.src/delaunay/repair.rs (1)
1160-1184: 📐 Maintainability & Code Quality | 🟡 MinorPreservation test uses default topology with limited discrimination power.
test_heuristic_rebuild_preserves_default_global_topologysetsGlobalTopology::Euclidean, which is the value a freshly constructed candidate would carry by default. If a future regression dropped theglobal_topologyargument fromwith_empty_kernel_and_topology_context(lines 749–753), the candidate would still default toEuclideanand this assertion would pass despite the regression.However, strengthening this test via non-default topology is not feasible:
try_set_global_topology(GlobalTopology::Spherical)fails validation on planar triangulations (rejects withBoundaryFacetInClosedTopology), andGlobalTopology::Toroidalcannot be set post-hoc on Euclidean fixtures—it requires construction via the builder. Consider instead adding an explicit assertion that the topology context argument is actually threaded through the rebuild path, or testing the rebuild with a fixture that can legitimately support a non-Euclidean topology.🤖 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/repair.rs` around lines 1160 - 1184, The test `test_heuristic_rebuild_preserves_default_global_topology` uses the default `GlobalTopology::Euclidean` value, which means if the `global_topology` argument is inadvertently dropped from the rebuild operation in the future, the test would still pass. Add an explicit assertion or verification mechanism that confirms the topology context is actually being threaded through the rebuild path in the repair operation, rather than relying on the implicit default behavior. This could involve adding a test fixture that explicitly tracks or verifies the topology context propagation during the rebuild process.
🧹 Nitpick comments (1)
src/delaunay/builder.rs (1)
1798-1807: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused test for the new D > 3 periodic guard.
This new typed error path should be covered directly so future refactors do not accidentally fall through into image construction or a different precondition error.
🧪 Suggested test coverage
+ #[test] + fn test_builder_toroidal_rejects_unsupported_dimension_4d() { + let vertices: Vec<Vertex<(), 4>> = Vec::new(); + let result = DelaunayTriangulationBuilder::new(&vertices) + .try_toroidal([1.0; 4]) + .unwrap() + .build::<()>(); + + assert_matches!( + result, + Err(DelaunayTriangulationConstructionError::Triangulation( + DelaunayConstructionFailure::UnsupportedPeriodicDimension { + dimension: 4, + max_validated_dimension: 3, + tracking_issue: 416, + } + )) + ); + }As per coding guidelines, “Unit tests must cover known values, error paths, and dimension-generic correctness.”
🤖 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/builder.rs` around lines 1798 - 1807, Add a focused unit test in the builder test module that validates the new periodic dimension guard in the delaunay builder. The test should attempt to construct a periodic Delaunay triangulation with a dimension D greater than 3 and verify that it properly returns the TriangulationConstructionError::UnsupportedPeriodicDimension error with the correct dimension and tracking_issue values. This test ensures the error path is explicitly covered and prevents future refactors from accidentally bypassing this precondition check.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/delaunay/repair.rs`:
- Around line 1160-1184: The test
`test_heuristic_rebuild_preserves_default_global_topology` uses the default
`GlobalTopology::Euclidean` value, which means if the `global_topology` argument
is inadvertently dropped from the rebuild operation in the future, the test
would still pass. Add an explicit assertion or verification mechanism that
confirms the topology context is actually being threaded through the rebuild
path in the repair operation, rather than relying on the implicit default
behavior. This could involve adding a test fixture that explicitly tracks or
verifies the topology context propagation during the rebuild process.
In `@src/topology/traits/global_topology_model.rs`:
- Around line 247-260: The `lift_for_orientation` method has early return
statements that skip coordinate validation when `periodic_offset` is `None` or
when `!self.supports_periodic_orientation_offsets()` returns true. This allows
non-finite coordinate values (like NaN or infinity) to be returned without
error, violating the method's contract. Move the coordinate validation logic to
execute before these early returns, or add validation at the beginning of the
function to ensure all coordinate values are checked for finiteness regardless
of which return path is taken.
---
Nitpick comments:
In `@src/delaunay/builder.rs`:
- Around line 1798-1807: Add a focused unit test in the builder test module that
validates the new periodic dimension guard in the delaunay builder. The test
should attempt to construct a periodic Delaunay triangulation with a dimension D
greater than 3 and verify that it properly returns the
TriangulationConstructionError::UnsupportedPeriodicDimension error with the
correct dimension and tracking_issue values. This test ensures the error path is
explicitly covered and prevents future refactors from accidentally bypassing
this precondition check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e71bce44-a46e-40c3-9246-0306cca7399f
📒 Files selected for processing (40)
benches/profiling_suite.rsdocs/dev/tooling-alignment.mddocs/topology.mdsemgrep.yamlsrc/bench_fixtures.rssrc/core/algorithms/flips.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/collections/buffers.rssrc/core/collections/triangulation_maps.rssrc/core/edge.rssrc/core/facet.rssrc/core/facet_incidence.rssrc/core/orientation.rssrc/core/query.rssrc/core/tds/errors.rssrc/core/tds/validation.rssrc/core/traits/facet_incidence_analysis.rssrc/core/triangulation.rssrc/core/validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/insertion.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/lib.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rssrc/topology/ridge.rssrc/topology/spaces/toroidal.rssrc/topology/traits/global_topology_model.rssrc/topology/traits/topological_space.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_facet.rstests/proptest_triangulation.rstests/semgrep/src/project_rules/rust_style.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
✅ Files skipped from review due to trivial changes (6)
- src/core/collections/buffers.rs
- tests/proptest_triangulation.rs
- src/core/traits/facet_incidence_analysis.rs
- docs/dev/tooling-alignment.md
- src/core/collections/triangulation_maps.rs
- docs/topology.md
🚧 Files skipped from review as they are similar to previous changes (20)
- benches/profiling_suite.rs
- tests/proptest_convex_hull.rs
- src/core/algorithms/pl_manifold_repair.rs
- src/bench_fixtures.rs
- tests/triangulation_builder.rs
- src/core/tds/errors.rs
- semgrep.yaml
- src/core/algorithms/flips.rs
- src/core/edge.rs
- src/core/tds/validation.rs
- src/core/query.rs
- tests/trait_bound_ergonomics.rs
- tests/prelude_exports.rs
- src/topology/spaces/toroidal.rs
- tests/semgrep/src/project_rules/rust_style.rs
- src/topology/ridge.rs
- src/core/facet_incidence.rs
- src/lib.rs
- src/geometry/algorithms/convex_hull.rs
- src/core/facet.rs
- Reject non-finite toroidal orientation-lift coordinates before offset and mode short-circuits. - Keep heuristic rebuilds covered for non-default topology context and periodic construction covered for unsupported dimensions. - Recalibrate large-scale smoke point counts for the current performance envelope.
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 `@justfile`:
- Around line 578-581: The justfile was modified to add or change the run_case
calls for debug_large_scale_2d, debug_large_scale_3d, debug_large_scale_4d, and
debug_large_scale_5d (lines 578-581), but the corresponding update to
docs/dev/tooling-alignment.md is missing. Add a comparison and rationale section
in docs/dev/tooling-alignment.md explaining the purpose of these debug large
scale test cases and the justification for the specific parameters used (point
counts like 32000, 9000, 1000, 160 and chunk sizes like 2000, 500, 100, 20) to
ensure alignment with project tooling documentation standards.
🪄 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: b0a5a483-fba7-43f7-97eb-b4b5bad2a979
📒 Files selected for processing (4)
justfilesrc/delaunay/builder.rssrc/delaunay/repair.rssrc/topology/traits/global_topology_model.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/delaunay/repair.rs
- src/topology/traits/global_topology_model.rs
- src/delaunay/builder.rs
- Explain why `perf-large-scale-smoke` is a Delaunay-specific local guard over release-mode debug large-scale cases. - Record the per-dimension point counts, progress chunks, and roughly 50-second calibration target under the 60-second cap.
- Enumerate all lifted ridge images for periodic ridge links instead of collapsing to the first quotient-key match. - Normalize simplex images against the actual lifted target anchor so toroidal link components keep their covering-space identity. - Move derived ridge-star buffers into link views without extra cloning.
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: BoundaryAnalysis and TDS boundary helpers are replaced by FacetIncidenceAnalysis and one-sided facet-incidence APIs. Use Triangulation::boundary_facets or DelaunayTriangulation::boundary_facets for semantic boundary queries.
BREAKING CHANGE: FacetView construction now validates and caches live-TDS access, making accessors such as vertices, simplex, opposite_vertex, and key infallible; Triangulation::facets now returns its iterator directly.
BREAKING CHANGE: Ridge topology now uses RidgeCandidate, RidgeQuery, RidgeView, and RidgeLinkView instead of detached RidgeVertices-style APIs, and topology-aware Euler/boundary helpers require GlobalTopology where classification matters.
Closes #461