feat(validation)!: add embedded triangulation validation layer (#449)#481
Merged
Merged
Conversation
Add a Level 4 "embedding" validation layer that certifies a triangulation is a faithful geometric embedding independently of the Delaunay predicate: maximal simplices are nondegenerate and meet only in shared faces within the active affine chart. This renumbers the Delaunay empty-circumsphere property to Level 5. - Add a public geometry::embedding module (backed by internal core::embedding) with the embedding checks and their typed errors. - Add Triangulation::is_valid_embedding, validate_embedding, and embedding_report for the new Level 4 layer. - Move Delaunay property validation to delaunay::property_validation (renamed from core::util::delaunay_validation) and expose the Level 5 check as DelaunayTriangulation::is_valid_delaunay. - Refresh the validation guide, invariants, API-design, and prelude docs for the five-level stack. BREAKING CHANGE: The validation stack is renumbered from four to five levels. Level 4 is now faithful embedding validation (Triangulation::is_valid_embedding / validate_embedding / embedding_report), and the Delaunay empty-circumsphere property moves to Level 5 (DelaunayTriangulation::is_valid_delaunay); migrate callers of the former Level-4 Delaunay entry points (e.g. is_valid -> is_valid_delaunay). The Delaunay property-validation items DelaunayValidationError, find_delaunay_violations, DelaunayViolationDetail, DelaunayViolationReport, delaunay_violation_report, and debug_print_first_delaunay_violation now live in delaunay::property_validation (renamed from core::util::delaunay_validation); import them from the crate root or prelude. Closes #449
acgetchell
enabled auto-merge (squash)
June 27, 2026 04:55
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 (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThis PR expands validation to a five-level model, adds embedding-specific predicates and reports, splits topology and structure validation APIs, and updates Delaunay validation, exports, docs, examples, tests, and tooling to use the new layer names and flows. ChangesValidation hierarchy expansion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 477 |
🟢 Coverage 82.82% diff coverage · -0.63% coverage variation
Metric Results Coverage variation ✅ -0.63% coverage variation (-1.00%) Diff coverage ✅ 82.82% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (8ef6a95) 74123 67989 91.72% Head commit (0e0b3a8) 76459 (+2336) 69649 (+1660) 91.09% (-0.63%) 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 (#481) 2812 2329 82.82% 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
coderabbitai
Bot
added
api
breaking change
documentation
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/delaunay/deletion.rs (1)
699-711: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale level reference in test message. The
expect_errstring on line 701 still says "roll back a Level 4 violation", but the matched error is nowVerificationFailed, which this PR classifies as Level 5. Update the message to "Level 5 violation" to match the renumbering applied to the surrounding docs (lines 85/98/124).🤖 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/deletion.rs` around lines 699 - 711, Update the stale test expectation message in the delete_vertex error assertion: the expect_err text in the deletion test still refers to a “Level 4 violation,” but the matched VerificationFailed path is now classified as Level 5. Adjust the message string in the delete_vertex-related test so it matches the new Level 5 terminology and stays consistent with the renumbering used elsewhere in the Delaunay deletion code.src/delaunay/query.rs (1)
919-925: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new embedding failure mode on
try_set_global_topology().Line 923 broadens this setter to surface Level 4 embedding errors as well, but the
# Errorsdocs above still describe only TDS/Level-3 failures. Please update that contract too, otherwise callers will miss that a topology change can now be rejected for embedding reasons.🤖 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 919 - 925, Update the `try_set_global_topology()` contract to document the new embedding failure path surfaced through `self.tri.try_set_global_topology(...)`. The `# Errors` section in `src/delaunay/query.rs` still only mentions TDS/Level-3 failures, so add the `InvariantError::Embedding`/Level-4 topology rejection case alongside the existing `InvariantError::Tds`, `InvariantError::Triangulation`, and `InvariantError::Delaunay` mappings. Keep the docs aligned with the match in `try_set_global_topology()` so callers can see that a topology change may now fail for embedding reasons.
🧹 Nitpick comments (2)
tests/proptest_flips.rs (1)
170-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
assert_validno longer checks the new embedding layer.
is_valid_topology()plusTriangulation::validate()only covers Levels 1–3, so a flip can preserve topology while still creating a degenerate or overlapping embedding and these proptests will miss it. Addvalidate_embedding()here, or rename the helper so its narrower contract is explicit. As per coding guidelines, tests should verify mathematical, geometric, and topological invariants.♻️ Minimal change
fn assert_valid<const D: usize>( triangulation: &Triangulation<AdaptiveKernel<f64>, (), (), D>, context: &str, ) -> Result<(), TestCaseError> { triangulation .is_valid_topology() .map_err(|err| TestCaseError::fail(format!("{context} invariant check failed: {err:?}")))?; + triangulation + .validate_embedding() + .map_err(|err| TestCaseError::fail(format!("{context} embedding validation failed: {err:?}")))?; triangulation .validate() .map_err(|err| TestCaseError::fail(format!("{context} validation failed: {err:?}")))?; Ok(()) }🤖 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_flips.rs` around lines 170 - 175, `assert_valid` is only checking topology and core triangulation validation, so it can miss embedding regressions after flips. Update the helper in `tests/proptest_flips.rs` to also call `Triangulation::validate_embedding()` alongside `is_valid_topology()` and `validate()`, and keep the error mapping consistent with the existing `context`/`TestCaseError::fail` pattern so the proptests cover the embedding invariants too.Source: Coding guidelines
benches/profiling_suite.rs (1)
1065-1077: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a dedicated
validate_embeddingbenchmark.The component suite now isolates Level 3 and Level 5, but the new Level 4 validator is only measured indirectly through
validate(). That makes it hard to attribute regressions in the embedding layer or explain any new gap betweenis_valid_topologyand full validation.🤖 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 `@benches/profiling_suite.rs` around lines 1065 - 1077, Add a dedicated validate_embedding benchmark in profiling_suite::profiling_suite’s benchmark group so Level 4 is measured directly instead of only through validate(). Mirror the existing pattern used by is_valid_topology and is_valid_delaunay: call the embedding validator on the benchmark triangulation, wrap it in black_box, and abort_benchmark on unexpected errors so regressions in the embedding layer can be isolated.
🤖 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 `@benches/common/flip_workflows.rs`:
- Around line 325-333: The current validate_topology_and_delaunay() path is
weaker than the replaced trial.validate() flow because it skips Level 4 and
collapses all Triangulation::validate() failures into
InvalidTopologyAfterRoundtrip. Update the helper to use the full
DelaunayTriangulation::validate() path and split errors by their typed source,
or add a dedicated embedding-validation branch with a separate error variant;
keep the validation logic at the lowest layer owned by the invariant and avoid
stringifying lower-layer diagnostics in validate_topology_and_delaunay() and
InvalidTopologyAfterRoundtrip.
In `@docs/invariants.md`:
- Around line 131-133: Update the wording in the documentation section about
Automatic validation during construction so it distinguishes local embedding
validation from full/global certification: make clear that ValidationPolicy can
trigger insertion-time local embedding checks, while only the full/global Level
4 certification remains explicit; use the surrounding text in the invariant
description to adjust the phrasing without changing the intended separation
between Level 3 insertion checks and later certification steps.
In `@docs/ORIENTATION_SPEC.md`:
- Around line 203-205: Update the `.try_toroidal([..])` documentation bullet to
explicitly mention Level 4 embedding validation, since the current wording skips
from Level 3 to Level 5 and implies it is omitted. Clarify in the
`ORIENTATION_SPEC.md` description whether the toroidal path runs the Level 4
checkpoint as part of the `try_toroidal([..])` flow, or state clearly that this
path is exempt; keep the wording aligned with the existing validation sequence
described around `.try_toroidal([..])`.
In `@src/core/repair.rs`:
- Around line 857-860: The debug/test-only validation block in
`Repair::delete_vertex` no longer checks the Level 4 embedding postcondition, so
test builds can miss degenerate or overlapping simplex regressions. Keep the
existing `self.tds.is_valid()` and `self.is_valid_topology()` checks, and also
restore the embedding invariant check by calling
`Triangulation::is_valid_embedding` (or the same validation path used by
`validate_embedding`) inside this `#[cfg(debug_assertions)]` block so mutating
operations still enforce all required invariants.
In `@src/core/validation.rs`:
- Line 149: The adapter round-trip in validation loses the original embedding
failure because InvariantError::Embedding is encoded into
InsertionError::DelaunayValidationFailed and then decoded in the reverse match
as InvariantError::Delaunay. Update the conversion logic around the
InsertionError-to-InvariantError mapping and the InvariantError encoding path so
embedding failures keep a distinct insertion variant or otherwise preserve the
original layer in the payload, and make sure the round-trip restores
InvariantError::Embedding instead of collapsing everything into Delaunay.
- Around line 1336-1341: The topology reporting path is now inconsistent with
the core topology predicate: `topology_report()` and `topology_diagnostic()`
should not run `validate_at_completion()`, since `is_valid_topology()`
intentionally stops earlier and accepts in-progress `PLManifold` triangulations.
Remove the completion-time check from `topology_report()` in
`src/core/validation.rs`, and keep `validate_at_completion()` only in the full
`validate()`/`validation_report()` flow so the Level 3-only API stays aligned
with `is_valid_topology()`.
In `@src/delaunay/property_validation.rs`:
- Line 1081: The clippy failure comes from the `std::ptr::eq` assertion in the
`property_validation` test using an implicit reference-to-raw-pointer coercion.
Update that check to make both sides’ pointer conversions explicit around
`detail` and `report.violation_details[0]`, keeping the assertion equivalent but
avoiding the implicit borrow in the comparison.
In `@src/delaunay/validation.rs`:
- Around line 1199-1204: Clippy is failing on the
DelaunayValidationError::DelaunayViolation construction because the boxed fields
use Default::default(); update the source creation in
DelaunayVerificationError::from(...) to use Box::default() for simplex_vertices
and neighbor_simplices so the lint is satisfied and CI passes.
In `@src/geometry/embedding.rs`:
- Around line 23-65: The LabeledSimplexEmbedding::try_new constructor currently
validates length and finiteness but still allows repeated labels, which breaks
the unique-vertex invariant used by overlap logic. Add a duplicate-label check
in try_new after collecting labels, return a new typed
LabeledSimplexEmbeddingError for duplicates, and ensure the validation happens
before constructing Self so all callers enforce the invariant at the lowest
layer.
In `@tests/benchmark_flip_fixtures.rs`:
- Around line 404-410: The benchmark fixture helper
`assert_topology_and_delaunay_valid` is missing the new embedding invariant
check. Update this helper so it also verifies the embedding layer using
`Triangulation::is_valid_embedding` or `validate_embedding` in addition to
`dt.as_triangulation().validate()` and `dt.is_valid_delaunay()`, and make the
panic message name the embedding level alongside the existing topology and
Delaunay checks.
In `@tests/delaunay_edge_cases.rs`:
- Around line 780-782: Replace the string-based assertion in the cube-corner
test with a typed match on the nested construction error variant so it checks
the actual error contract instead of `Debug` output. In the
`delaunay_edge_cases` test around the `err` assertion, destructure the error
returned by the construction path and verify it is the `DegenerateSimplex`
variant directly, using the relevant error enums/variants rather than
`format!("{err:?}")`.
In `@tests/pachner_roundtrip.rs`:
- Around line 44-59: The `topology_and_delaunay_valid` and
`assert_topology_and_delaunay_valid` helpers only check `validate()` and
`is_valid_delaunay()`, so they miss Level 4 embedding invariants. Update these
helpers to also verify the embedding via `Triangulation::is_valid_embedding` or
`validate_embedding` on `dt.as_triangulation()`, and include that check in both
the boolean helper and the panic-based assertion path so roundtrip tests fail on
degenerate or overlapping simplices.
In `@tests/proptest_delaunay_triangulation.rs`:
- Around line 1357-1360: Update the TODO comment in
proptest_delaunay_triangulation so the wording matches the current five-level
scheme: the commented checks in this block use is_valid_delaunay(), so the note
should no longer say “Level-4 checks.” Keep the comment aligned with the
surrounding Level-5 terminology near the Delaunay triangulation property tests
and the dt_a/dt_b validation placeholders.
- Around line 2040-2042: Update the stale inline comment near the
dt.is_valid_delaunay() call in proptest_delaunay_triangulation so the level
label matches the renamed validation level; change the “Level 4” reference to
“Level 5” to stay consistent with the current Delaunay validity naming used by
is_valid_delaunay().
In `@tests/regressions.rs`:
- Around line 194-197: After calling repair_delaunay_with_flips() in the
regression test, also assert the lower-layer mesh invariants before or alongside
is_valid_delaunay(), so the test verifies topology/embedding remain valid in
addition to the Delaunay property. Update the test around
dt.repair_delaunay_with_flips() to check the existing validation method for the
underlying structure on the dt object, keeping the current is_valid_delaunay()
assertion as the higher-level check.
In `@tests/semgrep/src/project_rules/rust_style.rs`:
- Around line 144-179: Add a validate_embedding allow-case to the
ValidationApiNamingFixture so the semgrep fixture covers the new public
spelling. Update the existing fixture in ValidationApiNamingFixture alongside
validate, validation_report, and embedding_report to include a
validate_embedding method marked as ok for
delaunay.rust.validation-api-naming-standard, ensuring the rule explicitly
allows this naming pattern.
---
Outside diff comments:
In `@src/delaunay/deletion.rs`:
- Around line 699-711: Update the stale test expectation message in the
delete_vertex error assertion: the expect_err text in the deletion test still
refers to a “Level 4 violation,” but the matched VerificationFailed path is now
classified as Level 5. Adjust the message string in the delete_vertex-related
test so it matches the new Level 5 terminology and stays consistent with the
renumbering used elsewhere in the Delaunay deletion code.
In `@src/delaunay/query.rs`:
- Around line 919-925: Update the `try_set_global_topology()` contract to
document the new embedding failure path surfaced through
`self.tri.try_set_global_topology(...)`. The `# Errors` section in
`src/delaunay/query.rs` still only mentions TDS/Level-3 failures, so add the
`InvariantError::Embedding`/Level-4 topology rejection case alongside the
existing `InvariantError::Tds`, `InvariantError::Triangulation`, and
`InvariantError::Delaunay` mappings. Keep the docs aligned with the match in
`try_set_global_topology()` so callers can see that a topology change may now
fail for embedding reasons.
---
Nitpick comments:
In `@benches/profiling_suite.rs`:
- Around line 1065-1077: Add a dedicated validate_embedding benchmark in
profiling_suite::profiling_suite’s benchmark group so Level 4 is measured
directly instead of only through validate(). Mirror the existing pattern used by
is_valid_topology and is_valid_delaunay: call the embedding validator on the
benchmark triangulation, wrap it in black_box, and abort_benchmark on unexpected
errors so regressions in the embedding layer can be isolated.
In `@tests/proptest_flips.rs`:
- Around line 170-175: `assert_valid` is only checking topology and core
triangulation validation, so it can miss embedding regressions after flips.
Update the helper in `tests/proptest_flips.rs` to also call
`Triangulation::validate_embedding()` alongside `is_valid_topology()` and
`validate()`, and keep the error mapping consistent with the existing
`context`/`TestCaseError::fail` pattern so the proptests cover the embedding
invariants too.
🪄 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: 21858286-2a77-4b74-a58f-356d9e945094
📒 Files selected for processing (71)
AGENTS.mdCITATION.cffREADME.mdbenches/ci_performance_suite.rsbenches/common/flip_fixtures.rsbenches/common/flip_workflows.rsbenches/profiling_suite.rsdocs/ORIENTATION_SPEC.mddocs/README.mddocs/api_design.mddocs/architecture/module_map.mddocs/architecture/prelude_reference.mddocs/code_organization.mddocs/dev/docs.mddocs/dev/testing.mddocs/dev/tooling-alignment.mddocs/diagnostics.mddocs/invariants.mddocs/limitations.mddocs/numerical_robustness_guide.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/topology_editing.rsscripts/tests/test_readme_citation_mirror.pysemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/construction.rssrc/core/embedding.rssrc/core/insertion.rssrc/core/orientation.rssrc/core/repair.rssrc/core/simplex.rssrc/core/tds/errors.rssrc/core/tds/storage.rssrc/core/tds/validation.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/deletion.rssrc/delaunay/insertion.rssrc/delaunay/property_validation.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/serialization.rssrc/delaunay/triangulation.rssrc/delaunay/validation.rssrc/geometry/embedding.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/traits/global_topology_model.rssrc/topology/traits/topological_space.rstests/benchmark_flip_fixtures.rstests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_repair_fallback.rstests/euler_characteristic.rstests/large_scale_debug.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/proptest_flips.rstests/proptest_serialization.rstests/regressions.rstests/semgrep/docs/validation_levels.mdtests/semgrep/src/project_rules/rust_style.rstests/triangulation_builder.rs
- Add Level 4 embedding reports for degenerate simplices, duplicate simplex labels, illegal simplex intersections, and periodic chart-span failures.
- Expose pure labeled-simplex embedding predicates through geometry and focused geometry preludes.
- Run changed-scope embedding guards during insertion and include embedding validation in cumulative triangulation, repair, and Delaunay validation before Level 5 checks.
- Document the validation hierarchy, embedding references, benchmark coverage, and local Clippy parity with GitHub code scanning.
BREAKING CHANGE: cumulative validation, insertion, repair, and topology-change APIs can now fail with Level 4 embedding errors before Delaunay-property checks; callers matching validation, construction, or insertion errors must handle the new embedding variants. LabeledSimplexEmbedding::try_new now requires Eq labels and rejects duplicates, and SimplexIntersectionFailure is non-exhaustive with IntersectionOutsideSharedFace { witness } instead of the previous tuple variant.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/validation.rs (1)
1263-1265: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the report coverage docs.
validation_report()is documented and implemented as cumulative Levels 1–3, so pointing callers to it for Levels 1–4 can make them skipembedding_report()/Level 4 checks.📝 Proposed fix
- /// structure is already valid; use [`validation_report`](Self::validation_report) - /// for cumulative Levels 1-4 diagnostics. + /// structure is already valid; use [`validation_report`](Self::validation_report) + /// for cumulative Levels 1-3 diagnostics, and + /// [`embedding_report`](Self::embedding_report) for Level 4 diagnostics.🤖 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 1263 - 1265, The doc comment for the topology-layer report is inaccurate about coverage: it should not direct callers to validation_report() for cumulative Levels 1–4, since validation_report() only covers cumulative Levels 1–3 and Level 4 lives in embedding_report(). Update the documentation near the report method(s) in validation.rs so the referenced symbols and level ranges match the actual implementation and callers are guided to run embedding_report() for Level 4 checks.
🧹 Nitpick comments (1)
src/geometry/point.rs (1)
49-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a debug assertion for finite coordinates
from_prevalidated_finite_valuesonly canonicalizes signed zero; it does not enforce the finiteness contract it relies on. Adebug_assert!here would catch any future caller that hands itNaN/infwithout affecting release builds.Proposed guard
pub(in crate::geometry) fn from_prevalidated_finite_values(mut values: [f64; D]) -> Self { + debug_assert!(values.iter().all(|coord| coord.is_finite())); for coord in &mut values { if *coord == 0.0 { *coord = 0.0; } }🤖 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/point.rs` around lines 49 - 58, `from_prevalidated_finite_values` currently only normalizes signed zero and does not enforce its finite-input contract, so add a `debug_assert!` inside this function to verify every value in `values` is finite before constructing `Self`. Keep the existing zero-canonicalization behavior intact, and use the `from_prevalidated_finite_values` symbol to place the guard so debug builds catch any future `NaN` or `inf` callers without changing release behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/algorithms/flips.rs`:
- Line 42: The flip error path is collapsing detailed lower-layer diagnostics
into only TriangulationEmbeddingValidationErrorKind, which loses the
simplex/pair witness data from TriangulationEmbeddingValidationError. Update
EmbeddingValidation and the conversion sites in flips.rs so the public error
preserves the full underlying error (including its witness context) instead of
downgrading it to a classification enum. Use the existing EmbeddingValidation
and TriangulationEmbeddingValidationError symbols to trace the conversion logic
and keep the higher-level error debuggable.
In `@src/core/validation.rs`:
- Around line 1797-1804: In InsertionValidationWork::FullValidation, do not
treat Some([]) as an automatic success in validation_after_insertion_work
handling, because that bypasses Level 4 embedding checks when a triangulation
exists. Update the match so empty local_simplices still falls through to the
embedding validation path (like validate_embedding_for_simplices or
is_valid_embedding), and keep the existing InvariantError::Embedding mapping so
full validation under ValidationPolicy::Always enforces all invariants.
---
Outside diff comments:
In `@src/core/validation.rs`:
- Around line 1263-1265: The doc comment for the topology-layer report is
inaccurate about coverage: it should not direct callers to validation_report()
for cumulative Levels 1–4, since validation_report() only covers cumulative
Levels 1–3 and Level 4 lives in embedding_report(). Update the documentation
near the report method(s) in validation.rs so the referenced symbols and level
ranges match the actual implementation and callers are guided to run
embedding_report() for Level 4 checks.
---
Nitpick comments:
In `@src/geometry/point.rs`:
- Around line 49-58: `from_prevalidated_finite_values` currently only normalizes
signed zero and does not enforce its finite-input contract, so add a
`debug_assert!` inside this function to verify every value in `values` is finite
before constructing `Self`. Keep the existing zero-canonicalization behavior
intact, and use the `from_prevalidated_finite_values` symbol to place the guard
so debug builds catch any future `NaN` or `inf` callers without changing release
behavior.
🪄 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: 5b9aa8bd-42cf-43b1-91ea-007c425ff554
📒 Files selected for processing (39)
REFERENCES.mdbenches/common/flip_fixtures.rsbenches/common/flip_workflows.rsbenches/profiling_suite.rsdocs/ORIENTATION_SPEC.mddocs/api_design.mddocs/architecture/module_map.mddocs/architecture/prelude_reference.mddocs/dev/commands.mddocs/dev/debug_env_vars.mddocs/invariants.mddocs/validation.mddocs/workflows.mdjustfilesrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/construction.rssrc/core/embedding.rssrc/core/insertion.rssrc/core/repair.rssrc/core/validation.rssrc/delaunay/construction.rssrc/delaunay/deletion.rssrc/delaunay/property_validation.rssrc/delaunay/query.rssrc/delaunay/validation.rssrc/geometry/embedding.rssrc/geometry/point.rssrc/lib.rstests/README.mdtests/benchmark_flip_fixtures.rstests/delaunay_edge_cases.rstests/large_scale_debug.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/proptest_flips.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rs
✅ Files skipped from review due to trivial changes (6)
- tests/README.md
- docs/dev/debug_env_vars.md
- docs/ORIENTATION_SPEC.md
- docs/architecture/prelude_reference.md
- docs/workflows.md
- REFERENCES.md
🚧 Files skipped from review as they are similar to previous changes (17)
- docs/architecture/module_map.md
- tests/regressions.rs
- tests/semgrep/src/project_rules/rust_style.rs
- benches/profiling_suite.rs
- src/delaunay/query.rs
- docs/api_design.md
- src/core/construction.rs
- src/core/repair.rs
- src/delaunay/deletion.rs
- tests/proptest_delaunay_triangulation.rs
- docs/invariants.md
- benches/common/flip_fixtures.rs
- src/geometry/embedding.rs
- src/core/embedding.rs
- src/lib.rs
- src/delaunay/validation.rs
- src/delaunay/property_validation.rs
- Preserve full embedding-validation errors through flip neighbor wiring so callers retain simplex, pair, and offending-vertex witness context. - Run Level 4 embedding validation during full post-insertion validation even when the local simplex scope is empty. - Clarify topology versus embedding report coverage and guard trusted finite-coordinate construction in debug builds. - Tighten regression coverage for SoS degeneracy, stale Pachner handles, toroidal validation guardrails, and flip benchmark fixtures. BREAKING CHANGE: FlipNeighborWiringError::EmbeddingValidation now stores the full TriangulationEmbeddingValidationError as source instead of a TriangulationEmbeddingValidationErrorKind classification.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #481 +/- ##
==========================================
- Coverage 91.70% 91.07% -0.64%
==========================================
Files 84 86 +2
Lines 73902 76238 +2336
==========================================
+ Hits 67770 69430 +1660
- Misses 6132 6808 +676
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
This was referenced Jul 5, 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
2 participants
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.
Add a Level 4 "embedding" validation layer that certifies a triangulation is a faithful geometric embedding independently of the Delaunay predicate: maximal simplices are nondegenerate and meet only in shared faces within the active affine chart. This renumbers the Delaunay empty-circumsphere property to Level 5.
BREAKING CHANGE: The validation stack is renumbered from four to five levels. Level 4 is now faithful embedding validation (Triangulation::is_valid_embedding / validate_embedding / embedding_report), and the Delaunay empty-circumsphere property moves to Level 5 (DelaunayTriangulation::is_valid_delaunay); migrate callers of the former Level-4 Delaunay entry points (e.g. is_valid -> is_valid_delaunay). The Delaunay property-validation items DelaunayValidationError, find_delaunay_violations, DelaunayViolationDetail, DelaunayViolationReport, delaunay_violation_report, and debug_print_first_delaunay_violation now live in delaunay::property_validation (renamed from core::util::delaunay_validation); import them from the crate root or prelude.
Closes #449