refactor(validation)!: reconcile topology validation policy (#385)#404
Merged
Conversation
- Add explicit caller-owned validation mode for PL-manifold topology guarantees. - Reject incoherent topology guarantee and validation policy pairings through typed fallible setters. - Keep compatibility setters non-committal when a requested pairing is invalid. - Derive builder validation policy from the selected topology guarantee and document the compatibility matrix. BREAKING CHANGE: `ValidationPolicy::Never` is now only coherent with `TopologyGuarantee::Pseudomanifold`; PL-manifold callers should use `ValidationPolicy::ExplicitOnly` for manual full-validation checkpoints.
Contributor
WalkthroughThis PR resolves the overlapping ChangesValidationPolicy Extension and Typed Configuration API
DelaunayTriangulation Public Wrapper and Builder Integration
Public Exports, Documentation, and Examples
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 | 0 |
🟢 Coverage 100.00% diff coverage · +0.02% coverage variation
Metric Results Coverage variation ✅ +0.02% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (bc5d48f) 62304 56378 90.49% Head commit (910530e) 62378 (+74) 56460 (+82) 90.51% (+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 (#404) 124 124 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/delaunay/construction.rs (1)
6274-6300: ⚡ Quick winAdd the
PLManifold → ExplicitOnlycase to this derivation test.This only exercises the strict and pseudomanifold ends of the matrix, so a regression in the new breaking-change path for
TopologyGuarantee::PLManifoldwould slip through here.As per coding guidelines, "Unit tests in Rust must cover known values, error paths, and dimension-generic correctness."➕ Suggested test addition
let strict: DelaunayTriangulation<_, (), (), 3> = DelaunayTriangulation::empty_with_topology_guarantee( TopologyGuarantee::PLManifoldStrict, ); assert_eq!( strict.topology_guarantee(), TopologyGuarantee::PLManifoldStrict ); assert_eq!(strict.validation_policy(), ValidationPolicy::Always); + let manifold: DelaunayTriangulation<_, (), (), 3> = + DelaunayTriangulation::empty_with_topology_guarantee( + TopologyGuarantee::PLManifold, + ); + assert_eq!(manifold.topology_guarantee(), TopologyGuarantee::PLManifold); + assert_eq!(manifold.validation_policy(), ValidationPolicy::ExplicitOnly); + let pseudomanifold: DelaunayTriangulation<_, (), (), 3> = DelaunayTriangulation::with_empty_kernel_and_topology_guarantee( FastKernel::<f64>::new(), TopologyGuarantee::Pseudomanifold, );🤖 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 6274 - 6300, Add a third case in test_empty_topology_guarantee_derives_validation_policy that constructs a triangulation with TopologyGuarantee::PLManifold (use DelaunayTriangulation::with_empty_kernel_and_topology_guarantee(FastKernel::<f64>::new(), TopologyGuarantee::PLManifold)), then assert that .topology_guarantee() is TopologyGuarantee::PLManifold and that .validation_policy() equals ValidationPolicy::ExplicitOnly so the PLManifold → ExplicitOnly derivation is covered.src/core/validation.rs (1)
1913-2056: ⚡ Quick winParameterize the new configuration-path tests across D=2..5.
The new
try_set_*/compatibility coverage is mostly 2D-only even though this API is const-generic and this file already usespasteyfor dimension fan-out. Please mirror that here so the success and rejection paths are exercised for D=2 through D=5 as well.As per coding guidelines,
Unit tests in Rust must cover known values, error paths, and dimension-generic correctness. Dimension-generic tests must cover D=2 through D=5 whenever feasible; use pastey macros to generate per-dimension tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/validation.rs` around lines 1913 - 2056, The new configuration-path tests (functions like incompatible_policy_rejected_even_when_completion_validation_succeeds, incompatible_guarantee_rejected_even_when_completion_validation_succeeds, incompatible_policy_rejected_when_completion_validation_fails, incompatible_guarantee_rejected_when_completion_validation_fails, etc.) are written only for D=2; update them to be generated for D=2..=5 using the existing pastey test expansion pattern so the const-generic Triangulation<..., D> coverage exercises try_set_validation_policy, try_set_topology_guarantee, validate_at_completion and set_* variants across dimensions 2–5; replace each lone #[test] with a pastey macro (or wrap the test body in a pastey-generated per-dimension test) that instantiates Triangulation::<FastKernel<f64>, (), (), D> for D=2..=5 to assert both success and rejection paths.
🤖 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 `@src/core/validation.rs`:
- Around line 1913-2056: The new configuration-path tests (functions like
incompatible_policy_rejected_even_when_completion_validation_succeeds,
incompatible_guarantee_rejected_even_when_completion_validation_succeeds,
incompatible_policy_rejected_when_completion_validation_fails,
incompatible_guarantee_rejected_when_completion_validation_fails, etc.) are
written only for D=2; update them to be generated for D=2..=5 using the existing
pastey test expansion pattern so the const-generic Triangulation<..., D>
coverage exercises try_set_validation_policy, try_set_topology_guarantee,
validate_at_completion and set_* variants across dimensions 2–5; replace each
lone #[test] with a pastey macro (or wrap the test body in a pastey-generated
per-dimension test) that instantiates Triangulation::<FastKernel<f64>, (), (),
D> for D=2..=5 to assert both success and rejection paths.
In `@src/delaunay/construction.rs`:
- Around line 6274-6300: Add a third case in
test_empty_topology_guarantee_derives_validation_policy that constructs a
triangulation with TopologyGuarantee::PLManifold (use
DelaunayTriangulation::with_empty_kernel_and_topology_guarantee(FastKernel::<f64>::new(),
TopologyGuarantee::PLManifold)), then assert that .topology_guarantee() is
TopologyGuarantee::PLManifold and that .validation_policy() equals
ValidationPolicy::ExplicitOnly so the PLManifold → ExplicitOnly derivation is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 78a7e666-4b73-45ee-84cf-157f1ab4192f
📒 Files selected for processing (13)
README.mddocs/api_design.mddocs/production_review_remediation_checklist.mddocs/validation.mddocs/workflows.mdsrc/core/validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/query.rssrc/lib.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/triangulation_builder.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
+ Coverage 90.47% 90.50% +0.02%
==========================================
Files 71 71
Lines 62100 62174 +74
==========================================
+ Hits 56188 56270 +82
+ Misses 5912 5904 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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:
ValidationPolicy::Neveris now only coherent withTopologyGuarantee::Pseudomanifold; PL-manifold callers should useValidationPolicy::ExplicitOnlyfor manual full-validation checkpoints.Closes #385