Refactor/387 tds mutation boundaries#390
Merged
Merged
Conversation
- Update the README pitch, feature list, references, and docs.rs-facing guidance around exact predicates, SoS, PL-manifold validation, and bistellar repair. - Refresh roadmap, release, limitation, robustness, orientation, invariant, diagnostics, property-testing, workflow, and validation docs for the v0.7.8 cleanup path and v0.8.0 paper-facing work. - Align contributor and script docs around non-mutating `just` checks before mutating fixes. - Update generated benchmark-summary guidance to surface `just bench-perf-summary`, current Criterion metadata, and large-scale characterization defaults.
- Represent cell neighbor slots as typed boundary, neighbor, and unassigned states so validation can reject stale assignment buffers. - Make `Cell::neighbors()` zero-copy and add explicit `neighbor_key()` and `neighbor_slots()` read APIs for indexed or typed access. - Keep `Vertex::incident_cell` mutation crate-owned and migrate insertion, flip, validation, example, benchmark, and property-test call sites. - Harden CI performance summary parsing so stale or malformed simplex-count sidecars are skipped, and make generated predicate performance notes dimension-dependent. BREAKING CHANGE: `Cell::neighbors()` now returns an iterator over `Option<CellKey>` instead of an owned `NeighborBuffer<Option<CellKey>>`. Direct field mutation of cell neighbors and vertex incident-cell pointers is no longer public; callers should use the read APIs and TDS-owned mutation paths.
Contributor
WalkthroughReplace per-cell optional neighbor buffers with typed NeighborSlot (Unassigned/Boundary/Neighbor), encapsulate Vertex incident-cell via getter/setter, and migrate TDS, triangulation, algorithm, tests, docs, and benchmarks to use neighbor_keys()/neighbor_key()/set_neighbors_from_keys(); add cached same-machine benchmark baseline and compare-ref CLI flow. ChangesTDS neighbor and incident-cell encapsulation refactoring
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
🟢 Coverage 93.02% diff coverage · +0.06% coverage variation
Metric Results Coverage variation ✅ +0.06% coverage variation (-1.00%) Diff coverage ✅ 93.02% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (526583c) 60205 54425 90.40% Head commit (2feb6a0) 60458 (+253) 54692 (+267) 90.46% (+0.06%) 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 (#390) 774 720 93.02% 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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
+ Coverage 90.39% 90.45% +0.06%
==========================================
Files 61 61
Lines 60007 60260 +253
==========================================
+ Hits 54241 54508 +267
+ Misses 5766 5752 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/algorithms/flips.rs (1)
822-838:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocal cavity validation can silently accept
Unassignedneighbor slots.Using
neighbor_keys()here collapses slot state, soNoneskips both boundary and potentially unassigned entries. InFlipValidationScope::LocalCavity, that can let partially wired trial cells pass and be committed.Suggested fix
- let Some(neighbors) = cell.neighbor_keys() else { + let Some(neighbor_slots) = cell.neighbor_slots() else { return Ok(()); }; - if neighbors.len() != D + 1 { + if neighbor_slots.len() != D + 1 { return Err(TdsValidationFailure::InvalidNeighbors { reason: NeighborValidationError::LengthMismatch { - actual: neighbors.len(), + actual: neighbor_slots.len(), expected: D + 1, context: "flip trial neighbor validation".to_string(), }, }); } - for (facet_idx, neighbor_key_opt) in neighbors.enumerate() { - let Some(neighbor_key) = neighbor_key_opt else { - continue; - }; + for (facet_idx, slot) in neighbor_slots.enumerate() { + let neighbor_key = match slot { + NeighborSlot::Neighbor(key) => key, + NeighborSlot::Boundary => continue, + NeighborSlot::Unassigned => { + return Err(TdsValidationFailure::InvalidNeighbors { + reason: NeighborValidationError::Other { + message: format!( + "flip trial cell {cell_key:?} has unassigned neighbor slot at facet {facet_idx}" + ), + }, + }); + } + };As per coding guidelines: “Every mutating operation must preserve invariants checked by Tds::is_valid (Level 1-3) and DelaunayTriangulation::is_valid (Level 4), or fail explicitly rather than leaving the triangulation inconsistent”.
🤖 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/flips.rs` around lines 822 - 838, The validation currently uses cell.neighbor_keys(), which collapses slot state so Unassigned entries and boundary None are indistinguishable, allowing partially wired trial cells to pass in FlipValidationScope::LocalCavity; change the check to iterate the raw neighbor slots (e.g., cell.neighbor_slots() or the equivalent API that preserves slot state) and explicitly treat Unassigned slots as a validation failure in the LocalCavity path (returning an appropriate TdsValidationFailure instead of skipping), while still permitting true boundary None where intended; update the loop that currently matches let Some(neighbor_key) = neighbor_key_opt to instead inspect each slot's enum/state and error on Unassigned to prevent committing invalid flips.
🧹 Nitpick comments (1)
src/core/triangulation.rs (1)
8375-8377: ⚡ Quick winMake these boundary-state assertions explicit.
These macros still go through
cell.neighbors()/Option<CellKey>, so they can't distinguishBoundaryfromUnassigned, and theif let Some(...)makes the check vacuous when the buffer is absent. That leaves the new neighbor-slot contract largely untested across D=2..5.Suggested assertion shape
- if let Some(mut neighbors) = cell.neighbors() { - assert!(neighbors.all(|n| n.is_none()), - "{}D: Initial simplex should have no neighbors (all boundary)", $dim); - } + let mut slots = cell + .neighbor_slots() + .expect("initial simplex should expose explicit neighbor-slot state"); + assert!(slots.all(|slot| slot.is_boundary()), + "{}D: Initial simplex should mark every facet as Boundary", $dim);Also applies to: 9679-9681
🤖 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/triangulation.rs` around lines 8375 - 8377, The current checks call cell.neighbors() and assert that Option<CellKey> slots are None, which doesn't distinguish the new NeighborSlot states (Boundary vs Unassigned) and the `if let Some(...)` makes the assertion vacuous when the neighbor buffer is absent; update the assertions to read the raw neighbor-slot buffer and explicitly assert each slot equals NeighborSlot::Boundary (or the intended variant) rather than checking Option<CellKey>, e.g. use the API that returns the NeighborSlot values (or index into cell.neighbor_slots / cell.neighbors_raw) and assert equality against NeighborSlot::Boundary for the initial simplex in triangulation::initialize (the block around cell.neighbors()) and replicate the same explicit NeighborSlot::Boundary checks in the other occurrence around lines 9679-9681 so the contract is actually tested across D=2..5.
🤖 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/cell.rs`:
- Around line 948-960: The new neighbors buffer must not be prefilled with
Boundary slots; change ensure_neighbors_buffer_mut to initialize the buffer with
the "unassigned" variant so partially-wired cells are visible — in the closure
that currently does buffer.resize(D + 1, NeighborSlot::Boundary) replace the
filler with the unassigned slot (e.g. NeighborSlot::Unassigned or the
UnassignedNeighborSlot variant used by your NeighborSlot enum) so new
NeighborBuffer entries start as unassigned; keep the debug_assert and len D+1
invariant intact.
- Around line 937-946: The helper set_neighbors_from_keys currently accepts any
iterator and installs a NeighborBuffer of arbitrary length; change it to
validate the neighbor arity equals the expected D + 1 before mutating self:
collect the iterator into a temporary NeighborBuffer (using
NeighborSlot::from_neighbor_key), check its length against the cell's required
neighbor count (the D+1 arity for this Cell), and if it does not match return an
explicit error (e.g., Result::Err with a new InvalidNeighborArity error) instead
of setting self.neighbors; only assign self.neighbors = Some(slots) when the
arity check passes. Use the function name set_neighbors_from_keys, types
NeighborBuffer, NeighborSlot, CellKey and the existing self.neighbors field to
locate and apply this change.
In `@src/core/tds.rs`:
- Around line 3652-3653: The star-walk fast path currently only fallbacks when
the seed cell lacks neighbors; update the logic in the start_cell traversal (the
code using start_cell.contains_vertex, start_cell.neighbor_slots(),
neighbor_keys(), and neighbor_slots() checks) so that any time a visited cell
has no neighbor buffer or contains an Unassigned slot the routine immediately
returns fallback_scan() instead of continuing and returning a partial star;
apply the same change to the other traversal block referenced around the
neighbor iteration (the region covering the logic near lines handling
neighbor_keys() between the current start and the block at the 3670-3704 region)
so that remove_vertex() is only called when the full, fully-wired star is
guaranteed.
- Around line 6056-6080: The current check uses
cell.neighbor_key(facet_index).flatten(), which collapses Unassigned and
Boundary into the same case and allows uninitialized boundary slots; instead,
fetch the raw NeighborSlot (use the non-flattened neighbor_key result),
explicitly match on its variants, and if it is Unassigned return an appropriate
TdsError::InvalidNeighbors (rejecting uninitialized boundary facets) while
preserving the existing handling for Boundary (including the periodic
self-adjacency path via Self::allows_periodic_self_neighbor) and for real
neighbor keys; update the logic around neighbor_key, neighbor == cell_key, and
the two existing NeighborValidationError branches to use this explicit match.
In `@src/core/triangulation.rs`:
- Around line 2013-2020: cell_neighbors currently yields every stored
Some(CellKey) even if the key points to a non-existent cell; update
cell_neighbors to filter out dangling neighbor keys by checking existence with
self.tds.contains_cell(key) before yielding them (similar to the guards used in
build_adjacency_index() and traverse_cell_neighbor_graph()). Locate the method
cell_neighbors (which calls self.tds.cell(c).and_then(Cell::neighbors)...) and
insert a filter step that only passes through neighbor keys where
self.tds.contains_cell(neighbor_key) is true, ensuring only existing neighbors
are exposed.
---
Outside diff comments:
In `@src/core/algorithms/flips.rs`:
- Around line 822-838: The validation currently uses cell.neighbor_keys(), which
collapses slot state so Unassigned entries and boundary None are
indistinguishable, allowing partially wired trial cells to pass in
FlipValidationScope::LocalCavity; change the check to iterate the raw neighbor
slots (e.g., cell.neighbor_slots() or the equivalent API that preserves slot
state) and explicitly treat Unassigned slots as a validation failure in the
LocalCavity path (returning an appropriate TdsValidationFailure instead of
skipping), while still permitting true boundary None where intended; update the
loop that currently matches let Some(neighbor_key) = neighbor_key_opt to instead
inspect each slot's enum/state and error on Unassigned to prevent committing
invalid flips.
---
Nitpick comments:
In `@src/core/triangulation.rs`:
- Around line 8375-8377: The current checks call cell.neighbors() and assert
that Option<CellKey> slots are None, which doesn't distinguish the new
NeighborSlot states (Boundary vs Unassigned) and the `if let Some(...)` makes
the assertion vacuous when the neighbor buffer is absent; update the assertions
to read the raw neighbor-slot buffer and explicitly assert each slot equals
NeighborSlot::Boundary (or the intended variant) rather than checking
Option<CellKey>, e.g. use the API that returns the NeighborSlot values (or index
into cell.neighbor_slots / cell.neighbors_raw) and assert equality against
NeighborSlot::Boundary for the initial simplex in triangulation::initialize (the
block around cell.neighbors()) and replicate the same explicit
NeighborSlot::Boundary checks in the other occurrence around lines 9679-9681 so
the contract is actually tested across D=2..5.
🪄 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: 40e3269a-bfb2-4c9d-9fdb-b8dca5be8ee4
📒 Files selected for processing (25)
benches/PERFORMANCE_RESULTS.mdbenches/ci_performance_suite.rsexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/pachner_roundtrip_4d.rsexamples/topology_editing_2d_3d.rsscripts/benchmark_utils.pyscripts/tests/test_benchmark_utils.pysrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/cell.rssrc/core/tds.rssrc/core/triangulation.rssrc/core/util/delaunay_validation.rssrc/core/vertex.rssrc/triangulation/delaunay.rssrc/triangulation/flips.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/prelude_exports.rstests/proptest_serialization.rstests/proptest_tds.rs
- Reject unassigned TDS neighbor slots during validation and flip trials instead of treating them as boundary facets. - Require neighbor-buffer arity before installation, assign explicit boundary slots for initial simplexes, and filter dangling public neighbor keys. - Cache same-machine performance-regression baselines by ref, commit, and Rust compiler for perf-no-regressions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/util/delaunay_validation.rs`:
- Around line 545-546: The code currently collapses neighbor slot state via
neighbor_keys().map_or_else(NeighborBuffer::new, Iterator::collect) which loses
the typed slot enum (Boundary vs Unassigned) so DelaunayViolationDetail cannot
distinguish hull facets from missing wiring; change these paths to preserve and
transport the actual slot enum (the typed neighbor-slot variant) instead of
converting to Option<CellKey>, update the construction of the NeighborBuffer
(and analogous code around the other occurrence near lines 749-767) to accept
and store the typed enum values, and adjust the DelaunayViolationDetail
rendering/debug dump to explicitly include the slot variant
(Boundary/Unassigned/Assigned) when emitting diagnostics rather than relying on
neighbor_keys().
🪄 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: 704748fe-0678-4b0f-96a8-e0163106dde6
📒 Files selected for processing (12)
docs/dev/commands.mddocs/dev/tooling-alignment.mdjustfilescripts/benchmark_utils.pyscripts/tests/test_benchmark_utils.pysrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/cell.rssrc/core/tds.rssrc/core/triangulation.rssrc/core/util/delaunay_validation.rs
✅ Files skipped from review due to trivial changes (2)
- docs/dev/tooling-alignment.md
- docs/dev/commands.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/core/algorithms/locate.rs
- src/core/cell.rs
- src/core/algorithms/flips.rs
- src/core/triangulation.rs
- src/core/algorithms/incremental_insertion.rs
- src/core/tds.rs
- Add a large-scale smoke recipe for quick 2D-5D pre-push performance checks. - Compare local ref baselines by total matched benchmark time while still reporting individual regressions and improvements. - Split release and worktree comparison reports into explicit filenames. - Add citation metadata validation to normal configuration linting. - Preserve raw neighbor slot state in Delaunay violation diagnostics and expose it through the diagnostics prelude. - Refresh README, workflow, and benchmark documentation for the updated validation and performance workflows.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
justfile (1)
953-953: ⚡ Quick winConsider passing TOML filenames via argv instead of embedding in Python source.
Embedding
{}directly into thepython -cstring would be brittle for filenames containing quotes or escape characters. While no tracked TOML files currently have such names, usingsys.argv[1]defensively prevents future failures.Suggested fix
- printf '%s\0' "${files[@]}" | xargs -0 -I {} uv run python -c "import tomllib; tomllib.load(open('{}', 'rb')); print('{} is valid TOML')" + printf '%s\0' "${files[@]}" | xargs -0 -n1 uv run python -c 'import sys, tomllib; p = sys.argv[1]; tomllib.load(open(p, "rb")); print(f"{p} is valid TOML")'🤖 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 `@justfile` at line 953, Replace embedding the filename into the inline Python code with passing it as an argv to Python: change the xargs call so the {} is provided as a program argument (after the -c script) and update the python -c string to import sys and tomllib, call tomllib.load(open(sys.argv[1], 'rb')) and print sys.argv[1] + " is valid TOML" (or use an f-string) instead of referencing '{}' inside the source; this uses sys.argv[1] and tomllib.load to avoid brittle quoting issues.
🤖 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 `@justfile`:
- Line 953: Replace embedding the filename into the inline Python code with
passing it as an argv to Python: change the xargs call so the {} is provided as
a program argument (after the -c script) and update the python -c string to
import sys and tomllib, call tomllib.load(open(sys.argv[1], 'rb')) and print
sys.argv[1] + " is valid TOML" (or use an f-string) instead of referencing '{}'
inside the source; this uses sys.argv[1] and tomllib.load to avoid brittle
quoting issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 128a2f41-430c-44b3-87dc-f8d3f19fab3b
📒 Files selected for processing (16)
.github/workflows/benchmarks.yml.gitignoreCITATION.cffREADME.mdbenches/README.mddocs/code_organization.mddocs/dev/commands.mddocs/dev/tooling-alignment.mddocs/workflows.mdjustfilescripts/README.mdscripts/benchmark_utils.pyscripts/tests/test_benchmark_utils.pysrc/core/util/delaunay_validation.rssrc/lib.rstests/prelude_exports.rs
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- docs/workflows.md
- CITATION.cff
- docs/code_organization.md
- scripts/README.md
- README.md
- benches/README.md
- docs/dev/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/prelude_exports.rs
This was referenced May 19, 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 #387