refactor(api)!: tighten invariant-bearing triangulation APIs#511
Conversation
- Encapsulate flip-kind dimensions and periodic span witnesses behind accessors while adding fluent repair and delaunayize config setters.
- Relax unnecessary serialization, surface-measure, and coordinate-representation bounds so callers can use narrower payload and kernel types.
- Precheck explicit simplex imports for duplicate cells and over-shared facets before using the prechecked TDS assembly path.
- Add explicit-import benchmark coverage, consolidate repeated dimension tests, and quiet ad hoc test logging.
- Align uv-backed workflows on uv 0.11.27 and run the papers workflow on macOS with Homebrew system dependencies.
BREAKING CHANGE: BistellarFlipKind::d and PeriodicSimplexSpan::{axis, span, period} are private; use d(), axis(), span(), and period() instead.
Closes #453
WalkthroughThis PR performs a broad invariant-preserving audit pass: adds fluent builder APIs for DelaunayizeConfig and DelaunayRepairHeuristicConfig, converts CLI vertex counts to NonZeroUsize, relaxes several trait bounds (Serialize, CoordinateRepresentation, surface_measure), encapsulates BistellarFlipKind's dimension field, adds a duplicate-index rebuild headroom optimization, prechecks explicit-simplex topology in the builder, refines error classification logic, adds an explicit-import benchmark, bumps uv versions, migrates the papers CI job to macOS, and cleans up test logging/imports. ChangesCI/Tooling Maintenance
Explicit-Import Benchmark
Typed Vertex-Count Validation
BistellarFlipKind Dimension Accessor
Insertion Retryability Classification
PeriodicSimplexSpan Accessors
Duplicate-Index Rebuild Headroom
Construction Helper Refactors
Explicit-Simplex Topology Precheck
Fluent Config Builder APIs
Relaxed Trait Bounds
Test Logging, Imports, and Harness Cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -13 |
🟢 Coverage 98.08% diff coverage · +0.02% coverage variation
Metric Results Coverage variation ✅ +0.02% coverage variation (-1.00%) Diff coverage ✅ 98.08% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (703eb1e) 79921 72752 91.03% Head commit (e7595d3) 80003 (+82) 72839 (+87) 91.05% (+0.02%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#511) 313 307 98.08% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
api
breaking change
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
.github/workflows/papers.yml (1)
55-59: 🧹 Nitpick | 🔵 TrivialmacOS migration looks consistent.
Runner switch to
macos-latestand Homebrew-based dependency install look correctly wired with the rest of the pipeline (Rust toolchain, uv, Tectonic all cross-platform). Worth noting macOS runners consume GitHub Actions minutes at a higher multiplier than Linux, so per-run cost/duration will increase for this job.Also applies to: 89-90
🤖 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 @.github/workflows/papers.yml around lines 55 - 59, The papers workflow has been switched to macOS, which increases GitHub Actions runtime cost; review the job in papers.yml around the papers job and either confirm this higher-cost runner is intentional or reduce usage by narrowing when the job runs or moving it back to a Linux runner if macOS is not required. Keep the existing Rust/uv/Tectonic setup intact while making any runner or trigger adjustment.src/delaunay/repair.rs (1)
198-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistent with the
DelaunayizeConfigbuilder pattern.Same with_/without_ shape duplicated across two config structs (here and
DelaunayizeConfig). Could be reduced with a small macro if more config types adopt this pattern in the future, but not worth doing for just two structs today.🤖 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 198 - 263, The builder methods in DelaunayRepairHeuristicConfig should follow the same with_/without_ pattern used by DelaunayizeConfig for consistency. Review the existing builder methods like with_shuffle_seed, with_perturbation_seed, with_max_flips, and without_max_flips, and make their naming/signature style match the corresponding methods in DelaunayizeConfig without introducing extra abstraction unless it is already used there.src/config.rs (1)
255-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: shared helper for typed non-zero vertex validation.
The
NonZeroUsize::new(...).filter(...).ok_or(...)pattern is duplicated betweenGenerateConfig::try_newandPachnerStressConfig::try_newwith only the error type/threshold differing. A small generic helper (e.g. taking a predicate and an error-constructing closure) could remove the duplication, but given only two call sites and differing error types, this is a minor DRY improvement rather than a pressing need.Also applies to: 1070-1076
🤖 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/config.rs` around lines 255 - 261, The typed non-zero vertex validation logic is duplicated in GenerateConfig::try_new and PachnerStressConfig::try_new, so extract a small shared helper around the NonZeroUsize::new(...).filter(...).ok_or(...) pattern. Make the helper generic over the threshold/predicate and accept an error-construction closure so each caller can still produce its own error type while reusing the same validation flow.src/delaunay/builder.rs (1)
1997-2020: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid retaining every explicit
Simplexbefore insertion.Line 1999 materializes a second full copy of the explicit connectivity just to run the precheck. Consider running the duplicate/facet precheck over
simplices.as_slice()plusindex_to_key, then constructing eachSimplexin the insertion loop. This keeps the prechecked path while reducing peak memory for large explicit imports.♻️ Sketch of the lower-allocation flow
- let mut explicit_simplices = Vec::with_capacity(simplices.len()); - for (simplex_idx, simplex_spec) in simplices.iter().enumerate() { - let vertex_keys: SimplexVertexKeyBuffer = - simplex_spec.iter().map(|&vi| index_to_key[vi]).collect(); - let simplex = Simplex::try_new(vertex_keys).map_err(|e| { - ExplicitConstructionError::SimplexCreation { - simplex_index: simplex_idx, - source: e, - } - })?; - explicit_simplices.push(simplex); - } - - Self::validate_explicit_simplices_for_prechecked_insert(&explicit_simplices).map_err( + Self::validate_explicit_simplices_for_prechecked_insert(simplices, &index_to_key).map_err( |source| ExplicitConstructionError::TdsAssembly { source: Box::new(source), }, )?; - // Insert simplices. - for simplex in explicit_simplices { + // Insert simplices. + for (simplex_idx, simplex_spec) in simplices.iter().enumerate() { + let vertex_keys: SimplexVertexKeyBuffer = + simplex_spec.iter().map(|&vi| index_to_key[vi]).collect(); + let simplex = Simplex::try_new(vertex_keys).map_err(|e| { + ExplicitConstructionError::SimplexCreation { + simplex_index: simplex_idx, + source: e, + } + })?; tds.insert_simplex_with_mapping_prechecked_topology(simplex) .map_err(|source| ExplicitConstructionError::TdsAssembly { source: Box::new(source),🤖 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 1997 - 2020, The explicit import path in the builder currently allocates and retains a full Vec of Simplex values before insertion, which increases peak memory. Update the logic around validate_explicit_simplices_for_prechecked_insert and tds.insert_simplex_with_mapping_prechecked_topology so the duplicate/facet precheck runs directly over simplices.as_slice() with index_to_key, then construct each Simplex only inside the insertion loop. Keep the existing ExplicitConstructionError handling and prechecked topology path, but remove the explicit_simplices collection.src/geometry/traits/coordinate.rs (1)
952-955: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo-op cleanup — explicit
Sizedbound was already redundant.Rust adds an implicit
Self: Sizedbound to trait declarations and generic parameters unless?Sizedis used, andDefault::default() -> Selfalready forces implementors to beSizedin practice. So removing the explicit+ Sizedhere doesn't change which types satisfyCoordinateRepresentation, or make the trait object-safe (the other supertraits likeDefaultstill precludedyn CoordinateRepresentation). Harmless, but worth knowing this isn't an actual bound relaxation.🤖 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/traits/coordinate.rs` around lines 952 - 955, The CoordinateRepresentation trait already has an implicit Sized requirement, so this change is only a cleanup and should not alter behavior. Keep the trait and blanket impl in coordinate.rs as Copy + Default + Debug + Serialize + DeserializeOwned, and make sure no surrounding code or comments imply that removing the explicit Sized bound changes trait object safety or accepted types.src/delaunay/serialization.rs (1)
162-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest bypasses public constructors via direct struct literal.
The test hand-builds
Triangulation/DelaunayTriangulationvia struct literals with a placeholderNotAKerneland an emptyTds, rather than going throughbuilder()/try_from_tds(). That's acceptable here since the goal is a compile-time proof thatSerializeno longer needs kernel/DataType bounds, and an empty TDS has no invariant to violate. Just flagging that this couples the test tightly to the current field layout ofTriangulation/DelaunayTriangulation; if those become further encapsulated (as is happening elsewhere in this PR for other types), this test will need to track that.🤖 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/serialization.rs` around lines 162 - 181, The serialization test in serialize_delaunay_triangulation_does_not_require_kernel_or_datatype_bounds is building Triangulation and DelaunayTriangulation with direct struct literals, which tightly couples it to private field layout. Update the test to use the public construction path (such as builder() or try_from_tds()) if it can still create the same empty-triangulation setup, while preserving the compile-time assertion that Serialize does not require kernel/DataType bounds; if no public constructor can express this case, keep the literal-based setup but make the dependency on internals explicit in the test name or nearby comment.
🤖 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.
Nitpick comments:
In @.github/workflows/papers.yml:
- Around line 55-59: The papers workflow has been switched to macOS, which
increases GitHub Actions runtime cost; review the job in papers.yml around the
papers job and either confirm this higher-cost runner is intentional or reduce
usage by narrowing when the job runs or moving it back to a Linux runner if
macOS is not required. Keep the existing Rust/uv/Tectonic setup intact while
making any runner or trigger adjustment.
In `@src/config.rs`:
- Around line 255-261: The typed non-zero vertex validation logic is duplicated
in GenerateConfig::try_new and PachnerStressConfig::try_new, so extract a small
shared helper around the NonZeroUsize::new(...).filter(...).ok_or(...) pattern.
Make the helper generic over the threshold/predicate and accept an
error-construction closure so each caller can still produce its own error type
while reusing the same validation flow.
In `@src/delaunay/builder.rs`:
- Around line 1997-2020: The explicit import path in the builder currently
allocates and retains a full Vec of Simplex values before insertion, which
increases peak memory. Update the logic around
validate_explicit_simplices_for_prechecked_insert and
tds.insert_simplex_with_mapping_prechecked_topology so the duplicate/facet
precheck runs directly over simplices.as_slice() with index_to_key, then
construct each Simplex only inside the insertion loop. Keep the existing
ExplicitConstructionError handling and prechecked topology path, but remove the
explicit_simplices collection.
In `@src/delaunay/repair.rs`:
- Around line 198-263: The builder methods in DelaunayRepairHeuristicConfig
should follow the same with_/without_ pattern used by DelaunayizeConfig for
consistency. Review the existing builder methods like with_shuffle_seed,
with_perturbation_seed, with_max_flips, and without_max_flips, and make their
naming/signature style match the corresponding methods in DelaunayizeConfig
without introducing extra abstraction unless it is already used there.
In `@src/delaunay/serialization.rs`:
- Around line 162-181: The serialization test in
serialize_delaunay_triangulation_does_not_require_kernel_or_datatype_bounds is
building Triangulation and DelaunayTriangulation with direct struct literals,
which tightly couples it to private field layout. Update the test to use the
public construction path (such as builder() or try_from_tds()) if it can still
create the same empty-triangulation setup, while preserving the compile-time
assertion that Serialize does not require kernel/DataType bounds; if no public
constructor can express this case, keep the literal-based setup but make the
dependency on internals explicit in the test name or nearby comment.
In `@src/geometry/traits/coordinate.rs`:
- Around line 952-955: The CoordinateRepresentation trait already has an
implicit Sized requirement, so this change is only a cleanup and should not
alter behavior. Keep the trait and blanket impl in coordinate.rs as Copy +
Default + Debug + Serialize + DeserializeOwned, and make sure no surrounding
code or comments imply that removing the explicit Sized bound changes trait
object safety or accepted types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: dd6ac686-562f-4309-a4e6-5878db2f64d8
📒 Files selected for processing (47)
.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/generate-baseline.yml.github/workflows/papers.yml.github/workflows/release-benchmarks.yml.github/workflows/semgrep-sarif.ymlREADME.mdbenches/ci_performance_suite.rsdocs/api_design.mddocs/dev/tooling-alignment.mdexamples/delaunayize_repair.rssrc/config.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/embedding.rssrc/core/insertion.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/locality.rssrc/delaunay/property_validation.rssrc/delaunay/repair.rssrc/delaunay/serialization.rssrc/geometry/embedding.rssrc/geometry/predicates.rssrc/geometry/traits/coordinate.rssrc/geometry/util/conversions.rssrc/geometry/util/measures.rssrc/geometry/util/point_generation.rssrc/lib.rstests/circumsphere_debug_tools.rstests/cli.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/large_scale_debug.rstests/mesh_export.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/proptest_sos.rstests/proptest_toroidal.rstests/proptest_triangulation.rstests/trait_bound_ergonomics.rs
💤 Files with no reviewable changes (2)
- tests/proptest_triangulation.rs
- tests/proptest_delaunay_triangulation.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #511 +/- ##
==========================================
+ Coverage 91.00% 91.02% +0.01%
==========================================
Files 88 88
Lines 79700 79782 +82
==========================================
+ Hits 72533 72620 +87
+ Misses 7167 7162 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
1 participant
BREAKING CHANGE: BistellarFlipKind::d and PeriodicSimplexSpan::{axis, span, period} are private; use d(), axis(), span(), and period() instead.
Closes #453