feat(cdt): implement explicit CDT strip construction#114
Merged
acgetchell merged 3 commits intomainfrom May 5, 2026
Merged
Conversation
- Replace the placeholder strip constructor with deterministic layered connectivity, direct time labels, overflow checks, and strict CDT validation. - Persist Up/Down cell classifications for explicit strip and toroidal constructors while preserving backend payloads on mutation failures. - Update foliation documentation for explicit open-boundary strips and toroidal meshes. - Tighten benchmark tooling diagnostics and Python exception boundaries so stdout remains data-only and broad support-script catches stay blocked.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
- Coverage 93.20% 92.50% -0.71%
==========================================
Files 13 13
Lines 7625 7883 +258
==========================================
+ Hits 7107 7292 +185
- Misses 518 591 +73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis PR adds an explicit deterministic from_cdt_strip() CDT constructor and strict per-face cell classification/validation, tightens classification and rollback semantics, narrows many broad Python except handlers in benchmark tooling, updates tests/docs, and expands Semgrep scope to cover all scripts/**/*.py. ChangesExplicit CDT Strip & Strict Cell Classification
Python Tooling, Tests & Semgrep
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant StripCtor as from_cdt_strip()
participant Delaunay as Delaunay Backend
participant Foliation as Foliation/Validator
participant Backend as Backend Cell Storage
Caller->>StripCtor: request strip(vertices_per_slice, num_slices)
StripCtor->>StripCtor: build deterministic vertex grid (assign time labels)
StripCtor->>StripCtor: generate explicit Up/Down cell connectivity
StripCtor->>Delaunay: build_delaunay2_from_cells(cells)
Delaunay-->>StripCtor: success / DelaunayGenerationFailed
StripCtor->>Foliation: construct foliation (fixed slice sizes)
Foliation->>Foliation: run topology/causality validation
Foliation->>StripCtor: validation result
StripCtor->>StripCtor: classify_all_cells() -> compute per-face classification
StripCtor->>Backend: clear existing cell payloads (transactional)
Backend-->>StripCtor: success / mutation failure
StripCtor->>Backend: write Up/Down payloads (rollback on failure)
Backend-->>StripCtor: final success
StripCtor-->>Caller: return validated CdtTriangulation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
docs/project.md (1)
55-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
validate_cell_classification()to this summary.
CdtTriangulation::validate()now includes the strict Up/Down pass, but this overview still lists only topology/foliation/causality validation. That leaves the project map out of sync with the public API.🤖 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/project.md` around lines 55 - 60, The docs summary is missing validate_cell_classification() which is now part of CdtTriangulation::validate(); update the list of Validation methods to include validate_cell_classification() alongside validate_topology(), validate_foliation(), and validate_causality(), and briefly describe it as the strict Up/Down cell classification/validation pass so the documentation matches the public API.
🤖 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/foliation.md`:
- Line 16: The intro paragraph contradicts the earlier topology bullets: update
the topology bullets (or their heading) to explicitly state they describe the
periodic case (S¹/cylinder/torus) or expand them to include open-boundary strip
variants; specifically reference the functions from_cdt_strip() and
from_toroidal_cdt() in the text so readers understand from_toroidal_cdt() builds
the periodic S¹×S¹/toroidal case while from_cdt_strip() builds open
spatial-interval (strip) geometries, and adjust the surrounding sentences to
consistently label bullets as “periodic” or “open-boundary” accordingly.
In `@src/cdt/triangulation.rs`:
- Around line 2152-2154: Replace guards that call has_foliation() with direct
checks against the foliation option so stale bookkeeping doesn’t hide an
existing foliation: change any early-return or gate like `if
!self.has_foliation() { return Ok(()); }` (and the other occurrence near the
classify_all_cells()/validate() paths) to `if self.foliation.is_none() { return
Ok(()); }` (or equivalently gate on `self.foliation.is_some()` when proceeding),
since geometry_mut() only clears foliation_synced_at_modification and does not
drop `self.foliation`; update the checks in the methods validate(),
classify_all_cells(), and any similar branches that currently rely on
has_foliation().
---
Outside diff comments:
In `@docs/project.md`:
- Around line 55-60: The docs summary is missing validate_cell_classification()
which is now part of CdtTriangulation::validate(); update the list of Validation
methods to include validate_cell_classification() alongside validate_topology(),
validate_foliation(), and validate_causality(), and briefly describe it as the
strict Up/Down cell classification/validation pass so the documentation matches
the public API.
🪄 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: dea87fc9-90b9-411c-b3aa-4bfaabdb70fa
📒 Files selected for processing (12)
docs/dev/tooling-alignment.mddocs/foliation.mddocs/project.mdscripts/benchmark_models.pyscripts/benchmark_utils.pyscripts/performance_analysis.pyscripts/tests/test_benchmark_models.pyscripts/tests/test_benchmark_utils.pysemgrep.yamlsrc/cdt/ergodic_moves.rssrc/cdt/triangulation.rstests/proptest_foliation.rs
🔴 Performance Regression DetectedPerformance Analysis ReportCDT Performance Analysis ReportGenerated: 2026-05-05T09:10:15+00:00 Summary
🔴 Performance Regressions
🟢 Performance Improvements
✅ Stable BenchmarksNo significant changes detected in 1 benchmarks. Performance analysis powered by Criterion.rs |
- Keep live foliation queries and cell classification active when mutable backend access invalidates sync bookkeeping without removing the foliation. - Clarify periodic and open-boundary CDT foliation docs, including `from_toroidal_cdt()` and `from_cdt_strip()` behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cdt/triangulation.rs`:
- Around line 1384-1401: The loop still builds huge allocations (vertex_specs
and cells) and creates one heap allocation per triangle; before any pushes use
checked arithmetic on total_vertices/total_cells and call try_reserve on
vertex_specs and cells (or validate against an explicit upper bound) and map
failures to the appropriate CdtError so allocation failures return an error
instead of aborting; replace Vec<Vec<usize>> cells with a flat representation
(e.g., Vec<[usize;3]> or Vec<usize> with 3-per-triangle chunks) to avoid
per-cell heap allocations, and update uses of index, t_count, n,
vertices_per_slice, total_vertices and total_cells accordingly.
- Around line 1410-1420: The check in from_cdt_strip() currently only validates
backend.vertex_count(); also verify the face count matches by asserting
backend.face_count() == total_cells as usize before accepting the strip. Update
the error path (the Err(CdtError::DelaunayGenerationFailed { ... })) to return a
clear message when face_count() differs (similar structure to the existing
vertex_count error), and reference backend.face_count(), total_cells, and the
caller build_delaunay2_from_cells()/from_cdt_strip() to ensure the returned
strip matches the requested vertices_per_slice and num_slices.
🪄 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: c46b03d3-a481-476e-964b-66d2b6ed2eac
📒 Files selected for processing (3)
docs/foliation.mddocs/project.mdsrc/cdt/triangulation.rs
✅ Files skipped from review due to trivial changes (1)
- docs/project.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/foliation.md
🔴 Performance Regression DetectedPerformance Analysis ReportCDT Performance Analysis ReportGenerated: 2026-05-05T15:15:04+00:00 Summary
🔴 Performance Regressions
🟢 Performance Improvements
✅ Stable BenchmarksNo significant changes detected in 25 benchmarks. Performance analysis powered by Criterion.rs |
- Reserve explicit strip construction buffers fallibly and report CDT generation errors with strip context when allocation fails. - Keep strip triangles in fixed-size cells until the backend boundary and reject generated backends whose vertex or face counts differ from the requested strip.
🔴 Performance Regression DetectedPerformance Analysis ReportCDT Performance Analysis ReportGenerated: 2026-05-05T22:17:43+00:00 Summary
🔴 Performance Regressions
Performance analysis powered by Criterion.rs |
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/cdt/triangulation.rs (1)
1659-1670:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
from_toroidal_cdtchecks vertex count but skips the face count postcondition.
from_cdt_stripnow usesvalidate_strip_countsto assert bothvertex_count()andface_count()match expectations. The toroidal path only verifies the vertex count: ifbuild_toroidal_delaunay2returns a topologically valid mesh with wrong face count (e.g., merged triangles), the constructor accepts it silently. For a regularN × Ttorus the invariant isface_count == 2 * total_vertices.🐛 Proposed fix
let backend = DelaunayBackend2D::from_triangulation(dt); if backend.vertex_count() != total_vertices as usize { return Err(CdtError::DelaunayGenerationFailed { vertex_count: total_vertices, coordinate_range: (0.0, 1.0), attempt: 1, underlying_error: format!( "explicit toroidal builder produced {} vertices, expected {}", backend.vertex_count(), total_vertices, ), }); } + let expected_toroidal_faces = 2 * total_vertices as usize; + if backend.face_count() != expected_toroidal_faces { + return Err(CdtError::DelaunayGenerationFailed { + vertex_count: total_vertices, + coordinate_range: (0.0, 1.0), + attempt: 1, + underlying_error: format!( + "explicit toroidal builder produced {} faces, expected {}", + backend.face_count(), + expected_toroidal_faces, + ), + }); + }🤖 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/cdt/triangulation.rs` around lines 1659 - 1670, from_toroidal_cdt currently only checks backend.vertex_count() but must also assert the expected face count like from_cdt_strip does via validate_strip_counts; update from_toroidal_cdt (the path that calls build_toroidal_delaunay2 and inspects backend) to call validate_strip_counts or manually compare backend.face_count() against 2 * total_vertices and return a DelaunayGenerationFailed with a descriptive underlying_error if it mismatches; reference the same error variant CdtError::DelaunayGenerationFailed and include both vertex_count and face_count expectations in the error text so failures on merged/incorrect triangles are caught.
🤖 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/cdt/triangulation.rs`:
- Around line 118-129: strip_generation_error is incorrectly declared as const
fn but moves a heap-allocated String (unsupported in stable Rust); change its
signature to plain fn strip_generation_error(...) -> CdtError and remove the
const qualifier so the function can construct CdtError::DelaunayGenerationFailed
with the underlying_error String at runtime (mirror the non-const pattern used
by remap_toroidal_generation_error and keep the same parameter names and return
type).
---
Outside diff comments:
In `@src/cdt/triangulation.rs`:
- Around line 1659-1670: from_toroidal_cdt currently only checks
backend.vertex_count() but must also assert the expected face count like
from_cdt_strip does via validate_strip_counts; update from_toroidal_cdt (the
path that calls build_toroidal_delaunay2 and inspects backend) to call
validate_strip_counts or manually compare backend.face_count() against 2 *
total_vertices and return a DelaunayGenerationFailed with a descriptive
underlying_error if it mismatches; reference the same error variant
CdtError::DelaunayGenerationFailed and include both vertex_count and face_count
expectations in the error text so failures on merged/incorrect triangles are
caught.
🪄 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: 26af1b57-c2d7-438e-be73-620f64cfc3f2
📒 Files selected for processing (1)
src/cdt/triangulation.rs
This was referenced May 6, 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
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.
Closes #84