refactor(api)!: make fluent Delaunay builders canonical - #499
Merged
Conversation
- Make DelaunayTriangulationBuilder the canonical construction API, with default simplex storage, typed simplex-data selection, kernel terminals, and statistics terminals in one staged workflow. - Remove the legacy DelaunayTriangulation::try_new* and try_with_* batch constructor family, and update examples, benches, docs, preludes, and semgrep rules to use builder chains. - Add post-construction simplex-data filling from closures or secondary maps with typed SimplexDataFillError handling. - Rework RandomTriangulationBuilder around validated point counts, coordinate ranges, vertex/simplex data type selection, and fluent build terminals. - Refresh public Pachner, Delaunay repair, and locate workflows around the current proposal and builder APIs. BREAKING CHANGE: DelaunayTriangulation::try_new* and try_with_* batch constructors have been removed. Use DelaunayTriangulation::builder(...).build() or DelaunayTriangulationBuilder::new(...).build(), and select simplex payload storage with simplex_data_type::<V>() before construction.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughThis PR replaces legacy triangulation construction with the fluent builder API across the crate, adds typed simplex-payload and construction-statistics support, introduces simplex-data fill APIs, refactors random triangulation generation, updates benchmarks/docs/tests, and removes postponed annotation imports from Python tooling scripts. ChangesDelaunay Builder API Finalization
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
acgetchell
enabled auto-merge (squash)
July 3, 2026 07:23
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 126 |
🟢 Coverage 98.67% diff coverage · +0.05% coverage variation
Metric Results Coverage variation ✅ +0.05% coverage variation (-1.00%) Diff coverage ✅ 98.67% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (be6728c) 77334 70394 91.03% Head commit (0be2880) 77903 (+569) 70953 (+559) 91.08% (+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 (#499) 1352 1334 98.67% 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.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
==========================================
+ Coverage 91.00% 91.05% +0.05%
==========================================
Files 87 87
Lines 77112 77679 +567
==========================================
+ Hits 70174 70731 +557
- Misses 6938 6948 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
coderabbitai
Bot
added
api
breaking change
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/proptest_triangulation.rs (1)
96-155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail partial simplex matches instead of discarding them.
prop_assume!(matched_simplices >= 1)lets this property pass even when only a subset of simplices matched, so translation/scale regressions can slip through unnoticed on Line 152. This should fail unless every original simplex found a counterpart.🔧 Suggested fix
- prop_assume!(matched_simplices >= 1); + let expected_simplices = tds_orig.simplex_keys().count(); + prop_assert_eq!(matched_simplices, expected_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 `@tests/proptest_triangulation.rs` around lines 96 - 155, The matching logic in compare_transformed_simplices is too permissive because prop_assume!(matched_simplices >= 1) allows partial matches to pass. Change this to require a full one-to-one match between all simplices from dt_orig and dt_transformed, so the property fails whenever any original simplex has no counterpart. Use matched_simplices together with the total simplex count from tds_orig (and/or tds_transformed) to assert completeness, and keep the existing compare_fn call only for confirmed matching simplex pairs.
🧹 Nitpick comments (5)
tests/serialization_vertex_preservation.rs (1)
56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing
.expect()panic message.
.expect("Tds construction succeeded")fires only whenbuild()returnsErr, so the panic will misleadingly read "Tds construction succeeded: ". Line 118 in this same file uses the correctly-phrased "Tds construction failed" for the analogous case.✏️ Proposed fix
- .expect("Tds construction succeeded"); + .expect("Tds construction should succeed despite duplicate 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 `@tests/serialization_vertex_preservation.rs` around lines 56 - 59, The `.expect()` message in the `DelaunayTriangulation::builder(...).build()` test is backwards and will produce a misleading panic if construction fails. Update the expectation string in this test case to match the failure path, consistent with the analogous check later in the same file, so the panic message clearly indicates `build()` failed rather than succeeded.src/delaunay/construction.rs (1)
5073-5078: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate the duplicated
DedupPolicy::Epsilonmatching.
grid_cell_size_valueand the per-vertexepsilonare derived from two separate matches on the samededup_policya few lines apart. Extracting a small helper (e.g., returning(epsilon, grid_cell_size)) would remove the risk of the two arms drifting apart on a future edit, but current behavior is correct and covered by tests.Also applies to: 5092-5093
🤖 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 5073 - 5078, The DedupPolicy::Epsilon logic is duplicated when deriving grid_cell_size_value and the per-vertex epsilon, so the two match expressions can drift apart over time. Refactor the nearby matching in construction.rs around the dedup_policy handling into a small helper that returns both values together, and update the code that sets grid_cell_size_value and the epsilon assignment to use that shared result while preserving the existing behavior for DedupPolicy::Epsilon, DedupPolicy::Exact, and DedupPolicy::Off.src/delaunay/builder.rs (1)
1667-1728: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting shared Euclidean/Canonicalized validation logic to avoid drift between
build_with_kernelandbuild_with_kernel_and_statistics.The Euclidean/Canonicalized branches here (topology rejection, model validation, canonicalization) duplicate the corresponding logic in
build_with_kernel(lines 1525-1556), differing only in callingbuild_with_kernel_options_and_statisticsvsbuild_with_kernel_options. A future fix/change to one path (e.g., a new topology check) could easily be missed in the other, silently makingbuild()andbuild_with_statistics()diverge in behavior.Consider factoring the shared "validate + canonicalize" preamble into a helper that both
build_with_kernelandbuild_with_kernel_and_statisticscall before choosing which underlying construction function to invoke.🤖 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 1667 - 1728, The Euclidean and Canonicalized branches in build_with_kernel_and_statistics duplicate the same validation/canonicalization preamble already present in build_with_kernel, so the two build paths can drift. Extract the shared topology rejection, model validation, and canonicalize_vertices setup into a helper used by both build_with_kernel and build_with_kernel_and_statistics, then have each path only choose between build_with_kernel_options and build_with_kernel_options_and_statistics for the final construction call.tests/semgrep/src/project_rules/rust_style.rs (1)
379-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend
legacy_delaunay_batch_constructors_badto cover all 9 legacy variants.Only 4 of the 9 name variants matched by the new
no-legacy-delaunay-try-new-constructorsregex (try_new_with_construction_statistics,try_new_with_topology_guarantee,try_with_topology_guarantee,try_with_topology_guarantee_and_options,try_with_options_and_statisticsare missing) are exercised here. Adding the remaining cases would give full regex-coverage confidence for this new rule.🤖 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/semgrep/src/project_rules/rust_style.rs` around lines 379 - 399, `legacy_delaunay_batch_constructors_bad` only exercises 4 of the 9 legacy `DelaunayTriangulation` constructor variants covered by the new `no-legacy-delaunay-try-new-constructors` regex. Extend this function to add the missing legacy calls using the same pattern and `ruleid` annotations, specifically covering `try_new_with_construction_statistics`, `try_new_with_topology_guarantee`, `try_with_topology_guarantee`, `try_with_topology_guarantee_and_options`, and `try_with_options_and_statistics`, so the test validates full regex coverage.examples/topology_editing.rs (1)
606-640: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFine for a demo, but brute-force facet search clones the whole triangulation per candidate.
find_roundtrip_k2_facet_3dclonesdtand attempts a forward+inverse Pachner move for every internal facet until one round-trips. For the small fixed 9-vertex fixture this is negligible, but it's worth keeping in mind if this helper is ever reused with larger inputs.🤖 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 `@examples/topology_editing.rs` around lines 606 - 640, The helper find_roundtrip_k2_facet_3d is doing a full dt.clone() and Pachner trial for every candidate facet, which makes the search unnecessarily expensive. Refactor the search to avoid cloning the whole triangulation per iteration—either prefilter candidate facets more cheaply before trialing, or restructure the loop so the expensive clone/propose_pachner/attempt_on path in find_roundtrip_k2_facet_3d is only used on a much smaller set of likely internal facets.
🤖 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`:
- Around line 629-673: Broaden the semgrep rule so it also matches the
specialized and bounded DelaunayTriangulation impl blocks used under
src/delaunay, since the current exact impl<$K, $U, $V, const $D: usize>
DelaunayTriangulation<$K, $U, $V, $D> shape can miss legacy constructor
wrappers. Update the pattern-inside around DelaunayTriangulation to cover those
variant impl headers while keeping the existing pattern-either checks for
try_new and try_with_* methods, so reintroduced legacy constructors are still
caught.
---
Outside diff comments:
In `@tests/proptest_triangulation.rs`:
- Around line 96-155: The matching logic in compare_transformed_simplices is too
permissive because prop_assume!(matched_simplices >= 1) allows partial matches
to pass. Change this to require a full one-to-one match between all simplices
from dt_orig and dt_transformed, so the property fails whenever any original
simplex has no counterpart. Use matched_simplices together with the total
simplex count from tds_orig (and/or tds_transformed) to assert completeness, and
keep the existing compare_fn call only for confirmed matching simplex pairs.
---
Nitpick comments:
In `@examples/topology_editing.rs`:
- Around line 606-640: The helper find_roundtrip_k2_facet_3d is doing a full
dt.clone() and Pachner trial for every candidate facet, which makes the search
unnecessarily expensive. Refactor the search to avoid cloning the whole
triangulation per iteration—either prefilter candidate facets more cheaply
before trialing, or restructure the loop so the expensive
clone/propose_pachner/attempt_on path in find_roundtrip_k2_facet_3d is only used
on a much smaller set of likely internal facets.
In `@src/delaunay/builder.rs`:
- Around line 1667-1728: The Euclidean and Canonicalized branches in
build_with_kernel_and_statistics duplicate the same validation/canonicalization
preamble already present in build_with_kernel, so the two build paths can drift.
Extract the shared topology rejection, model validation, and
canonicalize_vertices setup into a helper used by both build_with_kernel and
build_with_kernel_and_statistics, then have each path only choose between
build_with_kernel_options and build_with_kernel_options_and_statistics for the
final construction call.
In `@src/delaunay/construction.rs`:
- Around line 5073-5078: The DedupPolicy::Epsilon logic is duplicated when
deriving grid_cell_size_value and the per-vertex epsilon, so the two match
expressions can drift apart over time. Refactor the nearby matching in
construction.rs around the dedup_policy handling into a small helper that
returns both values together, and update the code that sets grid_cell_size_value
and the epsilon assignment to use that shared result while preserving the
existing behavior for DedupPolicy::Epsilon, DedupPolicy::Exact, and
DedupPolicy::Off.
In `@tests/semgrep/src/project_rules/rust_style.rs`:
- Around line 379-399: `legacy_delaunay_batch_constructors_bad` only exercises 4
of the 9 legacy `DelaunayTriangulation` constructor variants covered by the new
`no-legacy-delaunay-try-new-constructors` regex. Extend this function to add the
missing legacy calls using the same pattern and `ruleid` annotations,
specifically covering `try_new_with_construction_statistics`,
`try_new_with_topology_guarantee`, `try_with_topology_guarantee`,
`try_with_topology_guarantee_and_options`, and
`try_with_options_and_statistics`, so the test validates full regex coverage.
In `@tests/serialization_vertex_preservation.rs`:
- Around line 56-59: The `.expect()` message in the
`DelaunayTriangulation::builder(...).build()` test is backwards and will produce
a misleading panic if construction fails. Update the expectation string in this
test case to match the failure path, consistent with the analogous check later
in the same file, so the panic message clearly indicates `build()` failed rather
than succeeded.
🪄 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: 9e321b3a-666a-4031-af07-cf65ed9a94f5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (129)
CHANGELOG.mdCargo.tomlREADME.mdbenches/PERFORMANCE_RESULTS.mdbenches/README.mdbenches/allocation_hot_paths.rsbenches/boundary_uuid_iter.rsbenches/ci_performance_suite.rsbenches/common/flip_workflows.rsbenches/delaunay_repair.rsbenches/delete_vertex.rsbenches/edge_key_queries.rsbenches/locate.rsbenches/pachner_stress.rsbenches/profiling_suite.rsbenches/tds_clone.rsdocs/api_design.mddocs/architecture/project_structure.mddocs/dev/rust.mddocs/diagnostics.mddocs/mesh_export.mddocs/numerical_robustness_guide.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/into_from_conversions.rsexamples/numerical_robustness.rsexamples/topology_editing.rsexamples/triangulation_and_hull.rspyproject.tomlscripts/archive_changelog.pyscripts/ci/filter_codacy_sarif.pyscripts/notebook_check.pyscripts/postprocess_changelog.pyscripts/semgrep_fixture_config.pyscripts/tag_release.pyscripts/tests/conftest.pyscripts/tests/test_archive_changelog.pyscripts/tests/test_filter_codacy_sarif.pyscripts/tests/test_notebook_check.pyscripts/tests/test_postprocess_changelog.pyscripts/tests/test_readme_citation_mirror.pyscripts/tests/test_semgrep_fixture_config.pysemgrep.yamlsrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/collections/key_maps.rssrc/core/collections/secondary_maps.rssrc/core/edge.rssrc/core/embedding.rssrc/core/facet.rssrc/core/facet_incidence.rssrc/core/insertion.rssrc/core/query.rssrc/core/repair.rssrc/core/simplex.rssrc/core/tds/equality.rssrc/core/tds/errors.rssrc/core/tds/keys.rssrc/core/tds/mutation.rssrc/core/tds/snapshot.rssrc/core/tds/storage.rssrc/core/tds/validation.rssrc/core/traits/facet_cache.rssrc/core/triangulation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/jaccard.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/deletion.rssrc/delaunay/diagnostics.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/locality.rssrc/delaunay/pachner.rssrc/delaunay/property_validation.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/rollback.rssrc/delaunay/serialization.rssrc/delaunay/triangulation.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/quality.rssrc/geometry/util/measures.rssrc/geometry/util/triangulation_generation.rssrc/io/visualization.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rssrc/topology/ridge.rssrc/topology/spaces/toroidal.rstests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/example_workflows.rstests/large_scale_debug.rstests/mesh_export.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_delaunay_triangulation.rstests/proptest_euler_characteristic.rstests/proptest_facet.rstests/proptest_flips.rstests/proptest_orientation.rstests/proptest_serialization.rstests/proptest_simplex.rstests/proptest_tds.rstests/proptest_triangulation.rstests/public_topology_api.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (15)
- benches/boundary_uuid_iter.rs
- scripts/archive_changelog.py
- scripts/postprocess_changelog.py
- scripts/tests/conftest.py
- scripts/semgrep_fixture_config.py
- scripts/tests/test_semgrep_fixture_config.py
- scripts/ci/filter_codacy_sarif.py
- scripts/tests/test_filter_codacy_sarif.py
- scripts/tag_release.py
- scripts/notebook_check.py
- scripts/tests/test_postprocess_changelog.py
- benches/edge_key_queries.rs
- scripts/tests/test_notebook_check.py
- scripts/tests/test_readme_citation_mirror.py
- scripts/tests/test_archive_changelog.py
- Share Euclidean and canonicalized topology preparation across builder terminals so statistics and non-statistics construction reject and canonicalize consistently. - Keep deduplication grid sizing and epsilon tolerance derived together to preserve zero-tolerance behavior. - Tighten fluent-constructor guardrails, transformed-simplex matching, and topology-editing example prefilters.
coderabbitai
Bot
added
the
documentation
This was referenced Aug 14, 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: DelaunayTriangulation::try_new* and try_with_* batch constructors have been removed. Use DelaunayTriangulation::builder(...).build() or DelaunayTriangulationBuilder::new(...).build(), and select simplex payload storage with simplex_data_type::() before construction.