Refactor/445 realization validation#515
Draft
acgetchell wants to merge 2 commits into
Draft
Conversation
- Rename Level 4 validation APIs, diagnostics, reports, errors, and helper modules to use realization terminology. - Align Euclidean, toroidal, and spherical validation paths around Valid Realization before Level 5 geometric predicates. - Refresh validation docs, references, notebook figures, and paper artifacts to distinguish realization validity from Delaunay optimality. BREAKING CHANGE: Level 4 embedding APIs and types were removed in favor of realization names. Callers must migrate from `is_valid_embedding`, `validate_embedding`, `embedding_report`, and `TriangulationEmbeddingValidationError` to the corresponding realization APIs and types.
- Point validation Semgrep rules at the realization module after the Level 4 terminology rename. - Align uv and ty tool-version references with the current reviewed toolchain. - Make optional benchmark comparison reports use the existing atomic writer. - Clarify convex hull docs around stale detached hull snapshots.
Contributor
WalkthroughRenames "embedding validity" to "valid realization" across Level 4 validation throughout the codebase: core validation types, error enums, geometry predicates, delaunay/spherical wiring, public API/prelude exports, topology docs, tests, documentation, a validation notebook, a LaTeX paper, and bumps pinned uv/ty tooling versions in CI workflows. ChangesEmbedding to Realization Rename
CI Tooling Version Bumps
Validation Notebook and Paper Figures
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 296 |
🟢 Coverage 80.83% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 80.83% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (ff74d7c) 80105 72941 91.06% Head commit (ee5131c) 80114 (+9) 72947 (+6) 91.05% (0.00%) 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 (#515) 433 350 80.83% 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
documentation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #515 +/- ##
==========================================
- Coverage 91.03% 91.03% -0.01%
==========================================
Files 88 88
Lines 79884 79893 +9
==========================================
+ Hits 72722 72728 +6
- Misses 7162 7165 +3
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: 3
🤖 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 `@docs/invariants.md`:
- Around line 119-138: The Level 4 description is still attributing realization
validation to Euclidean/toroidal topology instead of the validator API. Update
the wording in this invariants section to make
`Triangulation::is_valid_realization()` and `validate_realization()` the
explicit Level 4 owners, while keeping `is_valid_topology()` as Level 3 and
`DelaunayTriangulation::is_valid_delaunay()` as Level 5. Rephrase the
surrounding text so the ownership model matches the actual API surface and
avoids implying topology performs Level 4 validation.
In `@docs/workflows.md`:
- Around line 440-441: The deletion description contains a duplicated “Level 4”
phrase in the workflow text. Update the sentence in the deletion/repair section
so it reads naturally by removing the repeated “Level 4” before “realization
validation,” while keeping the rest of the validation and rollback behavior
wording unchanged.
In `@papers/validation.tex`:
- Around line 74-79: Restore this `papers/validation.tex` block to an
author-owned placeholder instead of finished prose: the `\caption{...}` and
`\Description{...}` in the validation hierarchy figure should be wrapped back
into `\AuthorTodo{}` like the other prose blocks in this file. Keep the
structure in place, but remove the publication-ready caption/alt text and match
the file’s existing pattern so the author can supply the final wording later.
🪄 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: ab721cdd-7e01-4868-a488-21891d1fed5a
⛔ Files ignored due to path filters (8)
papers/generated/validation_hierarchy.pngis excluded by!**/*.png,!**/generated/**papers/generated/validation_level_1_element_validity.pngis excluded by!**/*.png,!**/generated/**papers/generated/validation_level_2_combinatorial_consistency.pngis excluded by!**/*.png,!**/generated/**papers/generated/validation_level_3_intrinsic_pl_topology.pngis excluded by!**/*.png,!**/generated/**papers/generated/validation_level_4_valid_realization.pngis excluded by!**/*.png,!**/generated/**papers/generated/validation_level_5_geometric_predicates.pngis excluded by!**/*.png,!**/generated/**papers/generated/validation_model_failures.pngis excluded by!**/*.png,!**/generated/**papers/validation.pdfis excluded by!**/*.pdf
📒 Files selected for processing (74)
.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.ymlAGENTS.mdCITATION.cffREADME.mdREFERENCES.mdbenches/README.mdbenches/common/flip_fixtures.rsbenches/profiling_suite.rsdocs/ORIENTATION_SPEC.mddocs/api_design.mddocs/architecture/module_map.mddocs/architecture/prelude_reference.mddocs/code_organization.mddocs/dev/commands.mddocs/dev/perf-tuning.mddocs/dev/tooling-alignment.mddocs/diagnostics.mddocs/invariants.mddocs/limitations.mddocs/topology.mddocs/validation.mddocs/workflows.mdnotebooks/01_validation.ipynbpapers/validation.texscripts/benchmark_utils.pysemgrep.yamlsrc/config.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/construction.rssrc/core/facet.rssrc/core/insertion.rssrc/core/realization.rssrc/core/repair.rssrc/core/tds/errors.rssrc/core/tds/mutation.rssrc/core/tds/rollback.rssrc/core/tds/storage.rssrc/core/tds/validation.rssrc/core/validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/flips.rssrc/delaunay/pachner.rssrc/delaunay/query.rssrc/delaunay/spherical.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/realization.rssrc/geometry/util/measures.rssrc/geometry/util/point_generation.rssrc/lib.rssrc/topology/spaces/euclidean.rssrc/topology/spaces/spherical.rssrc/topology/traits/global_topology_model.rssrc/topology/traits/topological_space.rstests/benchmark_flip_fixtures.rstests/cli.rstests/delaunay_edge_cases.rstests/euler_characteristic.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.rstests/spherical_delaunay.rstests/triangulation_builder.rs
Comment on lines
+119
to
+138
| 4. **Level 4 — Valid Realization**: the complex is geometrically valid in the chosen coordinate | ||
| model. Euclidean/toroidal topology validates affine-chart realizations, with toroidal checks | ||
| lifted to periodic covering-space charts; the bounded spherical prototype validates simplices on | ||
| `S^D \subset R^(D+1)`. | ||
| 5. **Level 5 — Geometric Predicates**: the embedding satisfies the selected geometry-specific | ||
| predicate family. The implemented family today is Delaunay, with Euclidean/toroidal | ||
| empty-circumsphere predicates and spherical empty-cap / ambient-hull-facet predicates. | ||
| 5. **Level 5 — Geometric Predicates**: the valid realization satisfies the selected | ||
| geometry-specific predicate family. The implemented family today is Delaunay, with | ||
| Euclidean/toroidal empty-circumsphere predicates and spherical empty-cap / ambient-hull-facet | ||
| predicates. | ||
|
|
||
| `Triangulation::is_valid_topology()` is a Level 3 intrinsic PL-topology check. | ||
| `DelaunayTriangulation::is_valid_delaunay()` is the implemented Level 5 geometric-predicate check | ||
| for an already-formed Delaunay triangulation. `Triangulation` also exposes Level 4 embedding | ||
| validation through `is_valid_embedding` / | ||
| `validate_embedding`. | ||
| for an already-formed Delaunay triangulation. `Triangulation` also exposes Level 4 realization | ||
| validation through `is_valid_realization` / | ||
| `validate_realization`. | ||
| Cumulative validation is exposed through the `validate` / `validation_report` APIs described in | ||
| [`docs/validation.md`](validation.md). | ||
|
|
||
| Automatic validation during construction is intentionally topology-oriented: `ValidationPolicy` | ||
| controls Level 3 checks during insertion and can enable local insertion-time embedding checks. | ||
| Full/global Level 4 embedding certification and Level 5 geometric-predicate validation remain | ||
| controls Level 3 checks during insertion and can enable local insertion-time realization checks. | ||
| Full/global Level 4 realization certification and Level 5 geometric-predicate validation remain |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep Level 4 ownership on the validator API.
This section still frames Level 4 as something “Euclidean/toroidal topology validates,” but the actual Level 4 surface is Triangulation::is_valid_realization() / validate_realization() in src/core/realization.rs. Rewording this around the validator would avoid a stale owner-model split.
🤖 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 `@docs/invariants.md` around lines 119 - 138, The Level 4 description is still
attributing realization validation to Euclidean/toroidal topology instead of the
validator API. Update the wording in this invariants section to make
`Triangulation::is_valid_realization()` and `validate_realization()` the
explicit Level 4 owners, while keeping `is_valid_topology()` as Level 3 and
`DelaunayTriangulation::is_valid_delaunay()` as Level 5. Rephrase the
surrounding text so the ownership model matches the actual API surface and
avoids implying topology performs Level 4 validation.
Comment on lines
440
to
+441
| `DelaunayRepairPolicy` allows it. If automatic repair is disabled, deletion still runs Level 4 | ||
| Embedding Validity and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or | ||
| Level 4 realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicated "Level 4" in the deletion description.
The sentence reads "deletion still runs Level 4 Level 4 realization validation and the Level 5 Delaunay predicate..." — the word "Level 4" is repeated.
✏️ Proposed fix
-Level 4 realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or
+Level 4 realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, orConcretely, remove the duplicate "Level 4" so the sentence reads:
-`DelaunayRepairPolicy` allows it. If automatic repair is disabled, deletion still runs Level 4
-Level 4 realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or
+`DelaunayRepairPolicy` allows it. If automatic repair is disabled, deletion still runs Level 4
+realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `DelaunayRepairPolicy` allows it. If automatic repair is disabled, deletion still runs Level 4 | |
| Embedding Validity and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or | |
| Level 4 realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or | |
| `DelaunayRepairPolicy` allows it. If automatic repair is disabled, deletion still runs Level 4 | |
| realization validation and the Level 5 Delaunay predicate, rolling back on any violation. If post-deletion repair, validation, or |
🤖 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 `@docs/workflows.md` around lines 440 - 441, The deletion description contains
a duplicated “Level 4” phrase in the workflow text. Update the sentence in the
deletion/repair section so it reads naturally by removing the repeated “Level 4”
before “realization validation,” while keeping the rest of the validation and
rollback behavior wording unchanged.
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.
No description provided.