fix(delaunayize): make targeted PL repair failure-atomic#488
Conversation
Closes #304 - Extend PL-manifold repair from facet over-sharing to bounded boundary-ridge, ridge-link, and vertex-link repair stages with typed diagnostics. - Run delaunayize through the Delaunay rollback transaction so failed topology repair, failed Delaunay repair, fallback snapshot failures, and rebuild failures restore the pre-call triangulation. - Expose PlManifoldRepairStage through the crate root and delaunayize prelude, and add benchmark fixtures for targeted topology repair. - Align local and CI tooling on Python 3.14, uv 0.11.26, and the reviewed Rust/Python tool pins.
|
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 (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR adds targeted PL-manifold repair stages, refactors delaunayize into transactional rollback/fallback handling, updates benchmark fixtures and exports, bumps CI/tooling pins for Python 3.14, and cleans up script CLI parsing, exception handling, and type hints. ChangesTargeted PL-manifold repair feature
Estimated code review effort: 4 (Complex) | ~75 minutes CI/tooling and Python 3.14 migration
Estimated code review effort: 2 (Simple) | ~10 minutes Scripts CLI and exception handling cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DelaunayizeByFlips
participant DelaunayRollbackTransaction
participant RepairPlManifoldTopology
participant FinishDelaunayizeAfterDelaunayRepair
DelaunayizeByFlips->>DelaunayRollbackTransaction: start transaction
DelaunayizeByFlips->>RepairPlManifoldTopology: repair facet, ridge-link, and vertex-link violations
RepairPlManifoldTopology-->>DelaunayizeByFlips: repair stats or error
DelaunayizeByFlips->>FinishDelaunayizeAfterDelaunayRepair: finalize flip repair
FinishDelaunayizeAfterDelaunayRepair->>DelaunayRollbackTransaction: commit or rollback
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.15.20)notebooks/nonfaithful_embedding.ipynbUnexpected end of JSON input Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 211 |
🟢 Coverage 92.17% diff coverage · +0.02% coverage variation
Metric Results Coverage variation ✅ +0.02% coverage variation (-1.00%) Diff coverage ✅ 92.17% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (d6e9eea) 76010 69168 91.00% Head commit (a6920bc) 76956 (+946) 70043 (+875) 91.02% (+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 (#488) 1086 1001 92.17% 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
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/delaunay/delaunayize.rs (1)
222-233: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve attempted-repair stats on fallback recovery.
Both successful fallback paths return zeroed
PlManifoldRepairStats/DelaunayRepairStats. That hides real work when topology repair exhausts its budget after removals or flip repair fails after checking facets/flipping cells, even thoughDelaunayizeOutcomeexposes those fields as the repair-pass statistics. Please thread the partial stats/diagnostics into the recovered outcome instead of resetting them.Also applies to: 753-757, 902-905
🤖 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/delaunayize.rs` around lines 222 - 233, The fallback recovery paths in DelaunayizeOutcome are overwriting attempted repair results with zeroed PlManifoldRepairStats and DelaunayRepairStats, which hides the partial work already done. Update the recovery logic in the DelaunayizeOutcome-producing flow so the fallback success cases preserve and return the stats/diagnostics accumulated by the failed topology repair or flip repair attempt instead of resetting them. Focus on the code paths that build the recovered outcome and thread the existing repair-pass stats through those branches.
🧹 Nitpick comments (3)
benches/pl_manifold_repair.rs (1)
87-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the benchmark label from
fixture.stage.
bench_targeted_fixturetakes a free-formstage_nameeven thoughTargetedTopologyRepairFixturealready carries the canonical stage. One mismatched call site will silently mislabel the benchmark output.Proposed refactor
-fn bench_targeted_fixture<const D: usize>( - group: &mut criterion::BenchmarkGroup<'_, criterion::measurement::WallTime>, - stage_name: &str, - fixture: &TargetedTopologyRepairFixture<D>, -) { +fn bench_targeted_fixture<const D: usize>( + group: &mut criterion::BenchmarkGroup<'_, criterion::measurement::WallTime>, + fixture: &TargetedTopologyRepairFixture<D>, +) { + let stage_name = match fixture.stage { + delaunay::PlManifoldRepairStage::BoundaryRidgeMultiplicity => "boundary_ridge_multiplicity", + delaunay::PlManifoldRepairStage::RidgeLink => "ridge_link", + delaunay::PlManifoldRepairStage::VertexLink => "vertex_link", + }; + group.throughput(Throughput::Elements( u64::try_from(fixture.cluster_count).or_abort(), )); group.bench_with_input( - BenchmarkId::new(stage_name, format!("{}_clusters", fixture.cluster_count)), + BenchmarkId::new(format!("{stage_name}_{D}d"), format!("{}_clusters", fixture.cluster_count)), fixture, |b, fixture| { b.iter_batched(🤖 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/pl_manifold_repair.rs` around lines 87 - 111, The benchmark label in bench_targeted_fixture is currently driven by a free-form stage_name instead of the canonical stage stored on TargetedTopologyRepairFixture. Update bench_targeted_fixture to derive the BenchmarkId name from fixture.stage, and remove or stop relying on the external stage_name parameter so the label always matches the fixture metadata. Make the change at the bench_targeted_fixture and BenchmarkId::new call site, using the fixture field as the single source of truth.src/bench_fixtures.rs (1)
482-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the core stage-to-error mapping here.
manifold_error_matches_stageduplicates theManifoldError→PlManifoldRepairStagecontract already defined insrc/core/algorithms/pl_manifold_repair.rs. If that mapping changes upstream, these fixtures can start validating the wrong stage without any local compiler signal. Please funnel this through a shared helper owned by the repair/validation layer instead of keeping a second match table here.As per coding guidelines, "Validation code must live at the lowest layer that owns the invariant."
🤖 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/bench_fixtures.rs` around lines 482 - 500, `manifold_error_matches_stage` is duplicating the stage-to-error contract already owned by `PlManifoldRepairStage` and `ManifoldError`, so move this check to a shared helper in the repair/validation layer and call that helper from the bench fixture instead of maintaining a second match table. Reuse the existing mapping logic from `pl_manifold_repair` rather than pattern-matching again here, so fixture validation stays aligned with the source of truth if the repair stages change.Source: Coding guidelines
src/core/algorithms/pl_manifold_repair.rs (1)
946-994: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the removal heuristic's provenance and conditioning.
simplex_quality_scorenow drives which simplex gets deleted, but its docs only name the aspect-ratio metric. They still don't explain how non-finite/invalid geometry is ranked (f64::MAX) or point readers to the algorithm reference, which makes the repair policy harder to audit. As per coding guidelines, "Algorithms must cite their source inREFERENCES.mdand document conditioning behavior."🤖 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/algorithms/pl_manifold_repair.rs` around lines 946 - 994, Document the provenance and conditioning of simplex deletion in simplex_quality_score by expanding its doc comment to cite the algorithm source in REFERENCES.md and explain that invalid or non-finite geometry is ranked with f64::MAX. Keep the explanation near the existing simplex_quality_score / repair-selection logic so readers can see how the removal heuristic handles bad input and how it maps to the referenced method.Source: Coding guidelines
🤖 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/delaunay/delaunayize.rs`:
- Around line 718-725: The `snapshot_rebuild_state` handling in
`delaunayize_by_flips` is too lenient: when the post-topology fallback snapshot
is already invalid, the `Ok(delaunay_stats)` path still commits and returns
success. Update the branch around `DelaunayizeOutcome` so that a failed snapshot
rebuild after topology repair immediately aborts with a typed error, rolls back
the transaction, and does not reach `transaction.commit()`. Apply the same
fail-fast behavior anywhere the fallback snapshot is validated so the
inconsistency is surfaced right away instead of being deferred until later
repair logic.
---
Outside diff comments:
In `@src/delaunay/delaunayize.rs`:
- Around line 222-233: The fallback recovery paths in DelaunayizeOutcome are
overwriting attempted repair results with zeroed PlManifoldRepairStats and
DelaunayRepairStats, which hides the partial work already done. Update the
recovery logic in the DelaunayizeOutcome-producing flow so the fallback success
cases preserve and return the stats/diagnostics accumulated by the failed
topology repair or flip repair attempt instead of resetting them. Focus on the
code paths that build the recovered outcome and thread the existing repair-pass
stats through those branches.
---
Nitpick comments:
In `@benches/pl_manifold_repair.rs`:
- Around line 87-111: The benchmark label in bench_targeted_fixture is currently
driven by a free-form stage_name instead of the canonical stage stored on
TargetedTopologyRepairFixture. Update bench_targeted_fixture to derive the
BenchmarkId name from fixture.stage, and remove or stop relying on the external
stage_name parameter so the label always matches the fixture metadata. Make the
change at the bench_targeted_fixture and BenchmarkId::new call site, using the
fixture field as the single source of truth.
In `@src/bench_fixtures.rs`:
- Around line 482-500: `manifold_error_matches_stage` is duplicating the
stage-to-error contract already owned by `PlManifoldRepairStage` and
`ManifoldError`, so move this check to a shared helper in the repair/validation
layer and call that helper from the bench fixture instead of maintaining a
second match table. Reuse the existing mapping logic from `pl_manifold_repair`
rather than pattern-matching again here, so fixture validation stays aligned
with the source of truth if the repair stages change.
In `@src/core/algorithms/pl_manifold_repair.rs`:
- Around line 946-994: Document the provenance and conditioning of simplex
deletion in simplex_quality_score by expanding its doc comment to cite the
algorithm source in REFERENCES.md and explain that invalid or non-finite
geometry is ranked with f64::MAX. Keep the explanation near the existing
simplex_quality_score / repair-selection logic so readers can see how the
removal heuristic handles bad input and how it maps to the referenced method.
🪄 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: 27efc85e-c54d-49e2-9c11-3dc0fbf766d7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/workflows/audit.yml.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/codacy.yml.github/workflows/codecov.yml.github/workflows/codeql.yml.github/workflows/generate-baseline.yml.github/workflows/profiling-benchmarks.yml.github/workflows/release-benchmarks.yml.github/workflows/rust-clippy.yml.github/workflows/semgrep-sarif.yml.python-versionCHANGELOG.mdbenches/README.mdbenches/pl_manifold_repair.rsdocs/dev/tooling-alignment.mdjustfilepyproject.tomlscripts/README.mdscripts/archive_changelog.pyscripts/benchmark_models.pyscripts/benchmark_utils.pyscripts/ci/filter_codacy_sarif.pyscripts/hardware_utils.pyscripts/notebook_check.pyscripts/postprocess_changelog.pyscripts/semgrep_fixture_config.pyscripts/subprocess_utils.pyscripts/tag_release.pyscripts/tests/conftest.pyscripts/tests/test_benchmark_utils.pyscripts/tests/test_hardware_utils.pyscripts/tests/test_notebook_check.pyscripts/tests/test_semgrep_fixture_config.pyscripts/tests/test_subprocess_utils.pysrc/bench_fixtures.rssrc/core/algorithms/pl_manifold_repair.rssrc/delaunay/delaunayize.rssrc/lib.rstests/prelude_exports.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #488 +/- ##
==========================================
+ Coverage 90.97% 90.99% +0.01%
==========================================
Files 86 86
Lines 75789 76734 +945
==========================================
+ Hits 68949 69823 +874
- Misses 6840 6911 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Roll back immediately when fallback simplex-data snapshots cannot be validated, before topology or Delaunay repair recovery can commit. Preserve repair diagnostics when fallback rebuild recovery succeeds. - Deprecate the legacy Delaunay repair snapshot-error variant in favor of the phase-independent fallback snapshot error. - Share the PL-manifold stage-to-error contract with benchmark fixtures and derive Criterion labels from validated fixture metadata. - Add the non-faithful embedding notebook and document the Codacy uv.lock line-length exemption.
1 participant
Closes #304
Closes #304