Fixed: Prevents timeout in 4D bulk construction#203
Merged
acgetchell merged 7 commits intomainfrom Feb 20, 2026
Merged
Conversation
Addresses a timeout issue in 4D bulk construction by implementing per-insertion local Delaunay repair (soft-fail) during bulk construction to prevent violation accumulation, which slows down the global repair process. Also adds a hard wall-clock time limit to the test harness. Refs: fix/4d-bulk-construction-timeout
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-attempt and per-seed flip-budget overrides and a cfg(test|debug_assertions) single-pass local repair; enriches conflict/boundary diagnostics with extra cell metadata; implements iterative, dimension-aware cavity-boundary reshaping; wires per-insertion local repairs and soft-fail seed collection into bulk construction; and introduces test harness runtime caps and debug modes. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
added
bug
![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: 1
🧹 Nitpick comments (4)
src/core/algorithms/incremental_insertion.rs (1)
2622-2629: LGTM —extra_cellsfield update is correct; consider a non-empty variant for completeness.The mechanical addition of
extra_cells: vec![]correctly satisfies the updatedConflictError::RidgeFanstruct definition. Theis_retryable()match at line 249 usesRidgeFan { .. }(wildcard), so retryability is rightly independent ofextra_cellscontents.As an optional improvement, adding a second assertion with a non-empty
extra_cellswould document and lock in the invariant that retryability is unconditional for anyRidgeFan:🔍 Optional complementary assertion
assert!( InsertionError::ConflictRegion(ConflictError::RidgeFan { facet_count: 3, ridge_vertex_count: 2, extra_cells: vec![], }) .is_retryable() ); +// Retryability must not depend on extra_cells content. +assert!( + InsertionError::ConflictRegion(ConflictError::RidgeFan { + facet_count: 3, + ridge_vertex_count: 2, + extra_cells: vec![CellKey::from(KeyData::from_ffi(1))], + }) + .is_retryable() +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/algorithms/incremental_insertion.rs` around lines 2622 - 2629, Add a complementary assertion to prove retryability is independent of the RidgeFan.extra_cells contents: keep the existing assert checking InsertionError::ConflictRegion(ConflictError::RidgeFan { facet_count: 3, ridge_vertex_count: 2, extra_cells: vec![] }).is_retryable() and add a second assertion that constructs InsertionError::ConflictRegion(ConflictError::RidgeFan { facet_count: 3, ridge_vertex_count: 2, extra_cells: vec![some non-empty placeholder element] }) and also calls .is_retryable(); reference the ConflictError::RidgeFan variant and the is_retryable() method so the test documents that extra_cells does not affect retryability.tests/large_scale_debug.rs (1)
66-72:std::process::exitskips stack unwinding — acceptable here but worth a note.
process::exitcalls libcexit, which flushes stdio but does not unwind the Rust stack. This means noDropimpls run, and the Cargo test runner won't print its normal failure summary. For an#[ignore]debug harness this is fine (and likely intentional), but if you ever need cleanup or want structured test-runner output on timeout, consider swapping tostd::process::abort()(which also skips unwinding but signals abnormal termination) or, for a softer approach, using astd::sync::mpsc::channel+recv_timeoutpattern so the main thread can panic normally.Also, if multiple
debug_large_casetests run in one process invocation, each spawns its own watchdog — the earliest-spawned will fire first regardless of which test is active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/large_scale_debug.rs` around lines 66 - 72, Replace the call to std::process::exit(2) inside install_runtime_cap with std::process::abort() to avoid relying on libc exit semantics; optionally, change install_runtime_cap to return the spawned thread's JoinHandle or implement a per-test watchdog using a std::sync::mpsc::channel + recv_timeout so each test can time out itself (avoid multiple global watchdogs firing across tests). Ensure you update the install_runtime_cap function signature and callers if you return the JoinHandle, or implement the channel/recv_timeout pattern in the debug_large_case test harness.src/core/algorithms/locate.rs (2)
1239-1250:filter_mapsilently dropsextra_facetsindices that should always be valid.All indices pushed into
extra_facetsareboundary_facets.len()captured before the corresponding push, soboundary_facets.get(fi)returningNoneis impossible by construction. Usingfilter_maphere masks any future index-bookkeeping regression by silently yielding anextra_cellsshorter thanfacet_count - 2rather than loudly failing.♻️ Suggested: add a
debug_assertto make the invariant explicitlet extra_cells: Vec<CellKey> = info .extra_facets .iter() - .filter_map(|&fi| boundary_facets.get(fi)) + .map(|&fi| { + debug_assert!(fi < boundary_facets.len(), "extra_facets index {fi} out of bounds"); + boundary_facets.get(fi) + }) + .flatten() .map(FacetHandle::cell_key) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/algorithms/locate.rs` around lines 1239 - 1250, The code silently drops invalid indices by using filter_map on info.extra_facets when constructing extra_cells; instead, assert the invariant and convert directly so regressions fail loudly. Add a debug_assert that each &fi in extra_facets is within boundary_facets.len(), then build extra_cells by mapping each &fi to boundary_facets.get(fi).expect(...) (or boundary_facets[fi]) and then calling FacetHandle::cell_key; return the same ConflictError::RidgeFan with the constructed extra_cells and preserve facet_count and ridge_vertex_count.
2057-2062: New payload fields have no content coverage — recommend extending these tests.Both
test_extract_cavity_boundary_detects_disconnected_boundary_2dandtest_extract_cavity_boundary_rejects_ridge_fan_2duse..to silently ignore the newdisconnected_cellsandextra_cellsfields. Since the entire purpose of these fields is to give callers actionable hints (which cells to trim from the conflict region), a regression in the cell-key computation would go undetected.Consider asserting that
disconnected_cellsis non-empty and contains only keys from the "right" (disconnected) component, and thatextra_cellscontains exactly the cell keys of the 3rd+ facets:// DisconnectedBoundary test — verify field contents ConflictError::DisconnectedBoundary { visited, total, disconnected_cells } => { assert!(visited < total); assert_eq!(total, 6); // Each disconnected component cell key must be a real key in the TDS assert!(!disconnected_cells.is_empty()); for ck in &disconnected_cells { assert!(tds.contains_cell(*ck)); } } // RidgeFan test — verify extra_cells is populated ConflictError::RidgeFan { facet_count, ridge_vertex_count, extra_cells } => { assert!(facet_count >= 3); assert_eq!(ridge_vertex_count, 1); assert_eq!(extra_cells.len(), facet_count - 2, "extra_cells should hold exactly the 3rd+ fan cells"); }Also applies to: 2149-2159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/algorithms/locate.rs` around lines 2057 - 2062, Update the two tests (test_extract_cavity_boundary_detects_disconnected_boundary_2d and test_extract_cavity_boundary_rejects_ridge_fan_2d) to assert on the new ConflictError payload fields instead of ignoring them; for ConflictError::DisconnectedBoundary verify disconnected_cells is non-empty and every cell key it contains passes tds.contains_cell(*ck) and still assert visited < total and total == 6, and for ConflictError::RidgeFan verify extra_cells.len() == facet_count - 2 (and that each key in extra_cells is present in the TDS) while keeping the existing checks for facet_count and ridge_vertex_count so regressions in cell-key computation are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/algorithms/locate.rs`:
- Around line 1215-1222: The code currently uses CellKey::default() as a silent
fallback when boundary_facets.get(info.first_facet) is None; instead, mirror the
handling used for the missing second_facet case by returning a descriptive error
(e.g., ConflictError::CellDataAccessFailed) rather than a null key. Replace the
map_or_else(CellKey::default, FacetHandle::cell_key) usage when computing
open_cell so that if get(info.first_facet) returns None you return
Err(ConflictError::CellDataAccessFailed { /* similar context as the second_facet
branch */ }) instead of producing a default CellKey; keep the returned
ConflictError::OpenBoundary path only when you successfully obtain the
FacetHandle and its cell_key.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 2622-2629: Add a complementary assertion to prove retryability is
independent of the RidgeFan.extra_cells contents: keep the existing assert
checking InsertionError::ConflictRegion(ConflictError::RidgeFan { facet_count:
3, ridge_vertex_count: 2, extra_cells: vec![] }).is_retryable() and add a second
assertion that constructs InsertionError::ConflictRegion(ConflictError::RidgeFan
{ facet_count: 3, ridge_vertex_count: 2, extra_cells: vec![some non-empty
placeholder element] }) and also calls .is_retryable(); reference the
ConflictError::RidgeFan variant and the is_retryable() method so the test
documents that extra_cells does not affect retryability.
In `@src/core/algorithms/locate.rs`:
- Around line 1239-1250: The code silently drops invalid indices by using
filter_map on info.extra_facets when constructing extra_cells; instead, assert
the invariant and convert directly so regressions fail loudly. Add a
debug_assert that each &fi in extra_facets is within boundary_facets.len(), then
build extra_cells by mapping each &fi to boundary_facets.get(fi).expect(...) (or
boundary_facets[fi]) and then calling FacetHandle::cell_key; return the same
ConflictError::RidgeFan with the constructed extra_cells and preserve
facet_count and ridge_vertex_count.
- Around line 2057-2062: Update the two tests
(test_extract_cavity_boundary_detects_disconnected_boundary_2d and
test_extract_cavity_boundary_rejects_ridge_fan_2d) to assert on the new
ConflictError payload fields instead of ignoring them; for
ConflictError::DisconnectedBoundary verify disconnected_cells is non-empty and
every cell key it contains passes tds.contains_cell(*ck) and still assert
visited < total and total == 6, and for ConflictError::RidgeFan verify
extra_cells.len() == facet_count - 2 (and that each key in extra_cells is
present in the TDS) while keeping the existing checks for facet_count and
ridge_vertex_count so regressions in cell-key computation are detected.
In `@tests/large_scale_debug.rs`:
- Around line 66-72: Replace the call to std::process::exit(2) inside
install_runtime_cap with std::process::abort() to avoid relying on libc exit
semantics; optionally, change install_runtime_cap to return the spawned thread's
JoinHandle or implement a per-test watchdog using a std::sync::mpsc::channel +
recv_timeout so each test can time out itself (avoid multiple global watchdogs
firing across tests). Ensure you update the install_runtime_cap function
signature and callers if you return the JoinHandle, or implement the
channel/recv_timeout pattern in the debug_large_case test harness.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (36.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
==========================================
- Coverage 60.82% 60.49% -0.34%
==========================================
Files 46 46
Lines 11136 11441 +305
==========================================
+ Hits 6774 6921 +147
- Misses 4362 4520 +158
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:
|
Addresses timeout issues in 4D bulk construction by: - Implementing per-insertion local Delaunay repair in debug/test modes for D>=3, preventing violation accumulation and reducing global repair time. - Adjusting the flip budget multiplier and minimum for per-insertion repair based on dimension, balancing repair quality and construction speed. - Modifying cavity boundary resolution to avoid immediate star-split fallbacks, retrying with perturbed vertices instead. - Adding a wall-clock timeout to tests, aborting if exceeded to prevent indefinite hangs. Refs: fix/4d-bulk-construction-timeout
coderabbitai
bot
added
api
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/algorithms/flips.rs`:
- Around line 2434-2498: Add an early return when seed_cells.is_empty() in
repair_delaunay_single_pass_local to enforce the local-only contract: before
computing the flip budget (or immediately after the D<2 check), if
seed_cells.is_empty() return Ok(DelaunayRepairStats::default()) (or the
appropriate zero/empty stats value used in the codebase) so the function never
falls back to global seeding via seed_repair_queues; reference the function name
repair_delaunay_single_pass_local and the seed_cells parameter when locating
where to add the guard.
In `@src/core/delaunay_triangulation.rs`:
- Around line 2569-2577: The comment incorrectly says "For D>=4" while the code
gating uses "if D >= 3"; update the wording in the two affected comments to "For
D>=3" so it matches the actual behavior. Specifically, edit the comment blocks
that describe per-insertion LOCAL repair near the functions/methods referenced
(insert_remaining_vertices_seeded, maybe_repair_after_insertion,
finalize_bulk_construction) and change any "D>=4" occurrences to "D>=3" to
reflect the gate used in the code.
In `@src/core/triangulation.rs`:
- Around line 3410-3415: The comment describing the fallback threshold is out of
sync with the code: the code uses a `D < 3` check (so D==3 yields the retryable
error path) but the comment says the retryable path starts at D>=4. Update the
comment near the `extract_cavity_boundary`/star-split block to match the actual
condition (describe that when the loop exhausts its budget: D>=3 returns a
retryable error so insert_transactional retries with a perturbed vertex; D<3
falls through to the star-split fallback), or alternatively change the condition
to `D < 4` if you intended the original D>=4 behavior—make the code and comment
consistent and reference the `extract_cavity_boundary` and star-split fallback
logic when making the fix.
---
Duplicate comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 2670-2674: Update the comment that currently says "For D>=4
debug/test builds, insert_remaining_vertices_seeded injects..." to read "For
D>=3 debug/test builds..." so it matches the earlier note; edit the comment
block that references maybe_repair_after_insertion,
insert_remaining_vertices_seeded, and the _with_construction_statistics variant
to change the dimension threshold from D>=4 to D>=3 and ensure wording remains
consistent with surrounding comments.
- Around line 2934-2966: The stats-enabled branch duplicates the per-insertion
local repair logic already present in the non-stats path; consolidate by
extracting the shared logic into a single helper or by removing the redundant
block so both paths call the same code. Specifically, factor the seed cell
collection and the call to repair_delaunay_single_pass_local (using &mut
self.tri.tds and &self.tri.kernel) into a helper (or reuse the existing
non-stats path) and keep only the stats-specific tracing (flips_performed) where
needed, referencing seed_cells, repair_delaunay_single_pass_local, self.tri.tds
and self.tri.kernel.
Addresses a timeout issue in 4D bulk Delaunay construction by implementing a per-insertion soft-fail local repair for D>=3. This reduces violation accumulation and prevents the global repair from processing a prohibitively large queue, which was slow in debug mode. Also adds tests for the local repair, including dimension checks and handling of empty seed sets. Refs: fix/4d-bulk-construction-timeout
Adds tests to cover degenerate cell scenarios in `is_point_outside_facet` and `find_conflict_region`, ensuring proper error handling when vertex keys are missing from the TDS. Also covers additional branches in the cavity reduction loop in `insert_with_conflict_region`, specifically `RidgeFan SHRINK`, `DisconnectedBoundary EXPAND/SHRINK`, and post-loop error paths, improving robustness and preventing potential panics. These fixes enhance the stability of the locate and triangulation algorithms in edge cases.
Contributor
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/core/algorithms/locate.rs (1)
209-217:⚠️ Potential issue | 🟡 MinorThe new
open_cellfield inConflictError::OpenBoundaryhas no test coverage.
DisconnectedBoundary::disconnected_cellsandRidgeFan::extra_cellsboth have tests that verify the new fields.OpenBoundary::open_cellis unexercised — a follow-up test should construct a TDS whose conflict region has a ridge incident to exactly one boundary facet (e.g., a conflict region consisting of a single cell whose facet is not shared, in a dimension where the adjacency check triggers) and assert thatopen_cellis present in the TDS.Would you like me to generate a test case that triggers
ConflictError::OpenBoundaryand validates the newopen_cellpayload?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/algorithms/locate.rs` around lines 209 - 217, Add a unit test that exercises ConflictError::OpenBoundary by building a TDS whose conflict region contains a ridge incident to exactly one boundary facet (for example, create a minimal TDS with a single cell whose one facet is unshared so the adjacency check triggers), invoke the operation that produces the conflict error, and assert that the returned error is ConflictError::OpenBoundary and that its OpenBoundary.open_cell matches the cell key contributed by that dangling facet; mirror the style used in tests for DisconnectedBoundary::disconnected_cells and RidgeFan::extra_cells to construct the mesh and perform the same kind of payload validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/algorithms/locate.rs`:
- Around line 1264-1268: extra_cells can contain duplicate CellKey entries
because info.extra_facets may map multiple facets from the same cell to the same
CellKey; change the construction of extra_cells in locate.rs so it deduplicates
keys (similar to how disconnected_cells uses a seen set) — e.g., iterate
info.extra_facets, map to boundary_facets[fi].cell_key(), and insert into a
HashSet or use a seen set to produce a unique Vec<CellKey>; keep the resulting
type compatible with downstream code that converts to FastHashSet
(triangulation.rs) and with the test that asserts extra_cells.len().
---
Outside diff comments:
In `@src/core/algorithms/locate.rs`:
- Around line 209-217: Add a unit test that exercises
ConflictError::OpenBoundary by building a TDS whose conflict region contains a
ridge incident to exactly one boundary facet (for example, create a minimal TDS
with a single cell whose one facet is unshared so the adjacency check triggers),
invoke the operation that produces the conflict error, and assert that the
returned error is ConflictError::OpenBoundary and that its
OpenBoundary.open_cell matches the cell key contributed by that dangling facet;
mirror the style used in tests for DisconnectedBoundary::disconnected_cells and
RidgeFan::extra_cells to construct the mesh and perform the same kind of payload
validation.
---
Duplicate comments:
In `@src/core/algorithms/locate.rs`:
- Around line 1219-1234: Replace the previous silent fallback CellKey::default()
by ensuring the code uses boundary_facets.get(info.first_facet) and maps the
found FacetHandle to its cell key via FacetHandle::cell_key, returning a
ConflictError::CellDataAccessFailed with detailed message if the index is
missing, then propagate that resulting open_cell into the
ConflictError::OpenBoundary payload (as shown using open_cell); update any
remaining code that relied on the old default fallback to use this explicit
error and the propagated open_cell instead.
… pass
Root causes of D=4 bulk construction timeouts and false failures:
1. `finalize_bulk_construction` called `repair_delaunay_with_flips_k2_k3`
seeded from all cells (O(cells × queues/cell)) — prohibitively slow in
debug/test mode with robust predicates at scale.
2. `verify_postcondition_inverse_k2_edges` / `_inverse_k3_triangles` treated
the `both_positive_artifact`-suppressed `violates = false` case as a
genuine inverse-flip hit, causing repair passes to abort with false
postcondition failures.
Changes:
- Re-enable per-insertion local Delaunay repair for D≥4 in
`insert_remaining_vertices_seeded` (previously excluded by `&& D < 4`).
Hard-fail for D<4 (flip graph is proven convergent); soft-fail for D≥4
(co-spherical cycling suppressed by `both_positive_artifact`).
- Replace `repair_delaunay_single_pass_local` (cfg(test/debug)-only, no
snapshot, single attempt) with `repair_delaunay_local_single_pass`
(unconditional, three FIFO→LIFO→FIFO attempts with TDS snapshot per
attempt, `max_flips` parameter). Seeded from the star of the inserted
vertex; O(seed_cells) cost regardless of triangulation size.
- Change `finalize_bulk_construction` to use `repair_delaunay_local_single_pass`
seeded from accumulated `soft_fail_seeds` instead of a `None`-seeded global
repair. An empty seed slice (no soft-fails during insertion) exits with an
empty queue in O(1). Passing seeds as `&[CellKey]` avoids the full cell-scan.
- Add `if D >= 4 { continue; }` guard in both inverse postcondition checks to
skip them when `both_positive_artifact` has suppressed the violation flag;
correctness for D≥4 is delegated to `is_delaunay_property_only()`.
- Add `both_positive_artifact` filter in `is_delaunay_property_only` for D≥4:
when both cells of a shared facet report INSIDE simultaneously (impossible by
cofactor antisymmetry), treat the pair as ambiguous and skip the violation.
- Add `Tds::is_connected()` BFS over neighbor pointers and
`InvariantKind::Connectedness`; surface in `validation_report`.
- Move connectedness check first in `Triangulation::is_valid()` (O(N·D) BFS
is cheaper than building the facet map); simplify `validate_global_connectedness`
to delegate to `Tds::is_connected()`. Update tests that expected other errors
before connectivity for disconnected configurations.
- Deduplicate `extra_cells` in `ConflictError::RidgeFan`: multiple extra facets
can come from the same cell; downstream cavity code expects unique keys.
- Switch `large_scale_debug` D=4 case from `FastKernel` to `RobustKernel`.
Add `DELAUNAY_LARGE_DEBUG_ALLOW_SKIPS=1` to `debug-large-scale-4d-100`.
- Add `gen_is_connected` proptest (2D/3D normal, 4D/5D `#[ignore]`) asserting
every successfully-constructed triangulation is connected. Add unit test
`test_is_connected_returns_false_for_isolated_cells`.
Contributor
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 (2)
src/core/delaunay_triangulation.rs (1)
2588-2607:⚠️ Potential issue | 🟠 MajorSoft-fail seeds are never recorded for D≥4.
Line 2588 introducessoft_fail_seeds, but Lines 2836-2844 and 2964-2987 only log on D≥4 soft-fails and never push the seed cells. This makes the final seeded repair a no-op even when per-insertion repair failed, which can leave Delaunay violations unresolved and undermine the intended timeout fix.🐛 Suggested fix (record seeds on D≥4 soft-fail)
tracing::debug!( error = %repair_err, idx = index, "bulk D≥4: per-insertion repair non-convergent; \ continuing (both_positive_artifact handled)" ); + _soft_fail_seeds.extend(seed_cells);tracing::debug!( error = %repair_err, idx = index, "bulk D≥4: per-insertion repair non-convergent; \ continuing (both_positive_artifact handled)" ); + _soft_fail_seeds.extend(seed_cells);Also applies to: 2671-2687, 2693-2700, 2783-2845, 2943-2989
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/delaunay_triangulation.rs` around lines 2588 - 2607, The soft_fail_seeds Vec is never appended to when a soft-fail path is taken for dimension D>=4, so finalize_bulk_construction receives an empty list and seeded repair never runs; locate the soft-fail handling branches in insert_remaining_vertices_seeded (and the other soft-fail logging sites referenced around the blocks that currently only log for D>=4) and add logic to push the failing CellKey (or the appropriate seed cell identifier used elsewhere) into the mutable soft_fail_seeds vector whenever a per-insertion soft-fail is detected; ensure you use the same CellKey type and preserve ownership/borrows so the seed collection is populated before calling finalize_bulk_construction.tests/large_scale_debug.rs (1)
17-48:⚠️ Potential issue | 🟡 Minor
DELAUNAY_LARGE_DEBUG_DEBUG_MODEenv var is undocumented in the header.
debug_mode_from_env()(line 343) readsDELAUNAY_LARGE_DEBUG_DEBUG_MODE, but this variable never appears in the module-level env-var listing (lines 17–48). Every other runtime-configurable variable has an entry there; this one is silently missing.📝 Proposed fix — add the missing entry to the env-var listing
//! # Hard wall-clock cap in seconds before the harness aborts (0 = no cap; default: 600) //! DELAUNAY_LARGE_DEBUG_MAX_RUNTIME_SECS=600 \ +//! # Debug mode: "cadenced" (default, suspicion-driven validation) or "strict" (per-insertion validation) +//! DELAUNAY_LARGE_DEBUG_DEBUG_MODE=cadenced \ //! cargo test --test large_scale_debug debug_large_scale_4d -- --ignored --nocapture🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/large_scale_debug.rs` around lines 17 - 48, The header env-var listing is missing the documented entry for the environment variable used by debug_mode_from_env(); add a line describing DELAUNAY_LARGE_DEBUG_DEBUG_MODE (what values it accepts and its default) to the module-level comment block at the top of tests/large_scale_debug.rs so the README-like bash snippet includes this variable alongside the other DELAUNAY_LARGE_DEBUG_* entries; ensure the name matches exactly (DELAUNAY_LARGE_DEBUG_DEBUG_MODE) and the description matches the behavior implemented in debug_mode_from_env().
🧹 Nitpick comments (3)
src/core/delaunay_triangulation.rs (1)
3063-3114: Skip seeded repair when there are no soft-fail seeds.
repair_delaunay_local_single_passsnapshots/clones the TDS even when the seed set is empty, so the current code still pays O(cells) cost in the “no soft-fails” case. Gating on!soft_fail_seeds.is_empty()avoids that overhead and aligns with the O(1) intent in the comment.♻️ Suggested refactor
- if run_final_repair && self.should_run_delaunay_repair_for(topology, 0) { + if run_final_repair + && self.should_run_delaunay_repair_for(topology, 0) + && !soft_fail_seeds.is_empty() + { // Use a single-pass seeded repair bounded to the soft-fail neighbourhood. - // An empty soft_fail_seeds slice means per-insertion repair had no soft-fails: - // seed_repair_queues leaves the queue empty → O(1) exit. // A non-empty slice targets only the affected neighbourhoods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/delaunay_triangulation.rs` around lines 3063 - 3114, The code always calls repair_delaunay_local_single_pass even when soft_fail_seeds (finalize_seeds) is empty, causing an unnecessary O(cells) snapshot/clone; guard the repair block with a conditional like if !soft_fail_seeds.is_empty() so that when finalize_seeds is empty you skip calling repair_delaunay_local_single_pass and set repair_outcome = Ok(()) directly, otherwise compute global_repair_max_flips and invoke repair_delaunay_local_single_pass as before (referencing repair_delaunay_local_single_pass, finalize_seeds/soft_fail_seeds, global_repair_max_flips, and repair_outcome).src/core/algorithms/locate.rs (1)
2297-2299: Minor: preferFastHashSet::default()for consistency with the rest of the codebase.♻️ One-line fix
- let mut seen = std::collections::HashSet::new(); + let mut seen = FastHashSet::default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/algorithms/locate.rs` around lines 2297 - 2299, Replace the local HashSet instantiation used for deduping in the loop over extra_cells: change let mut seen = std::collections::HashSet::new(); to let mut seen = FastHashSet::default(); and ensure FastHashSet is imported into scope so the code compiles; keep the variable name seen and its usage in the for ck in &extra_cells loop unchanged.tests/large_scale_debug.rs (1)
460-466: Optional: simplifyNonZeroUsizeround-trip tothen_some.
NonZeroUsize::new(repair_every).map(NonZeroUsize::get)is correct but usesNonZeroUsizeas an indirect "0 ⇒ None" gate. The intent is clearer withthen_some.♻️ Proposed simplification
- let validate_every = env_usize("DELAUNAY_LARGE_DEBUG_VALIDATE_EVERY").or_else(|| { - if matches!(debug_mode, DebugMode::Cadenced) { - NonZeroUsize::new(repair_every).map(NonZeroUsize::get) - } else { - None - } - }); + let validate_every = env_usize("DELAUNAY_LARGE_DEBUG_VALIDATE_EVERY").or_else(|| { + (matches!(debug_mode, DebugMode::Cadenced) && repair_every != 0).then_some(repair_every) + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/large_scale_debug.rs` around lines 460 - 466, The Option returned by NonZeroUsize::new(repair_every).map(NonZeroUsize::get) is a round‑trip that can be expressed more clearly; in the branch where you compute validate_every (inside the matches!(debug_mode, DebugMode::Cadenced) closure), replace NonZeroUsize::new(repair_every).map(NonZeroUsize::get) with a direct presence check that returns the original usize, e.g. NonZeroUsize::new(repair_every).map(|_| repair_every) or (repair_every != 0).then_some(repair_every) so the intent is clearer while keeping validate_every, debug_mode, DebugMode::Cadenced and repair_every as the referenced symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 2567-2575: The comment describing bulk construction is out of
date: it references repair_delaunay_with_flips_k2_k3 but the code now calls
repair_delaunay_local_single_pass; update the comment near
maybe_repair_after_insertion/insert_remaining_vertices_seeded to describe that
insertions call repair_delaunay_local_single_pass (single-pass local repair, no
topology check or heuristic rebuild, soft-fail behavior) and that
soft_fail_seeds is collected and later used by finalize_bulk_construction for a
global repair; ensure the wording matches the new helper name and its behavior.
---
Outside diff comments:
In `@src/core/delaunay_triangulation.rs`:
- Around line 2588-2607: The soft_fail_seeds Vec is never appended to when a
soft-fail path is taken for dimension D>=4, so finalize_bulk_construction
receives an empty list and seeded repair never runs; locate the soft-fail
handling branches in insert_remaining_vertices_seeded (and the other soft-fail
logging sites referenced around the blocks that currently only log for D>=4) and
add logic to push the failing CellKey (or the appropriate seed cell identifier
used elsewhere) into the mutable soft_fail_seeds vector whenever a per-insertion
soft-fail is detected; ensure you use the same CellKey type and preserve
ownership/borrows so the seed collection is populated before calling
finalize_bulk_construction.
In `@tests/large_scale_debug.rs`:
- Around line 17-48: The header env-var listing is missing the documented entry
for the environment variable used by debug_mode_from_env(); add a line
describing DELAUNAY_LARGE_DEBUG_DEBUG_MODE (what values it accepts and its
default) to the module-level comment block at the top of
tests/large_scale_debug.rs so the README-like bash snippet includes this
variable alongside the other DELAUNAY_LARGE_DEBUG_* entries; ensure the name
matches exactly (DELAUNAY_LARGE_DEBUG_DEBUG_MODE) and the description matches
the behavior implemented in debug_mode_from_env().
---
Duplicate comments:
In `@src/core/algorithms/locate.rs`:
- Around line 1219-1234: The open_cell extraction using
boundary_facets.get(...).ok_or_else(...).map(FacetHandle::cell_key)? is
correct—CellKey::default() only appears inside the unreachable
ConflictError::CellDataAccessFailed diagnostic path and is not propagated; no
code change is required for open_cell or its error handling (keep the current
usage of open_cell, FacetHandle::cell_key, and the ConflictError variants
as-is), and remove the duplicate review comment/marker if present.
- Around line 1256-1274: The loop indexing into boundary_facets with
boundary_facets[fi] can produce an opaque index-out-of-bounds panic in release
builds; change the access to use boundary_facets.get(fi).expect(...) (or similar
explicit check) to produce a clear, descriptive panic message that includes fi
and boundary_facets.len(), updating the loop over info.extra_facets and the
creation of ck (cell_key()) accordingly so the debug_assert remains but
release-mode errors are informative.
---
Nitpick comments:
In `@src/core/algorithms/locate.rs`:
- Around line 2297-2299: Replace the local HashSet instantiation used for
deduping in the loop over extra_cells: change let mut seen =
std::collections::HashSet::new(); to let mut seen = FastHashSet::default(); and
ensure FastHashSet is imported into scope so the code compiles; keep the
variable name seen and its usage in the for ck in &extra_cells loop unchanged.
In `@src/core/delaunay_triangulation.rs`:
- Around line 3063-3114: The code always calls repair_delaunay_local_single_pass
even when soft_fail_seeds (finalize_seeds) is empty, causing an unnecessary
O(cells) snapshot/clone; guard the repair block with a conditional like if
!soft_fail_seeds.is_empty() so that when finalize_seeds is empty you skip
calling repair_delaunay_local_single_pass and set repair_outcome = Ok(())
directly, otherwise compute global_repair_max_flips and invoke
repair_delaunay_local_single_pass as before (referencing
repair_delaunay_local_single_pass, finalize_seeds/soft_fail_seeds,
global_repair_max_flips, and repair_outcome).
In `@tests/large_scale_debug.rs`:
- Around line 460-466: The Option returned by
NonZeroUsize::new(repair_every).map(NonZeroUsize::get) is a round‑trip that can
be expressed more clearly; in the branch where you compute validate_every
(inside the matches!(debug_mode, DebugMode::Cadenced) closure), replace
NonZeroUsize::new(repair_every).map(NonZeroUsize::get) with a direct presence
check that returns the original usize, e.g.
NonZeroUsize::new(repair_every).map(|_| repair_every) or (repair_every !=
0).then_some(repair_every) so the intent is clearer while keeping
validate_every, debug_mode, DebugMode::Cadenced and repair_every as the
referenced symbols.
src/core/delaunay_triangulation.rs
Outdated
Comment on lines
+2567
to
+2575
| // Disable maybe_repair_after_insertion during bulk construction: its full pipeline | ||
| // (multi-pass repair + topology validation + heuristic rebuild) is too expensive | ||
| // per insertion. Instead, insert_remaining_vertices_seeded runs a targeted | ||
| // repair_delaunay_with_flips_k2_k3 call directly after each insertion (no topology | ||
| // check, no heuristic rebuild, soft-fail on non-convergence). Soft-failed | ||
| // insertions record their adjacent cells in soft_fail_seeds, which is used as the | ||
| // seed for the final global repair in finalize_bulk_construction. If no soft-fails | ||
| // occurred, the seed is empty and the global repair returns immediately with an | ||
| // empty queue (no work needed). |
Contributor
There was a problem hiding this comment.
Update the bulk-construction comment to match the new repair helper.
Line 2570 still references repair_delaunay_with_flips_k2_k3, but the code now calls repair_delaunay_local_single_pass. This mismatches the actual behavior and can mislead future changes.
✏️ Suggested comment fix
- // repair_delaunay_with_flips_k2_k3 call directly after each insertion (no topology
+ // repair_delaunay_local_single_pass call directly after each insertion (no topology
// check, no heuristic rebuild, soft-fail on non-convergence). Soft-failed
- // insertions record their adjacent cells in soft_fail_seeds, which is used as the
- // seed for the final global repair in finalize_bulk_construction. If no soft-fails
+ // insertions record their adjacent cells in soft_fail_seeds, which is used as the
+ // seed for the final seeded repair in finalize_bulk_construction. If no soft-fails
// occurred, the seed is empty and the global repair returns immediately with an
// empty queue (no work needed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/delaunay_triangulation.rs` around lines 2567 - 2575, The comment
describing bulk construction is out of date: it references
repair_delaunay_with_flips_k2_k3 but the code now calls
repair_delaunay_local_single_pass; update the comment near
maybe_repair_after_insertion/insert_remaining_vertices_seeded to describe that
insertions call repair_delaunay_local_single_pass (single-pass local repair, no
topology check or heuristic rebuild, soft-fail behavior) and that
soft_fail_seeds is collected and later used by finalize_bulk_construction for a
global repair; ensure the wording matches the new helper name and its behavior.
- Fix formatting in delaunay_triangulation.rs - Fix clippy unnested-or-patterns in delaunay_validation.rs - Add docs/KNOWN_ISSUES_4D.md documenting large-scale limitations - Document workarounds and future architectural work needed
Updates the known issues document for 4D bulk construction to improve clarity and formatting, specifically regarding Delaunay validation failures and repair convergence issues. Also, reformats a code block into a bash code block.
This was referenced Feb 27, 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.
Addresses a timeout issue in 4D bulk construction
by implementing per-insertion local Delaunay repair (soft-fail) during bulk construction to prevent
violation accumulation, which slows down the global repair process. Also adds a hard wall-clock time
limit to the test harness.
Refs: fix/4d-bulk-construction-timeout