Changed: Improves flip algorithm with topology index

[acgetchell]

Improves flip algorithm by introducing a topology index to efficiently check for duplicate cells and non-manifold facets. This avoids redundant scans of the triangulation data structure, especially during repair operations, by pre-computing and storing facet and cell signatures. This change is internal.

Refs: perf/optimize-flips

[coderabbitai]

Walkthrough

Adds deterministic, seed-searchable benchmark input generation with per-count seeding and retry-policy-backed triangulation construction; introduces an internal FlipTopologyIndex to detect duplicate/non-manifold cells in k-move flips; and clarifies docs/tests that Cell/Vertex/Point/Tds do not require CoordinateScalar at the type level.

Changes

Cohort / File(s)	Summary
Benchmarking Enhancements benches/ci_performance_suite.rs	Adds seed-search toggles (bench_seed_search_enabled, bench_seed_search_limit), per-count seed derivation, bounds tracking, Shuffled RetryPolicy via ConstructionOptions, replaces DelaunayTriangulation::new with new_with_options, and improves error/log messages to include seed and bounds.
Flip Topology Optimization src/core/algorithms/flips.rs	Introduces FlipTopologyIndex and CandidateFacetInfo, helper utils (sorted_vertex_key_values, cell_signature, build_flip_topology_index), switches duplicate/non-manifold checks in apply_bistellar_flip_with_k to use the topology index, refactors k=3 ridge-adjacency counting to SmallBuffer, and adds tests for duplicate/non-manifold rejection.
Documentation Clarifications src/core/cell.rs, src/core/vertex.rs, src/geometry/point.rs, src/core/triangulation_data_structure.rs	Rewords doc comments to state Cell/Vertex/Point/Tds do not require T: CoordinateScalar at the type level; clarifies layering (topology vs geometry/predicates) and serialization/validation gating.
Geometry API Change src/geometry/matrix.rs, src/geometry/predicates.rs, src/geometry/robust_predicates.rs, tests/circumsphere_debug_tools.rs	determinant now takes &Matrix instead of owning Matrix; updated call sites to determinant(&matrix) and adjusted error handling for non-finite determinants (NaN).
Tests tests/proptest_tds.rs	Adds tds_empty_does_not_require_coordinate_scalar to assert an empty Tds can be constructed with a non-scalar vertex type and basic combinatorial invariants hold.
Cargo / deps Cargo.toml	Reformatted smallvec features array and enabled fast-rng feature for uuid.

Sequence Diagram(s)

sequenceDiagram
    participant CI as CI Runner
    participant Bench as Bench Macro
    participant Gen as Input Generator
    participant DT as DelaunayTriangulation
    participant Retry as RetryPolicy

    rect rgba(200,200,255,0.5)
    CI->>Bench: invoke CI performance suite
    Bench->>Gen: derive base seed / per-count seed
    Gen-->>Bench: sample points (with bounds)
    Bench->>DT: DelaunayTriangulation::new_with_options(points, ConstructionOptions{RetryPolicy=Shuffled})
    DT->>Retry: attempt construction (may shuffle/retry)
    alt success
      DT-->>Bench: triangulation ready
      Bench-->>CI: report metrics / exit 0
    else failure & seed-search enabled
      Bench->>Bench: iterate candidate seeds up to limit
      loop until found or limit
        Bench->>Gen: generate points for candidate seed
        Bench->>DT: try new_with_options(...)
        DT->>Retry: retries...
      end
      alt found
        Bench-->>CI: report found seed / exit 0
      else not found
        Bench-->>CI: report last seed and bounds / exit 1
      end
    end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• perf(collections): add stack-allocated SmallBuffer types for D+1 operations #126: Introduces stack-allocated SmallBuffer/neighbor buffers — strongly related to SmallBuffer usage and HashMap → SmallBuffer refactor in flips.rs.
• feat: Memory profiling system and performance optimizations (v0.4.3) #68: Modifies benches/ci_performance_suite.rs for deterministic seeded input generation — directly related to benchmark seed/search and per-count seeding changes.
• perf: migrate geometry predicates to la-stack #155: Changes geometry matrix/predicate code (determinant/predicate call sites) — related to the determinant(&Matrix) signature and updated predicate call sites.

Poem

🐰 I hopped through meshes with seeds in paw,

signatures counted, no duplicate flaw,
bounds and retries all neat in a line,
seeds tried and found, or the last one I sign,
nibbling bugs, I code — a hop, a tiny rhyme.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: improving the flip algorithm with a topology index for efficient duplicate and non-manifold facet detection.
Description check	✅ Passed	The description is directly related to the changeset, explaining the introduction of a topology index to efficiently check for duplicate cells and non-manifold facets.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch perf/optimize-flips

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

benches/ci_performance_suite.rs (2)

88-152: Seed-search early return exits after the first matching count — document or iterate all matches.

When no filter is supplied (or a filter matches multiple counts), the return on line 137 exits the function after finding a seed for the first count only, silently skipping the remaining counts. The inline comment (lines 76-84) says "meant to be run with a Criterion filter that selects a single case," which is reasonable — but if a user forgets the filter, the silent partial completion could be confusing.

Consider either:

• Printing all successful seeds (replace return with continue) and returning after the loop, or
• Adding a diagnostic message when filters.is_empty() warning that only the first count will be searched.

This is minor since the feature is opt-in and documented inline.

---

120-125: Duplicated ConstructionOptions + RetryPolicy setup — consider extracting a helper.

The same ConstructionOptions::default().with_retry_policy(RetryPolicy::Shuffled { attempts: NonZeroUsize::new(6).expect(...), base_seed: Some(...) }) pattern appears in both the seed-search path (lines 120-125) and the normal benchmark path (lines 189-192). A small helper (e.g., fn bench_options(seed: u64) -> ConstructionOptions) would reduce duplication inside the macro.

Example helper

fn bench_options(seed: u64) -> ConstructionOptions {
    ConstructionOptions::default().with_retry_policy(RetryPolicy::Shuffled {
        attempts: NonZeroUsize::new(6).expect("retry attempts must be non-zero"),
        base_seed: Some(seed),
    })
}

Also applies to: 189-192

src/core/algorithms/flips.rs (3)

3812-3819: Hash-only dedup: silent false-positive risk on facet tracking.

The intentional hash-only dedup (no vertex-level fallback) means a 64-bit hash collision between two distinct internal facets would cause one to be silently dropped from candidate_facet_info. The dropped facet would then never be checked for existing-cell overlap, so a non-manifold flip could slip through undetected.

The comment correctly notes the collision probability is astronomically low, and in practice this is fine. However, if you ever want belt-and-suspenders, a debug_assert! verifying the vertex sets match on dedup would catch any collision during testing at zero release cost.

---

3726-3743: Const-generic capacity expression in SmallBuffer is worth a brief doc note.

{ MAX_PRACTICAL_DIMENSION_SIZE * MAX_PRACTICAL_DIMENSION_SIZE } as the inline capacity for candidate_facet_info is correct (upper-bounds the unique internal facets per flip), but could be puzzling to a future reader. A one-line comment explaining the bound (e.g., // Upper bound: (D+2−k) cells × (D−k) internal facets per cell) would help.

---

3951-3964: Parallel sorted arrays are correct but slightly redundant.

sorted_values (u64) and sorted_vertices (VertexKey) are sorted by the same key (as_ffi()), so they're kept in lock-step. sorted_vertices is only used for the facet_vertices trace output (line 3990). If tracing is rare, you could defer its construction into the if repair_trace_enabled() block. Very minor — current form is fine for clarity.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.12% (target: -1.00%)	✅ 73.57%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (e69f3d1)	10126	6382	63.03%
Head commit (d381a47)	10203 (+77)	6443 (+61)	63.15% (+0.12%)

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 (#194)	140	103	73.57%

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%

See your quality gate settings    Change summary preferences

[codecov]

Codecov Report

❌ Patch coverage is 73.57143% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.18%. Comparing base (e69f3d1) to head (d381a47).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/core/algorithms/flips.rs	72.93%	36 Missing ⚠️
src/geometry/robust_predicates.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
+ Coverage   63.05%   63.18%   +0.12%     
==========================================
  Files          46       46              
  Lines       10119    10196      +77     
==========================================
+ Hits         6381     6442      +61     
- Misses       3738     3754      +16     

Flag	Coverage Δ
unittests	63.18% <73.57%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/core/algorithms/flips.rs`:
- Around line 3895-3954: The function flip_would_create_nonmanifold_facets_any
sorts VertexKey values directly which mismatches the facet hash ordering used by
build_flip_topology_index; replace the current sort_unstable()/as_ffi extraction
with a call to sorted_vertex_key_values() to produce the sorted u64 key sequence
used for hashing (and use that sequence when building facet_values and computing
facet_hash against topology.candidate_facet_info); for tracing output
(facet_vertices) either reconstruct it from the sorted u64s or maintain a
parallel sorted VertexKey buffer using sort_unstable_by_key(|v|
v.data().as_ffi()) so logs remain meaningful.

🧹 Nitpick comments (1)

benches/ci_performance_suite.rs (1)

92-142: Seed search exits on the first successful count, not after validating all counts.

The std::process::exit(0) on line 130 terminates the entire process when any single (dim, count) succeeds. Since each count has its own derived seed (base + count), a seed that works for count=10 may fail for count=50. If the goal is to find a base seed that works for all counts in a given dimension, the search would need to validate all counts before reporting success.

This may be intentional (search per pair), but worth a brief comment in the code so future readers understand the intended usage.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@benches/ci_performance_suite.rs`:
- Line 28: The unresolved imports indicate ConstructionOptions, RetryPolicy, and
new_with_options are not publicly exported; either make them pub in
src/core/delaunay_triangulation.rs and add public re-exports (e.g., pub use
crate::core::delaunay_triangulation::{ConstructionOptions, RetryPolicy,
new_with_options};) in the crate's public API (lib.rs or prelude module), or
update the benchmark import to the actual public path where they are exported;
locate the symbols ConstructionOptions, RetryPolicy, and new_with_options in the
source and then either mark their definitions pub and re-export them from the
crate root/prelude or change the use line to the correct existing public module
path.

🧹 Nitpick comments (2)

benches/ci_performance_suite.rs (1)

140-151: std::process::exit bypasses destructors and Criterion cleanup.

The seed-search helper exits the process directly, which skips all Drop implementations (Criterion state, temp files, etc.). The inline comments document the intent well, but consider returning early or using panic! instead, which would at least unwind the stack. If exit is preferred for clean exit codes, this is acceptable as-is given it's behind an opt-in env var.

src/core/algorithms/flips.rs (1)

3793-3809: Hash-collision risk in candidate_facet_info dedup is negligible but undocumented.

Lines 3804–3808 skip inserting a facet entry when a matching hash already exists, and lines 3874–3878 use binary_search_by_key on the hash. If two distinct internal facets were to produce the same stable_hash_u64_slice value, one facet would be silently dropped—potentially causing a false negative in the non-manifold check (or a false positive if an unrelated existing facet collides with the tracked hash).

The probability is astronomically low (~2⁻⁶⁴ per pair) given FNV on short sorted u64 slices, so this is safe in practice. A brief inline comment noting the intentional hash-only comparison (no vertex-level tiebreak) would help future readers understand the trade-off.