diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 125b805f..7638abc8 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -204,7 +204,7 @@ jobs: run: | set -euo pipefail - results_file="benches/compare_results.txt" + results_file="benches/main_vs_release_compare_results.txt" # Successful compare step => no regressions beyond configured threshold. if [ "${{ steps.compare_regression.outcome }}" = "success" ]; then @@ -258,7 +258,7 @@ jobs: with: name: performance-regression-results-${{ github.run_number }} path: | - benches/compare_results.txt + benches/main_vs_release_compare_results.txt baseline-artifact/baseline_results.txt if-no-files-found: warn retention-days: 30 diff --git a/.gitignore b/.gitignore index 646d1065..26ecf47e 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,9 @@ /cobertura.xml /logs /benches/results +/benches/compare_results.txt +/benches/main_vs_release_compare_results.txt +/benches/worktree_vs_*_compare_results.txt /node_modules/ /.package-lock.json /package-lock.json diff --git a/CITATION.cff b/CITATION.cff index 6c6d3149..b4f650e3 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -19,10 +19,19 @@ keywords: - "simplicial-complex" - "triangulation" abstract: >- - A Rust library for d-dimensional Delaunay triangulations and convex hulls - inspired by CGAL. Features exact arithmetic predicates with Simulation of - Simplicity (SoS) for deterministic degeneracy resolution, a 4-level - validation hierarchy (element, structural, manifold, Delaunay), bistellar - flip editing and repair, and serialization support. Supports 2D through 5D - with safe Rust (no unsafe code). + Rust crate providing D-dimensional Delaunay triangulations and convex hulls + (2D through 5D explicitly tested) constructed with a PL-manifold (default) or + pseudomanifold guarantee on finite point sets with Euclidean and toroidal + global topologies. Uses exact predicates and Simulation of Simplicity for + robustness and degeneracy handling, and Hilbert curves for deterministic + insertion ordering and efficient spatial indexing. Provides an explicit + 4-level validation hierarchy on individual elements, triangulation data + structure validity, manifold topology, and Delaunay property adherence. + Allows for the complete set of Pachner moves up to D=5 using bistellar flips, + vertex insertion and deletion, and the conversion of non-Delaunay + triangulations into Delaunay triangulations via bounded flip/rebuilds. + Auxiliary data may be stored directly in vertices and simplices with external + secondary maps provided for vertex- and simplex-keyed algorithm use, and the + entire data structure is serializable/deserializable. Written in safe Rust + with no unsafe code. license: BSD-3-Clause diff --git a/README.md b/README.md index 471cc5c2..5e45b782 100644 --- a/README.md +++ b/README.md @@ -12,11 +12,22 @@ [![Audit dependencies](https://github.com/acgetchell/delaunay/actions/workflows/audit.yml/badge.svg)](https://github.com/acgetchell/delaunay/actions/workflows/audit.yml) [![Codacy Badge](https://app.codacy.com/project/badge/Grade/3cad94f994f5434d877ae77f0daee692)](https://app.codacy.com/gh/acgetchell/delaunay/dashboard?utm_source=gh&utm_medium=referral&utm_content=&utm_campaign=Badge_grade) -A reusable, tested, and benchmarked [Rust] software artifact for -dimension-generic Delaunay triangulations and convex hulls in finite Euclidean -and periodic point sets, with exact predicates, Simulation-of-Simplicity -degeneracy handling, [PL-manifold]-aware local moves, and exposed bistellar -flips ([Pachner moves]) inspired by [CGAL]. +[Rust] crate providing D-dimensional [Delaunay triangulations] and +[convex hulls][Convex hulls] (2D through 5D explicitly tested) constructed with +a [PL-manifold] (default) or [pseudomanifold][Pseudomanifold] guarantee on +finite point sets with Euclidean and toroidal global topologies. Uses +[exact predicates] and [Simulation of Simplicity] for robustness and degeneracy +handling, and [Hilbert curve]s for deterministic insertion ordering and +efficient spatial indexing. Provides an explicit +[4-level validation hierarchy][Validation Guide] on individual elements, +triangulation data structure validity, manifold topology, and Delaunay property +adherence. Allows for the complete set of [Pachner moves] up to D=5 using +bistellar flips, vertex insertion and deletion, and the conversion of +non-Delaunay triangulations into Delaunay triangulations via bounded +flip/rebuilds. Auxiliary data may be stored directly in vertices and simplices +with external [secondary maps][Secondary maps] provided for vertex- and +simplex-keyed algorithm use, and the entire data structure is +serializable/deserializable. Written in safe Rust with no unsafe code. ## πŸ“ Introduction @@ -91,9 +102,10 @@ complete technical background. ## ✨ Features - [x] Copyable data types associated with vertices and cells (integers, - floats, chars, custom enums) -- [x] d-dimensional [Delaunay triangulations] -- [x] d-dimensional [Convex hulls] + floats, chars, custom enums), plus `CellSecondaryMap` and + `VertexSecondaryMap` aliases for caller-owned key-indexed algorithm state +- [x] D-dimensional [Delaunay triangulations] +- [x] D-dimensional [Convex hulls] - [x] Bistellar flip / [Pachner moves] Edit API up to 5D: k-flips for k = 1, 2, 3 plus inverse moves - [x] [Delaunay repair] using bistellar flips for k=2/k=3 with inverse @@ -522,6 +534,10 @@ Portions of this library were developed with the assistance of these AI tools: [Voronoi diagrams]: https://en.wikipedia.org/wiki/Voronoi_diagram [Convex hulls]: https://en.wikipedia.org/wiki/Convex_hull [Hilbert curve]: https://en.wikipedia.org/wiki/Hilbert_curve +[exact predicates]: docs/numerical_robustness_guide.md +[Simulation of Simplicity]: docs/numerical_robustness_guide.md#simulation-of-simplicity-sos +[Secondary maps]: docs/workflows.md#builder-api-auxiliary-vertex-and-cell-data +[Validation Guide]: docs/validation.md [ChatGPT]: https://openai.com/chatgpt [Claude]: https://www.anthropic.com/claude [CodeRabbit]: https://coderabbit.ai/ diff --git a/benches/PERFORMANCE_RESULTS.md b/benches/PERFORMANCE_RESULTS.md index 9e62c9cf..16d90cca 100644 --- a/benches/PERFORMANCE_RESULTS.md +++ b/benches/PERFORMANCE_RESULTS.md @@ -179,11 +179,11 @@ insphere query performance independently from full triangulation workflows. ## Implementation Notes -### Performance Advantages of `insphere_lifted` +### Dimension-Dependent InSphere Predicate Performance -1. More efficient matrix formulation using relative coordinates -2. Avoids redundant circumcenter calculations -3. Optimized determinant computation +The tables above are the source of truth for predicate timing. `insphere_lifted` +shows advantages in lower dimensions such as 2D/3D, while `insphere_distance` +often wins in 4D/5D; boundary cases may favor `insphere` because of early exits. ## Benchmark Structure diff --git a/benches/README.md b/benches/README.md index e98709b5..aecbd228 100644 --- a/benches/README.md +++ b/benches/README.md @@ -20,6 +20,7 @@ predicates fast across 2D-5D. | Use Case | Command | |----------|---------| | Final local correctness check | `just ci` | +| Quick local large-scale wall-clock guard | `just perf-large-scale-smoke` | | Fast local PR performance guard with temporary same-machine baseline | `just perf-no-regressions` | | Full CI benchmark suite only | `just bench-ci` | | Persist/update the default local baseline artifact | `just perf-baseline` | @@ -52,11 +53,26 @@ The `perf` profile inherits from release and restores ThinLTO with instead: it catches formatting, lint, test, documentation, example, and benchmark-harness compile errors rather than publishing benchmark data. -`just perf-no-regressions [threshold]` is the recommended fast performance -check before pushing a PR that touches Rust or benchmark code: +`just perf-large-scale-smoke [max_secs]` is a coarse local wall-clock guard for +the release-mode large-scale harness. It runs the same 2D-5D cases as +`just debug-large-scale-{2,3,4,5}d` with their local defaults, caps each test +runtime at 60 seconds by default, and reports all failing dimensions before +exiting. This is useful while iterating, but it is not a Criterion comparison +and should not be treated as published benchmark data. Run it before pushing +Rust or benchmark changes to catch obvious local performance drift early. + +For ordinary Rust or benchmark pushes, use the quick smoke guard with the usual +correctness checks: ```bash just ci +just perf-large-scale-smoke +``` + +`just perf-no-regressions [threshold]` is the fuller branch-vs-main comparison +for performance-sensitive changes and PR-ready work: + +```bash just perf-no-regressions ``` @@ -298,7 +314,12 @@ uv run benchmark-utils compare-tags --old-tag vX.Y.Z --new-tag vA.B.C Generated summaries should come from fresh perf-profile runs when they are used as release evidence. For routine PR work, use `just ci` plus -`just perf-no-regressions`. +`just perf-no-regressions`. Release-baseline comparisons write +`benches/main_vs_release_compare_results.txt`; PR/ref comparisons write +`benches/worktree_vs__compare_results.txt`. The comparison commands print +a short pass/regression/error status and the report path before exiting. The +local PR/ref guard fails on total matched-time regressions, while individual +regressions and improvements are surfaced in the report. The generated `Triangulation Data Structure Performance` section is intentionally first: it is built from the current `target/criterion` construction results, diff --git a/benches/ci_performance_suite.rs b/benches/ci_performance_suite.rs index 1ce55f91..bf2ec00e 100644 --- a/benches/ci_performance_suite.rs +++ b/benches/ci_performance_suite.rs @@ -592,7 +592,7 @@ fn interior_facets_4d(dt: &FlipTriangulation4) -> Vec { let mut facets = Vec::new(); for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (facet_index, neighbor) in neighbors.iter().enumerate() { + for (facet_index, neighbor) in neighbors.enumerate() { if neighbor.is_some() { let Ok(facet_index) = u8::try_from(facet_index) else { continue; diff --git a/docs/code_organization.md b/docs/code_organization.md index 3498c5e3..a0e1c937 100644 --- a/docs/code_organization.md +++ b/docs/code_organization.md @@ -586,7 +586,7 @@ just test-allocation # Run allocation profiling tests just lint # All linting (code + docs + config) just lint-code # Code linting (Rust, Python, Shell) just lint-docs # Documentation linting (Markdown, Spelling) -just lint-config # Configuration validation (JSON, TOML, Actions) +just lint-config # Configuration checks (JSON, TOML, YAML/CFF, Actions) just clippy # Run Clippy with strict settings just doc-check # Validate documentation builds just python-check # Python format/lint/typecheck checks diff --git a/docs/dev/commands.md b/docs/dev/commands.md index 76826e31..ed40ced5 100644 --- a/docs/dev/commands.md +++ b/docs/dev/commands.md @@ -17,9 +17,10 @@ Agents must run appropriate checks after modifying code. - [Benchmark Profiles](#benchmark-profiles) - [Examples](#examples) - [Spell Checking](#spell-checking) -- [TOML Formatting](#toml-formatting) +- [TOML Checks](#toml-checks) - [Shell Script Validation](#shell-script-validation) - [JSON Validation](#json-validation) +- [CITATION.cff Validation](#citationcff-validation) - [GitHub Actions Validation](#github-actions-validation) - [Recommended Command Matrix](#recommended-command-matrix) - [CI Expectations](#ci-expectations) @@ -187,6 +188,13 @@ compile, lint, test, documentation, example, or benchmark-harness build errors. Use `just bench-smoke` only for quick harness validation with minimal samples; do not treat smoke output as performance data. +Use `just perf-large-scale-smoke [max_secs]` for a coarse local wall-clock guard +over the release-mode large-scale debug harness. It runs the same 2D-5D defaults +as `just debug-large-scale-{2,3,4,5}d`, caps each test runtime at 60 seconds by +default, and reports all failing dimensions before exiting. It does not compare +against a baseline and should not be treated as benchmark data. Run it before +pushing Rust or benchmark changes to catch obvious local performance drift early. + Use `just bench-perf-summary` from the release PR branch after version and documentation updates. It runs fresh perf-profile summary benchmarks, records the current Criterion construction metadata and generated simplex counts, and @@ -196,14 +204,32 @@ Before pushing Rust or benchmark changes, run: ```bash just ci +just perf-large-scale-smoke +``` + +For performance-sensitive changes and PR-ready work, also run: + +```bash just perf-no-regressions ``` -`just perf-no-regressions` is the fast local PR guard. It runs +`just perf-no-regressions` is the fuller local PR guard. It runs `ci_performance_suite` with the shared dev-mode Criterion arguments against a -temporary same-machine baseline generated from the current GitHub `main` ref. -The temporary baseline checkout and artifact directory are removed after the -comparison. +same-machine baseline generated from the current GitHub `main` ref. The guard +reuses a local cache under `baseline-artifacts/perf-no-regressions/` keyed by +the resolved `origin/main` commit and local Rust compiler version, and refreshes +that baseline when `main` or the compiler changes, or when the cached artifact +does not match the benchmark contract. The current worktree benchmark still runs +fresh each time so repeated comparisons can catch local performance drift. +The comparison report is written to +`benches/worktree_vs_main_compare_results.txt` by default so it is visibly a +branch/PR-vs-main check. The local guard exits nonzero only when benchmark +execution fails or total matched benchmark mean time regresses beyond the +threshold; individual benchmark regressions are warnings in the report. The +report also lists total, geomean, median, top regressions, and top improvements, +and the command prints a short terminal status with the report path. +`just clean` removes Criterion data under `target/`, but it does not remove this +local baseline cache. ```bash just perf-no-regressions @@ -211,7 +237,18 @@ just perf-no-regressions `just perf-baseline` is optional and intentionally persistent: use it only when you want to create or refresh `baseline-artifact/baseline_results.txt` for later -manual comparisons. +manual comparisons. `just perf-compare ` uses that release-baseline +workflow and writes `benches/main_vs_release_compare_results.txt` by default. +It follows the same terminal-status convention, but remains stricter: individual +benchmark regressions still make release-style comparisons fail. + +For lower-level workflows, `uv run benchmark-utils ensure-ref-baseline --ref + --dev` prints the cached/generated same-machine baseline path for a branch +or version tag, and `uv run benchmark-utils fetch-baseline --ref ` downloads +the GitHub Actions artifact instead. Use the generated local baseline for +same-machine regression checks; use the downloaded artifact when you explicitly +want CI-runner parity. `uv run benchmark-utils compare-ref --ref ` writes +`benches/worktree_vs__compare_results.txt` unless `--output` is supplied. To generate a scratch baseline without replacing the default artifact, write it somewhere else and compare directly: @@ -271,13 +308,15 @@ under: --- -## TOML Formatting +## TOML Checks -TOML files should be validated and formatted using Taplo. +TOML files should parse cleanly, pass Taplo linting, and match Taplo +formatting. Commands: ```bash +just toml-check just toml-lint just toml-fmt-check just toml-fmt @@ -302,10 +341,23 @@ Run via CI or `just` commands. JSON files should be validated after edits. -Example: +Run: + +```bash +just json-check +``` + +--- + +## CITATION.cff Validation + +Citation metadata should pass both YAML style linting and CFF schema +validation. + +Run: ```bash -jq empty file.json +just citation-check ``` --- diff --git a/docs/dev/tooling-alignment.md b/docs/dev/tooling-alignment.md index 85367f00..e0251c82 100644 --- a/docs/dev/tooling-alignment.md +++ b/docs/dev/tooling-alignment.md @@ -15,7 +15,11 @@ Both repositories now share the same core Rust and Python support-tooling loop: - `.markdownlint.json`, `.yamllint`, and `typos.toml` define documentation and configuration checks. Delaunay still uses Node-backed Markdownlint and Prettier-for-YAML today; a Rust-native `dprint`/`pretty_yaml` and Markdown - linter migration remains a future tooling change. + linter migration remains a future tooling change. `CITATION.cff` is + YAML-style-linted with `.yamllint` and schema-validated through an + exact-version `uvx --from cffconvert==2.0.0` invocation, keeping + `cffconvert`'s old `jsonschema` constraint isolated from Semgrep's newer + dependency requirements. - `justfile` is the local entry point for formatting, linting, tests, coverage, Semgrep, changelog, setup commands, and supported Cargo feature surface checks. @@ -73,13 +77,26 @@ Some causal-triangulations tooling remains project-specific and was not ported: - CDT's `performance-analysis` script; Delaunay keeps its benchmark helpers and generated performance-summary workflow. - Delaunay has a project-specific `just perf-no-regressions` recipe that runs - the calibrated 2D-5D `ci_performance_suite` canaries against the current - dev-mode baseline artifact. This stays local to Delaunay because the - benchmark contract, fixture sizes, and regression threshold are tied to this - library's triangulation performance expectations. `just perf-baseline [ref]` - generates that baseline on the developer's machine from a temporary checkout - of the requested GitHub ref so the comparison uses the same local hardware as - the current branch while GitHub Actions still publishes shared CI artifacts. + the calibrated 2D-5D `ci_performance_suite` canaries against a cached + same-machine dev-mode baseline for the current GitHub `main` commit. This + stays local to Delaunay because the benchmark contract, fixture sizes, and + regression threshold are tied to this library's triangulation performance + expectations. The cache lives under + `baseline-artifacts/perf-no-regressions/`, is keyed by the resolved + `origin/main` commit and local Rust compiler version, and is refreshed when + either key changes or the artifact no longer matches the benchmark contract. + The cache/validation logic lives in `benchmark-utils ensure-ref-baseline` and + `benchmark-utils compare-ref` so workflows can reuse the same behavior without + depending on justfile shell internals. + `compare-ref` writes branch/PR-vs-ref reports with explicit names such as + `benches/worktree_vs_main_compare_results.txt`, while release-baseline + comparisons use `benches/main_vs_release_compare_results.txt`. Ref + comparisons fail on total matched-time regressions and report individual + regressions/improvements as context; release-baseline comparisons remain + strict for individual regressions. + `just perf-baseline [ref]` remains the manual persistent-baseline workflow for + a requested GitHub ref, so ad-hoc comparisons still use the developer's local + hardware while GitHub Actions publishes shared CI artifacts. - Delaunay keeps a single `profiling_suite` benchmark target for manual large-scale and flamegraph work. The previous standalone large-scale target was folded into that harness so `.github/workflows/profiling-benchmarks.yml` diff --git a/docs/workflows.md b/docs/workflows.md index 847a2cf2..d5ec1948 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -283,6 +283,23 @@ assert_eq!(dt.tds().cell(cell_key).unwrap().data(), Some(&42)); `set_vertex_data` and `set_cell_data` are safe O(1) operations β€” they modify only the user-data field and do not invalidate geometry, topology, or Delaunay invariants. +For algorithm-local state keyed by existing vertices or cells, prefer the +caller-owned secondary-map aliases instead of mutating stored user data: + +```rust +use delaunay::prelude::collections::{CellSecondaryMap, VertexSecondaryMap}; + +let mut visited_cells: CellSecondaryMap = CellSecondaryMap::new(); +let mut vertex_order: VertexSecondaryMap = VertexSecondaryMap::new(); + +for (cell_key, _) in dt.cells() { + visited_cells.insert(cell_key, false); +} +for (order, (vertex_key, _)) in dt.vertices().enumerate() { + vertex_order.insert(vertex_key, order); +} +``` + ## Builder API: insertion statistics If you need observability (or you want to handle skipped vertices explicitly), use diff --git a/examples/delaunayize_repair.rs b/examples/delaunayize_repair.rs index bb0e02bf..6381eca4 100644 --- a/examples/delaunayize_repair.rs +++ b/examples/delaunayize_repair.rs @@ -163,7 +163,7 @@ fn flip_then_repair_2d() -> Result<(), DelaunayizeRepairExampleError> { let mut facets: Vec<_> = Vec::new(); for (ck, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (i, n) in neighbors.iter().enumerate() { + for (i, n) in neighbors.enumerate() { if let (Some(_), Ok(idx)) = (n, u8::try_from(i)) { facets.push(FacetHandle::new(ck, idx)); } diff --git a/examples/diagnostics.rs b/examples/diagnostics.rs index 9f555579..7944b48c 100644 --- a/examples/diagnostics.rs +++ b/examples/diagnostics.rs @@ -128,7 +128,7 @@ fn build_non_delaunay_triangulation_2d() for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (facet_index, neighbor) in neighbors.iter().enumerate() { + for (facet_index, neighbor) in neighbors.enumerate() { if neighbor.is_none() { continue; } diff --git a/examples/pachner_roundtrip_4d.rs b/examples/pachner_roundtrip_4d.rs index 1cbb8024..7cfe5d24 100644 --- a/examples/pachner_roundtrip_4d.rs +++ b/examples/pachner_roundtrip_4d.rs @@ -275,7 +275,7 @@ fn collect_interior_facets(dt: &Dt4) -> Vec { let mut facets = Vec::new(); for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (facet_index, neighbor) in neighbors.iter().enumerate() { + for (facet_index, neighbor) in neighbors.enumerate() { if neighbor.is_some() { let Ok(facet_index) = u8::try_from(facet_index) else { continue; diff --git a/examples/topology_editing_2d_3d.rs b/examples/topology_editing_2d_3d.rs index b13684c2..a84e25ac 100644 --- a/examples/topology_editing_2d_3d.rs +++ b/examples/topology_editing_2d_3d.rs @@ -560,7 +560,7 @@ fn find_interior_facet_2d>( ) -> Option { for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (facet_idx, neighbor) in neighbors.iter().enumerate() { + for (facet_idx, neighbor) in neighbors.enumerate() { if neighbor.is_some() { let Ok(facet_idx) = u8::try_from(facet_idx) else { continue; @@ -578,7 +578,7 @@ fn find_interior_facet_3d>( ) -> Option { for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (facet_idx, neighbor) in neighbors.iter().enumerate() { + for (facet_idx, neighbor) in neighbors.enumerate() { if neighbor.is_some() { let Ok(facet_idx) = u8::try_from(facet_idx) else { continue; diff --git a/justfile b/justfile index 65e74a5a..8b83ba9a 100644 --- a/justfile +++ b/justfile @@ -198,6 +198,10 @@ ci-baseline ref="main": ci-slow: ci test-slow @echo "βœ… CI + slow tests passed!" +# Validate CITATION.cff against the Citation File Format schema. +citation-check: _ensure-uv + uvx --from cffconvert==2.0.0 cffconvert --validate -i CITATION.cff + # Clean build artifacts clean: cargo clean @@ -282,6 +286,7 @@ help-workflows: @echo " just bench-smoke # Smoke-test benchmark harnesses (minimal samples)" @echo " just bench # Run all benchmarks with perf profile (ThinLTO)" @echo " just bench-ci # CI regression benchmarks with perf profile (~5-10 min)" + @echo " just perf-large-scale-smoke [max_secs] # Quick pre-push 2D-5D wall-clock guard (default 60s)" @echo " just perf-no-regressions [threshold] # Fast pre-PR 2D-5D regression guard (default 7.5%)" @echo " just perf-baseline [ref] # Persist/update default local baseline (default: main)" @echo " just perf-baseline-to [ref] # Generate scratch baseline without replacing default" @@ -296,14 +301,28 @@ help-workflows: @echo "" @echo "Use 'just --list' for every granular recipe." +# Check JSON files parse cleanly. +json-check: _ensure-jq + #!/usr/bin/env bash + set -euo pipefail + files=() + while IFS= read -r -d '' file; do + files+=("$file") + done < <(git ls-files -z '*.json') + if [ "${#files[@]}" -gt 0 ]; then + printf '%s\0' "${files[@]}" | xargs -0 -n1 jq empty + else + echo "No JSON files found to check." + fi + # All linting: code + documentation + configuration lint: lint-code lint-docs lint-config # Code linting: Rust (fmt-check, clippy, docs, Semgrep) + Python (Ruff, Ty) + Shell scripts lint-code: fmt-check clippy doc-check semgrep semgrep-test python-lint shell-lint -# Configuration validation: JSON, TOML, YAML, GitHub Actions workflows -lint-config: validate-json toml-lint toml-fmt-check yaml-lint action-lint +# Configuration checks: JSON, TOML, YAML/CFF, GitHub Actions workflows +lint-config: json-check toml-check toml-lint toml-fmt-check yaml-lint citation-check action-lint # Documentation linting: Markdown + spell checking lint-docs: markdown-check spell-check @@ -354,7 +373,8 @@ perf-compare file threshold="7.5": _ensure-uv perf-help: @echo "Performance Analysis Commands:" - @echo " just perf-no-regressions # Fast pre-PR guard with a temporary same-machine main baseline" + @echo " just perf-large-scale-smoke # Quick pre-push 2D-5D wall-clock smoke guard" + @echo " just perf-no-regressions # Fast pre-PR guard with a cached same-machine main baseline" @echo " just perf-baseline [ref] # Persist/update baseline-artifact for a GitHub ref (default: main)" @echo " just perf-baseline-to [ref] # Generate a scratch baseline artifact without replacing the default" @echo " just perf-compare [threshold] # Compare current tree with a specific dev-mode baseline" @@ -368,7 +388,8 @@ perf-help: @echo " just profile-mem # Samply profile memory allocations (with count-allocations feature)" @echo "" @echo "Benchmark System (Delaunay-specific):" - @echo " just perf-no-regressions # Generate temporary main baseline, compare current tree, clean up" + @echo " just perf-large-scale-smoke # Pre-push guard using debug-large-scale 2D-5D with a short cap" + @echo " just perf-no-regressions # Reuse cached main baseline, compare current tree" @echo " just perf-baseline [ref] # Persist baseline-artifact/baseline_results.txt from a GitHub ref" @echo " just perf-baseline-to [ref] # Generate an alternate local baseline artifact directory" @echo " just perf-compare # Compare against a specific dev-mode baseline" @@ -384,6 +405,7 @@ perf-help: @echo " DELAUNAY_BENCH_SEED=N # Random seed (decimal or 0x-hex)" @echo "" @echo "Examples:" + @echo " just perf-large-scale-smoke # Run before pushing to catch obvious performance drift" @echo " just perf-no-regressions # Recommended local PR performance guard" @echo " just perf-baseline # Persist/update default local baseline for GitHub main" @echo " just perf-baseline v0.7.5 # Persist/update default local baseline for a release tag" @@ -395,56 +417,61 @@ perf-help: @echo " just profile 1.95 # Current tree on Rust 1.95" @echo " just profile 1.95 v0.7.5 # v0.7.5 code on Rust 1.95" -# Fast pre-PR performance guard against a temporary same-machine main baseline. -perf-no-regressions threshold="7.5": _ensure-uv +# Quick pre-push 2D-5D large-scale wall-clock smoke guard. +perf-large-scale-smoke max_secs="60": #!/usr/bin/env bash set -euo pipefail - relevant_worktree_dirty() { - if ! git diff --quiet -- src benches Cargo.toml Cargo.lock scripts/benchmark_utils.py; then - return 0 - fi - if ! git diff --cached --quiet -- src benches Cargo.toml Cargo.lock scripts/benchmark_utils.py; then - return 0 - fi - if [ -n "$(git ls-files --others --exclude-standard -- src benches Cargo.toml Cargo.lock scripts/benchmark_utils.py)" ]; then - return 0 + max_secs="{{max_secs}}" + if [[ ! "$max_secs" =~ ^[1-9][0-9]*$ ]]; then + echo "❌ max_secs must be a positive integer, got: $max_secs" >&2 + exit 2 + fi + + status=0 + failures=() + + run_case() { + local dimension="$1" + local test_name="$2" + local n_env="$3" + local n_points="$4" + local progress_every="$5" + + echo "" + echo "β–Ά ${dimension}: ${test_name} (${n_points} vertices, ${max_secs}s cap)" + if env \ + DELAUNAY_BULK_PROGRESS_EVERY="$progress_every" \ + DELAUNAY_LARGE_DEBUG_MAX_RUNTIME_SECS="$max_secs" \ + "$n_env=$n_points" \ + DELAUNAY_LARGE_DEBUG_REPAIR_EVERY=1 \ + cargo test --release --test large_scale_debug "$test_name" -- --ignored --exact --nocapture; then + echo "βœ… ${dimension} completed within the ${max_secs}s test-runtime cap" + else + local code=$? + echo "❌ ${dimension} failed or exceeded the ${max_secs}s test-runtime cap (exit ${code})" + failures+=("$dimension") + status=1 fi - return 1 } - current_commit="$(git rev-parse HEAD)" - remote_line="$(git ls-remote origin refs/heads/main || true)" - remote_main_commit="" - if [ -n "$remote_line" ]; then - read -r remote_main_commit _ <<< "$remote_line" - fi - if [ -n "$remote_main_commit" ] && [ "$remote_main_commit" = "$current_commit" ] && ! relevant_worktree_dirty; then - echo "πŸ” origin/main matches HEAD (${current_commit}); no relevant worktree changes to compare." - echo " Skipping perf-no-regressions before generating a same-commit baseline." - exit 0 - fi + run_case "2D" "debug_large_scale_2d" "DELAUNAY_LARGE_DEBUG_N_2D" "36000" "2000" + run_case "3D" "debug_large_scale_3d" "DELAUNAY_LARGE_DEBUG_N_3D" "8000" "500" + run_case "4D" "debug_large_scale_4d" "DELAUNAY_LARGE_DEBUG_N_4D" "900" "100" + run_case "5D" "debug_large_scale_5d" "DELAUNAY_LARGE_DEBUG_N_5D" "140" "20" - tmp="$(mktemp -d "${TMPDIR:-/tmp}/delaunay-perf-baseline.XXXXXX")" - trap 'rm -rf "$tmp"' EXIT - uv run benchmark-utils generate-ref-baseline --ref main --out "$tmp/baseline" --dev - baseline="$tmp/baseline/baseline_results.txt" - if ! grep -q 'Benchmark ID: tds_new_2d/tds_new/2000' "$baseline"; then - echo "❌ Temporary baseline for main does not match the current ci_performance_suite contract." - echo " The benchmark contract probably changed on this branch; inspect ci_performance_suite before comparing." - exit 1 - fi - baseline_line="$(grep -m1 '^Git commit:' "$baseline" || true)" - baseline_commit="${baseline_line#Git commit: }" - if [ -n "$baseline_commit" ] && [ "$baseline_commit" = "$current_commit" ]; then - if ! relevant_worktree_dirty; then - echo "πŸ” Current commit matches the main baseline (${baseline_commit}); no relevant worktree changes to compare." - echo " Skipping perf-no-regressions because a same-commit baseline would mask regressions." - exit 0 - fi - echo "⚠️ Main baseline commit matches HEAD, but relevant uncommitted changes exist; comparing the worktree against HEAD." + if (( ${#failures[@]} > 0 )); then + echo "" + echo "❌ Large-scale smoke guard failed for: ${failures[*]}" + exit "$status" fi - uv run benchmark-utils compare --baseline "$baseline" --threshold {{threshold}} --dev + + echo "" + echo "βœ… Large-scale smoke guard passed for 2D-5D" + +# Fast pre-PR performance guard against a cached same-machine main baseline. +perf-no-regressions threshold="7.5": _ensure-uv + uv run benchmark-utils compare-ref --ref main --threshold {{threshold}} --dev --output benches/worktree_vs_main_compare_results.txt # Run the selected CI benchmark suite for one compiler/code pair. profile toolchain="" code_ref="current": @@ -914,6 +941,20 @@ test-unit: cargo test --lib --verbose cargo test --doc --verbose +# Check TOML files parse cleanly. +toml-check: _ensure-uv + #!/usr/bin/env bash + set -euo pipefail + files=() + while IFS= read -r -d '' file; do + files+=("$file") + done < <(git ls-files -z '*.toml') + if [ "${#files[@]}" -gt 0 ]; then + printf '%s\0' "${files[@]}" | xargs -0 -I {} uv run python -c "import tomllib; tomllib.load(open('{}', 'rb')); print('{} is valid TOML')" + else + echo "No TOML files found to check." + fi + toml-fmt: _ensure-taplo #!/usr/bin/env bash set -euo pipefail @@ -957,33 +998,6 @@ toml-lint: _ensure-taplo unused-deps: _ensure-cargo-machete cargo machete -# File validation -validate-json: _ensure-jq - #!/usr/bin/env bash - set -euo pipefail - files=() - while IFS= read -r -d '' file; do - files+=("$file") - done < <(git ls-files -z '*.json') - if [ "${#files[@]}" -gt 0 ]; then - printf '%s\0' "${files[@]}" | xargs -0 -n1 jq empty - else - echo "No JSON files found to validate." - fi - -validate-toml: _ensure-uv - #!/usr/bin/env bash - set -euo pipefail - files=() - while IFS= read -r -d '' file; do - files+=("$file") - done < <(git ls-files -z '*.toml') - if [ "${#files[@]}" -gt 0 ]; then - printf '%s\0' "${files[@]}" | xargs -0 -I {} uv run python -c "import tomllib; tomllib.load(open('{}', 'rb')); print('{} is valid TOML')" - else - echo "No TOML files found to validate." - fi - verify-expect-counts: #!/usr/bin/env bash set -euo pipefail @@ -1046,9 +1060,9 @@ yaml-lint: _ensure-yamllint files=() while IFS= read -r -d '' file; do files+=("$file") - done < <(git ls-files -z '*.yml' '*.yaml') + done < <(git ls-files -z '*.yml' '*.yaml' 'CITATION.cff') if [ "${#files[@]}" -gt 0 ]; then - echo "πŸ” yamllint (${#files[@]} files)" + echo "πŸ” yamllint (${#files[@]} YAML/CFF files)" yamllint --strict -c .yamllint "${files[@]}" else echo "No YAML files found to lint." diff --git a/scripts/README.md b/scripts/README.md index db4cfd7e..9922c4bf 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -48,7 +48,10 @@ uv run benchmark-utils generate-summary --run-benchmarks --profile perf ``` `benchmark-utils` handles Criterion baseline generation, comparison, and -release performance summaries. +release performance summaries. The default comparison report for release +baselines is `benches/main_vs_release_compare_results.txt`; the ref-comparison +guard writes `benches/worktree_vs__compare_results.txt` and fails only on +total matched-time regressions or execution errors. ### Hardware utilities diff --git a/scripts/benchmark_utils.py b/scripts/benchmark_utils.py index 150d942c..64b1d688 100755 --- a/scripts/benchmark_utils.py +++ b/scripts/benchmark_utils.py @@ -30,7 +30,7 @@ from itertools import product from pathlib import Path from shutil import copy2 as copyfile # NOTE: Use copy2 (metadata-preserving) under the 'copyfile' alias for tests/patching convenience. -from typing import TYPE_CHECKING, TextIO +from typing import TYPE_CHECKING, Literal, NoReturn, TextIO from urllib.parse import urlparse from uuid import uuid4 @@ -39,6 +39,8 @@ logger = logging.getLogger(__name__) DEFAULT_REGRESSION_THRESHOLD = 7.5 +TIME_UNIT_TO_MICROSECONDS = {"ns": 1e-3, "Β΅s": 1.0, "ΞΌs": 1.0, "us": 1.0, "ms": 1e3, "s": 1e6} +ComparisonFailurePolicy = Literal["strict", "total-time"] class BaselineParseError(ValueError): @@ -172,6 +174,48 @@ def from_environment(cls) -> "BaselineArtifactMetadata": _CI_PERFORMANCE_SUITE_MANIFEST_IDS_FILE = "ci_performance_suite_manifest_ids.txt" _CI_PERFORMANCE_SUITE_METRICS_FILE = "ci_performance_suite_metrics.json" _CI_PERFORMANCE_SUITE_RUN_METADATA_FILE = "ci_performance_suite_run_metadata.json" +PERF_NO_REGRESSIONS_REQUIRED_BENCHMARK_ID = "tds_new_2d/tds_new/4000" +MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE = "main_vs_release_compare_results.txt" +WORKTREE_VS_REF_COMPARISON_RESULTS_TEMPLATE = "worktree_vs_{ref}_compare_results.txt" +PERF_NO_REGRESSIONS_RELEVANT_PATHS = ( + "src", + "benches", + "Cargo.toml", + "Cargo.lock", + "scripts/benchmark_utils.py", +) + + +@dataclass(frozen=True) +class BenchmarkTimeChange: + """Normalized timing comparison used by benchmark summary policies.""" + + label: str + current_mean_us: float + baseline_mean_us: float + time_change_pct: float + + +@dataclass(frozen=True) +class ComparisonFileRequest: + """Context for writing a benchmark comparison report.""" + + baseline_content: str + output_file: Path + dev_mode: bool + failure_policy: ComparisonFailurePolicy + + +@dataclass(frozen=True) +class ComparisonSummaryStats: + """Summary statistics for benchmark comparison failure policy decisions.""" + + total_time_change: float + geomean_change: float + median_change: float + individual_regressions: int + compared_count: int + failure_policy: ComparisonFailurePolicy def ci_suite_group_key(first_path_part: str) -> str | None: @@ -332,7 +376,7 @@ def _load_ci_performance_manifest_ids(criterion_dir: Path) -> set[str] | None: return manifest_ids or None -def _load_ci_performance_metrics(criterion_dir: Path) -> dict[str, dict[str, int]]: +def _load_ci_performance_metrics(criterion_dir: Path) -> dict[str, Mapping[str, object]]: """Load ci_performance_suite construction metrics when present.""" metrics_path = _ci_performance_metrics_path(criterion_dir) if not metrics_path.exists(): @@ -349,17 +393,12 @@ def _load_ci_performance_metrics(criterion_dir: Path) -> dict[str, dict[str, int msg = f"malformed ci_performance_suite metrics sidecar {metrics_path}: expected JSON object" raise TypeError(msg) - metrics: dict[str, dict[str, int]] = {} + metrics: dict[str, Mapping[str, object]] = {} for benchmark_id, values in data.items(): if not isinstance(benchmark_id, str) or not isinstance(values, dict): - msg = f"malformed ci_performance_suite metrics sidecar {metrics_path}: invalid entry {benchmark_id!r}" - raise TypeError(msg) - vertices = values.get("vertices") - simplices = values.get("simplices") - if not isinstance(vertices, int) or not isinstance(simplices, int): - msg = f"malformed ci_performance_suite metrics sidecar {metrics_path}: invalid counts for {benchmark_id!r}" - raise TypeError(msg) - metrics[benchmark_id] = {"vertices": vertices, "simplices": simplices} + logger.debug("Skipping malformed ci_performance_suite metric entry %r from %s", benchmark_id, metrics_path) + continue + metrics[benchmark_id] = values return metrics @@ -513,7 +552,7 @@ def __init__(self, project_root: Path) -> None: # Prefer CI artifact location; fall back to benches/ for local runs self.baseline_file = project_root / "baseline-artifact" / "baseline_results.txt" self._baseline_fallback = project_root / "benches" / "baseline_results.txt" - self.comparison_file = project_root / "benches" / "compare_results.txt" + self.comparison_file = release_comparison_results_path(project_root) # Path for storing Criterion benchmark results self.circumsphere_results_dir = project_root / "target" / "criterion" @@ -1850,11 +1889,11 @@ def _get_implementation_notes() -> list[str]: return [ "## Implementation Notes", "", - "### Performance Advantages of `insphere_lifted`", + "### Dimension-Dependent InSphere Predicate Performance", "", - "1. More efficient matrix formulation using relative coordinates", - "2. Avoids redundant circumcenter calculations", - "3. Optimized determinant computation", + "The tables above are the source of truth for predicate timing. `insphere_lifted`", + "shows advantages in lower dimensions such as 2D/3D, while `insphere_distance`", + "often wins in 4D/5D; boundary cases may favor `insphere` because of early exits.", "", ] @@ -1980,6 +2019,48 @@ def _ci_suite_input_points(path_parts: tuple[str, ...]) -> int | None: return int(path_parts[-1]) return None + @staticmethod + def _ci_suite_metric_simplices( + metric: Mapping[str, object] | None, + *, + benchmark_id: str, + path_parts: tuple[str, ...], + points: int | None, + dimension: str, + ) -> int | None: + """Return sidecar simplex counts only when they match the Criterion result.""" + if metric is None: + return None + + expected_dimension = ci_suite_dimension(benchmark_id) + expected_points = CriterionParser._ci_suite_input_points(path_parts) + if expected_dimension != dimension or expected_points != points: + logger.debug("Skipping stale ci_performance_suite metric for %s", benchmark_id) + return None + + vertices = metric.get("vertices") + simplices = metric.get("simplices") + if ( + not isinstance(vertices, int) + or isinstance(vertices, bool) + or not isinstance(simplices, int) + or isinstance(simplices, bool) + or simplices < 0 + ): + logger.debug("Skipping malformed ci_performance_suite metric for %s", benchmark_id) + return None + + if points is None or vertices != points: + logger.debug( + "Skipping stale ci_performance_suite metric for %s: vertices=%s, Criterion input=%s", + benchmark_id, + vertices, + points, + ) + return None + + return simplices + @staticmethod def _process_ci_performance_suite_results(criterion_dir: Path) -> list[BenchmarkData]: """Discover ci_performance_suite Criterion results with expanded benchmark IDs.""" @@ -1997,9 +2078,15 @@ def _process_ci_performance_suite_results(criterion_dir: Path) -> list[Benchmark continue benchmark_data.benchmark_id = benchmark_id - metric = metrics.get(benchmark_id) - if metric is not None: - benchmark_data.simplices = metric["simplices"] + metric_simplices = CriterionParser._ci_suite_metric_simplices( + metrics.get(benchmark_id), + benchmark_id=benchmark_id, + path_parts=path_parts, + points=points, + dimension=dimension, + ) + if metric_simplices is not None: + benchmark_data.simplices = metric_simplices results.append(benchmark_data) group_order = {group: index for index, group in enumerate(CI_PERFORMANCE_SUITE_GROUP_ORDER)} @@ -2373,6 +2460,347 @@ def generate_for_ref( return output_file +@dataclass(frozen=True) +class LocalRefBaselineCacheOptions: + """Options for a cached same-machine baseline generated from a git ref.""" + + ref_name: str = "main" + remote: str = "origin" + cache_root: Path | None = None + dev_mode: bool = False + bench_timeout: int = 1800 + required_benchmark_id: str = PERF_NO_REGRESSIONS_REQUIRED_BENCHMARK_ID + + +@dataclass(frozen=True) +class LocalRefBaselineCacheResult: + """Result of ensuring a cached same-machine ref baseline exists.""" + + baseline_path: Path + resolved_commit: str | None + reused: bool + + +def _sanitize_cache_component(value: str, *, fallback: str) -> str: + """Return a stable filesystem-safe cache component.""" + sanitized = _sanitize_ref_name(value.strip()) + return sanitized or fallback + + +def release_comparison_results_path(project_root: Path) -> Path: + """Return the release-baseline comparison report path.""" + return project_root / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE + + +def ref_comparison_results_path(project_root: Path, ref_name: str) -> Path: + """Return the worktree-vs-ref comparison report path for a git ref.""" + ref_key = _sanitize_cache_component(ref_name, fallback="ref") + return project_root / "benches" / WORKTREE_VS_REF_COMPARISON_RESULTS_TEMPLATE.format(ref=ref_key) + + +def _first_ls_remote_commit(stdout: str) -> str | None: + """Extract the first object id from git ls-remote output.""" + for line in stdout.splitlines(): + parts = line.split() + if parts and re.fullmatch(r"[0-9a-fA-F]+", parts[0]): + return parts[0] + return None + + +def _remote_ref_candidates(ref_name: str) -> list[str]: + """Return deterministic ls-remote candidates for a branch, tag, or full ref.""" + if ref_name.startswith("refs/"): + return [ref_name] + return [ + f"refs/heads/{ref_name}", + f"refs/tags/{ref_name}^{{}}", + f"refs/tags/{ref_name}", + ref_name, + ] + + +def _local_tracking_ref_candidates(remote: str, ref_name: str) -> list[str]: + """Return local remote-tracking refs that can stand in when offline.""" + if ref_name.startswith("refs/heads/"): + branch = ref_name.removeprefix("refs/heads/") + elif ref_name.startswith("refs/"): + return [] + else: + branch = ref_name + return [f"refs/remotes/{remote}/{branch}"] + + +def resolve_ref_commit(project_root: Path, *, ref_name: str, remote: str = "origin") -> str | None: + """Resolve a remote git ref to a commit-ish object id, falling back to local tracking refs.""" + for candidate in _remote_ref_candidates(ref_name): + result = run_git_command( + ["ls-remote", remote, candidate], + cwd=project_root, + check=False, + timeout=120, + ) + if result.returncode == 0: + commit = _first_ls_remote_commit(result.stdout) + if commit is not None: + return commit + else: + logger.debug("git ls-remote failed for %s/%s: %s", remote, candidate, (result.stderr or result.stdout or "").strip()) + break + + for candidate in _local_tracking_ref_candidates(remote, ref_name): + result = run_git_command( + ["rev-parse", "--verify", "--quiet", candidate], + cwd=project_root, + check=False, + timeout=30, + ) + if result.returncode == 0 and result.stdout.strip(): + return result.stdout.strip() + + return None + + +def _local_rustc_version(project_root: Path) -> str: + """Return the local rustc version used to key same-machine benchmark caches.""" + try: + result = run_safe_command("rustc", ["-V"], cwd=project_root, check=False, timeout=30) + except (ExecutableNotFoundError, OSError, subprocess.SubprocessError): + return "unknown-rustc" + if result.returncode == 0 and result.stdout.strip(): + return result.stdout.strip() + return "unknown-rustc" + + +def _default_local_ref_baseline_cache_root(project_root: Path) -> Path: + """Default cache root for local same-machine ref baselines.""" + if env_cache_root := os.getenv("DELAUNAY_PERF_BASELINE_CACHE"): + cache_root = Path(env_cache_root) + return cache_root if cache_root.is_absolute() else project_root / cache_root + return project_root / "baseline-artifacts" / "perf-no-regressions" + + +def _local_ref_baseline_cache_dir( + project_root: Path, + options: LocalRefBaselineCacheOptions, + *, + resolved_commit: str | None, +) -> Path: + """Return the deterministic cache directory for a local ref baseline.""" + cache_root = options.cache_root or _default_local_ref_baseline_cache_root(project_root) + if not cache_root.is_absolute(): + cache_root = project_root / cache_root + + ref_key = _sanitize_cache_component(options.ref_name, fallback="ref") + commit_key = _sanitize_cache_component(resolved_commit or options.ref_name, fallback="unresolved") + mode_key = "dev" if options.dev_mode else "full" + toolchain_key = _sanitize_cache_component(_local_rustc_version(project_root), fallback="unknown-rustc") + return cache_root / ref_key / commit_key / mode_key / toolchain_key + + +def _local_ref_baseline_candidates( + project_root: Path, + options: LocalRefBaselineCacheOptions, + *, + resolved_commit: str | None, +) -> list[Path]: + """Return primary and commit-alias cache candidates for a local ref baseline.""" + primary = _local_ref_baseline_cache_dir(project_root, options, resolved_commit=resolved_commit) / "baseline_results.txt" + if resolved_commit is None: + return [primary] + + cache_root = options.cache_root or _default_local_ref_baseline_cache_root(project_root) + if not cache_root.is_absolute(): + cache_root = project_root / cache_root + + commit_key = _sanitize_cache_component(resolved_commit, fallback="unresolved") + mode_key = "dev" if options.dev_mode else "full" + toolchain_key = _sanitize_cache_component(_local_rustc_version(project_root), fallback="unknown-rustc") + alias_pattern = f"*/{commit_key}/{mode_key}/{toolchain_key}/baseline_results.txt" + aliases = sorted(cache_root.glob(alias_pattern)) if cache_root.exists() else [] + + candidates = [primary] + candidates.extend(alias for alias in aliases if alias != primary) + return candidates + + +def _cached_baseline_valid( + project_root: Path, + baseline_path: Path, + *, + expected_commit: str | None, + required_benchmark_id: str, +) -> tuple[bool, str]: + """Validate cached baseline metadata and parseability before reuse.""" + if not baseline_path.exists(): + return False, f"missing baseline file: {baseline_path}" + + try: + baseline_content = baseline_path.read_text(encoding="utf-8") + except OSError as exc: + return False, f"unable to read baseline file {baseline_path}: {exc}" + + metadata = _parse_baseline_metadata(baseline_content) + if expected_commit is not None and metadata["commit"] != expected_commit: + return False, f"cached commit {metadata['commit']} does not match expected {expected_commit}" + + try: + baseline_results = PerformanceComparator(project_root).parse_baseline_file(baseline_content) + except BaselineParseError as exc: + return False, f"malformed baseline: {exc}" + + if not any(benchmark.benchmark_id == required_benchmark_id for benchmark in baseline_results.values()): + return False, f"missing required benchmark id {required_benchmark_id}" + + return True, "valid" + + +def ensure_cached_ref_baseline( + project_root: Path, + options: LocalRefBaselineCacheOptions, + *, + resolved_commit: str | None, +) -> LocalRefBaselineCacheResult: + """Ensure a cached same-machine baseline exists for a resolved git ref.""" + baseline_dir = _local_ref_baseline_cache_dir(project_root, options, resolved_commit=resolved_commit) + reason = "no cache candidates checked" + for baseline_path in _local_ref_baseline_candidates(project_root, options, resolved_commit=resolved_commit): + valid, reason = _cached_baseline_valid( + project_root, + baseline_path, + expected_commit=resolved_commit, + required_benchmark_id=options.required_benchmark_id, + ) + if not valid: + continue + + print(f"πŸ“¦ Reusing cached {options.ref_name} baseline: {baseline_path}", file=sys.stderr) + return LocalRefBaselineCacheResult(baseline_path=baseline_path, resolved_commit=resolved_commit, reused=True) + + print(f"πŸš€ Refreshing cached {options.ref_name} baseline ({reason})...", file=sys.stderr) + generator = LocalRefBaselineGenerator(project_root, remote=options.remote) + generated_path = generator.generate_for_ref( + ref_name=options.ref_name, + out_dir=baseline_dir, + dev_mode=options.dev_mode, + bench_timeout=options.bench_timeout, + ) + + valid, reason = _cached_baseline_valid( + project_root, + generated_path, + expected_commit=resolved_commit, + required_benchmark_id=options.required_benchmark_id, + ) + if not valid: + msg = f"Generated baseline for {options.ref_name} is not reusable: {reason}" + raise RuntimeError(msg) + + return LocalRefBaselineCacheResult(baseline_path=generated_path, resolved_commit=resolved_commit, reused=False) + + +def ensure_cached_ref_baseline_for_ref(project_root: Path, options: LocalRefBaselineCacheOptions) -> LocalRefBaselineCacheResult: + """Resolve a ref and ensure its cached same-machine baseline exists.""" + resolved_commit = resolve_ref_commit(project_root, ref_name=options.ref_name, remote=options.remote) + if resolved_commit is None: + print(f"⚠️ Could not resolve {options.remote}/{options.ref_name}; cache freshness cannot be verified.", file=sys.stderr) + return ensure_cached_ref_baseline(project_root, options, resolved_commit=resolved_commit) + + +def relevant_perf_worktree_dirty(project_root: Path, paths: tuple[str, ...] = PERF_NO_REGRESSIONS_RELEVANT_PATHS) -> bool: + """Return whether performance-relevant tracked or untracked paths changed.""" + diff_args = ["diff", "--quiet", "--", *paths] + for label, args in ( + ("unstaged diff", diff_args), + ("staged diff", ["diff", "--cached", "--quiet", "--", *paths]), + ): + result = run_git_command(args, cwd=project_root, check=False, timeout=60) + if result.returncode == 1: + return True + if result.returncode != 0: + details = (result.stderr or result.stdout or "").strip() + msg = f"git {label} failed with exit code {result.returncode}: {details}" + raise RuntimeError(msg) + + result = run_git_command( + ["ls-files", "--others", "--exclude-standard", "--", *paths], + cwd=project_root, + check=False, + timeout=60, + ) + if result.returncode != 0: + details = (result.stderr or result.stdout or "").strip() + msg = f"git ls-files for untracked perf paths failed with exit code {result.returncode}: {details}" + raise RuntimeError(msg) + return bool(result.stdout.strip()) + + +def compare_with_cached_ref_baseline( + project_root: Path, + options: LocalRefBaselineCacheOptions, + *, + threshold: float, + output_file: Path | None = None, +) -> int: + """Compare the current worktree against a cached same-machine ref baseline.""" + current_commit = get_git_commit_hash(cwd=project_root) + dirty = relevant_perf_worktree_dirty(project_root) + resolved_commit = resolve_ref_commit(project_root, ref_name=options.ref_name, remote=options.remote) + + if resolved_commit == current_commit and not dirty: + print(f"πŸ” {options.remote}/{options.ref_name} matches HEAD ({current_commit}); no relevant worktree changes to compare.") + print(" Skipping before generating a same-commit baseline.") + return 0 + + cache_result = ensure_cached_ref_baseline(project_root, options, resolved_commit=resolved_commit) + baseline_content = cache_result.baseline_path.read_text(encoding="utf-8") + baseline_commit = _parse_baseline_metadata(baseline_content)["commit"] + + if baseline_commit == current_commit: + if not dirty: + print(f"πŸ” Current commit matches the {options.ref_name} baseline ({baseline_commit}); no relevant worktree changes to compare.") + print(" Skipping because a same-commit baseline would mask regressions.") + return 0 + print(f"⚠️ {options.ref_name} baseline commit matches HEAD, but relevant uncommitted changes exist; comparing the worktree against HEAD.") + + if output_file is None: + output_file = ref_comparison_results_path(project_root, options.ref_name) + + comparator = PerformanceComparator(project_root) + comparator.regression_threshold = threshold + success, regression_found = comparator.compare_with_baseline( + cache_result.baseline_path, + dev_mode=options.dev_mode, + output_file=output_file, + failure_policy="total-time", + bench_timeout=options.bench_timeout, + ) + _display_comparison_result(output_file, success=success, regression_found=regression_found) + if not success: + return 1 + return 1 if regression_found else 0 + + +def _display_comparison_result(output_file: Path, *, success: bool, regression_found: bool) -> None: + """Print the comparison outcome and report path for command-line users.""" + if not success: + print(f"❌ Benchmark comparison failed; see {output_file}", file=sys.stderr) + return + + if regression_found: + print(f"⚠️ Performance regressions detected; see {output_file}", file=sys.stderr) + return + + try: + report_text = output_file.read_text(encoding="utf-8") + except OSError: + report_text = "" + if "INDIVIDUAL REGRESSION WARNING" in report_text: + print(f"βœ… Net performance OK; individual regression warnings in report: {output_file}") + return + + print(f"βœ… No significant performance regressions detected; report: {output_file}") + + class PerformanceComparator: """Compare current performance against baseline.""" @@ -2393,6 +2821,7 @@ def compare_with_baseline( dev_mode: bool = False, output_file: Path | None = None, bench_timeout: int = 1800, + failure_policy: ComparisonFailurePolicy = "strict", ) -> tuple[bool, bool]: """ Compare current performance against baseline. @@ -2400,14 +2829,15 @@ def compare_with_baseline( Args: baseline_file: Path to baseline file dev_mode: Use faster Criterion settings with the trusted Cargo profile - output_file: Output file path (default: benches/compare_results.txt) + output_file: Output file path (default: benches/main_vs_release_compare_results.txt) bench_timeout: Timeout for cargo bench commands in seconds + failure_policy: Regression policy for deciding the command exit status Returns: Tuple of (success, regression_found) """ if output_file is None: - output_file = self.project_root / "benches" / "compare_results.txt" + output_file = release_comparison_results_path(self.project_root) if not baseline_file.exists(): self._write_error_file(output_file, "Baseline file not found", baseline_file) @@ -2452,7 +2882,16 @@ def compare_with_baseline( baseline_results = self._parse_baseline_file(baseline_content) # Generate comparison report - regression_found = self._write_comparison_file(current_results, baseline_results, baseline_content, output_file, dev_mode=dev_mode) + regression_found = self._write_comparison_file( + current_results, + baseline_results, + ComparisonFileRequest( + baseline_content=baseline_content, + output_file=output_file, + dev_mode=dev_mode, + failure_policy=failure_policy, + ), + ) return True, regression_found @@ -2505,7 +2944,7 @@ def write_performance_comparison(self, f: TextIO, current_results: list[Benchmar """Public wrapper for writing the performance comparison section. Returns: - True if any individual benchmark or the overall average exceeds the + True if the selected failure policy detects a regression exceeding the regression threshold. """ return self._write_performance_comparison(f, current_results, baseline_results) @@ -2514,10 +2953,7 @@ def _write_comparison_file( self, current_results: list[BenchmarkData], baseline_results: dict[str, BenchmarkData], - baseline_content: str, - output_file: Path, - *, - dev_mode: bool = False, + request: ComparisonFileRequest, ) -> bool: """Write comparison results to file.""" logger.debug( @@ -2527,17 +2963,22 @@ def _write_comparison_file( len(baseline_results), ) # Prepare metadata - metadata = self._prepare_comparison_metadata(baseline_content) + metadata = self._prepare_comparison_metadata(request.baseline_content) # Prepare hardware comparison - hardware_report = self._prepare_hardware_comparison(baseline_content) - sampling_warning = self._sampling_warning(baseline_content, dev_mode=dev_mode) + hardware_report = self._prepare_hardware_comparison(request.baseline_content) + sampling_warning = self._sampling_warning(request.baseline_content, dev_mode=request.dev_mode) # Write comparison file - output_file.parent.mkdir(parents=True, exist_ok=True) - with output_file.open("w", encoding="utf-8") as f: + request.output_file.parent.mkdir(parents=True, exist_ok=True) + with request.output_file.open("w", encoding="utf-8") as f: self._write_comparison_header(f, metadata, hardware_report, sampling_warning=sampling_warning) - return self._write_performance_comparison(f, current_results, baseline_results) + return self._write_performance_comparison( + f, + current_results, + baseline_results, + failure_policy=request.failure_policy, + ) def _prepare_comparison_metadata(self, baseline_content: str) -> dict[str, str]: """Prepare metadata for comparison report.""" @@ -2645,10 +3086,18 @@ def _matching_baseline(current: BenchmarkData, baseline_results: dict[str, Bench return None return baseline_results.get(f"{current.points}_{current.dimension}") - def _write_performance_comparison(self, f: TextIO, current_results: list[BenchmarkData], baseline_results: dict[str, BenchmarkData]) -> bool: + def _write_performance_comparison( + self, + f: TextIO, + current_results: list[BenchmarkData], + baseline_results: dict[str, BenchmarkData], + *, + failure_policy: ComparisonFailurePolicy = "strict", + ) -> bool: """Write performance comparison section and return whether any regression exceeds threshold.""" - time_changes = [] # Track all time changes for average calculation + time_changes: list[BenchmarkTimeChange] = [] individual_regressions = 0 + individual_improvements = 0 for current_benchmark in current_results: baseline_benchmark = self._matching_baseline(current_benchmark, baseline_results) @@ -2660,53 +3109,65 @@ def _write_performance_comparison(self, f: TextIO, current_results: list[Benchma self._write_baseline_benchmark_data(f, baseline_benchmark) time_change, is_individual_regression = self._write_time_comparison(f, current_benchmark, baseline_benchmark) if time_change is not None: - time_changes.append(time_change) + mean_times = self._mean_times_us(current_benchmark, baseline_benchmark) + if mean_times is not None: + current_mean_us, baseline_mean_us = mean_times + time_changes.append( + BenchmarkTimeChange( + label=self._comparison_label(current_benchmark), + current_mean_us=current_mean_us, + baseline_mean_us=baseline_mean_us, + time_change_pct=time_change, + ), + ) if is_individual_regression: individual_regressions += 1 + elif time_change < -self.regression_threshold: + individual_improvements += 1 self._write_throughput_comparison(f, current_benchmark, baseline_benchmark) else: f.write("Baseline: N/A (no matching entry)\n") f.write("\n") - # Calculate and report average regression if time_changes: - # Prefer geometric mean of ratios to reflect multiplicative changes across benchmarks - ratios = [1.0 + (tc / 100.0) for tc in time_changes] - # Guard against non-positive ratios (defensive; should not occur with sane data) - positive_ratios = [r for r in ratios if r > 0] - if not positive_ratios: - average_change = 0.0 - else: - avg_log = sum(math.log(r) for r in positive_ratios) / len(positive_ratios) - avg_ratio = math.exp(avg_log) - average_change = (avg_ratio - 1.0) * 100.0 - logger.debug( - "Average change computed: %.2f%% with threshold %.2f%% across %s benchmarks", - average_change, - self.regression_threshold, - len(time_changes), - ) + total_current_us = sum(change.current_mean_us for change in time_changes) + total_baseline_us = sum(change.baseline_mean_us for change in time_changes) + total_time_change = ((total_current_us - total_baseline_us) / total_baseline_us) * 100.0 + geomean_change = self._geomean_time_change(time_changes) + median_change = self._median_time_change(time_changes) + f.write("\n=== SUMMARY ===\n") f.write(f"Total benchmarks compared: {len(time_changes)}\n") f.write(f"Individual regressions (>{self.regression_threshold}%): {individual_regressions}\n") - f.write(f"Average time change: {average_change:.1f}%\n") - # Optional: top regressions - top = sorted(time_changes, reverse=True)[:5] - if top: - f.write("Top regressions (by time change %): " + ", ".join(f"{t:.1f}%" for t in top) + "\n") + f.write(f"Individual improvements (>{self.regression_threshold}%): {individual_improvements}\n") + f.write(f"Total baseline matched mean time: {total_baseline_us:.3f} Β΅s\n") + f.write(f"Total current matched mean time: {total_current_us:.3f} Β΅s\n") + f.write(f"Total time change: {total_time_change:+.1f}%\n") + f.write(f"Geomean time change: {geomean_change:+.1f}%\n") + f.write(f"Median time change: {median_change:+.1f}%\n") + self._write_top_time_changes(f, "Top regressions", self._top_regressions(time_changes)) + self._write_top_time_changes(f, "Top improvements", self._top_improvements(time_changes)) regression_found = self._write_summary_status( f, - average_change=average_change, - individual_regressions=individual_regressions, - compared_count=len(time_changes), + ComparisonSummaryStats( + total_time_change=total_time_change, + geomean_change=geomean_change, + median_change=median_change, + individual_regressions=individual_regressions, + compared_count=len(time_changes), + failure_policy=failure_policy, + ), ) logger.debug( - "Performance comparison summary: individual_regressions=%s top_regressions=%s", + "Performance comparison summary: policy=%s total_change=%.2f%% geomean_change=%.2f%% median_change=%.2f%% individual_regressions=%s", + failure_policy, + total_time_change, + geomean_change, + median_change, individual_regressions, - top, ) f.write("\n") @@ -2714,57 +3175,136 @@ def _write_performance_comparison(self, f: TextIO, current_results: list[Benchma return False - def _write_summary_status(self, f: TextIO, *, average_change: float, individual_regressions: int, compared_count: int) -> bool: + @staticmethod + def _geomean_time_change(time_changes: list[BenchmarkTimeChange]) -> float: + """Return the geometric mean time change across matched benchmarks.""" + ratios = [1.0 + (change.time_change_pct / 100.0) for change in time_changes] + positive_ratios = [ratio for ratio in ratios if ratio > 0.0] + if not positive_ratios: + return 0.0 + avg_log = sum(math.log(ratio) for ratio in positive_ratios) / len(positive_ratios) + return (math.exp(avg_log) - 1.0) * 100.0 + + @staticmethod + def _median_time_change(time_changes: list[BenchmarkTimeChange]) -> float: + """Return the median time change across matched benchmarks.""" + sorted_changes = sorted(change.time_change_pct for change in time_changes) + midpoint = len(sorted_changes) // 2 + if len(sorted_changes) % 2 == 1: + return sorted_changes[midpoint] + return (sorted_changes[midpoint - 1] + sorted_changes[midpoint]) / 2.0 + + def _top_regressions(self, time_changes: list[BenchmarkTimeChange]) -> list[BenchmarkTimeChange]: + """Return the largest individual slowdowns beyond the regression threshold.""" + regressions = [change for change in time_changes if change.time_change_pct > self.regression_threshold] + return sorted(regressions, key=lambda change: change.time_change_pct, reverse=True)[:5] + + def _top_improvements(self, time_changes: list[BenchmarkTimeChange]) -> list[BenchmarkTimeChange]: + """Return the largest individual speedups beyond the improvement threshold.""" + improvements = [change for change in time_changes if change.time_change_pct < -self.regression_threshold] + return sorted(improvements, key=lambda change: change.time_change_pct)[:5] + + @staticmethod + def _write_top_time_changes(f: TextIO, title: str, changes: list[BenchmarkTimeChange]) -> None: + """Write a compact top-N timing change list.""" + if not changes: + return + f.write(f"{title}:\n") + f.writelines(f"- {change.label}: {change.time_change_pct:+.1f}%\n" for change in changes) + + def _write_summary_status(self, f: TextIO, summary: ComparisonSummaryStats) -> bool: """Write the summary status line and return whether the comparison failed.""" - average_regression_found = average_change > self.regression_threshold - if average_regression_found: + total_regression_found = summary.total_time_change > self.regression_threshold + if total_regression_found: f.write( - f"🚨 OVERALL REGRESSION: Average performance decreased by {average_change:.1f}% (exceeds {self.regression_threshold}% threshold)\n", + f"🚨 OVERALL REGRESSION: Total matched benchmark time increased by {summary.total_time_change:.1f}% " + f"(exceeds {self.regression_threshold}% threshold)\n", ) logger.warning( - "Average regression detected: average_change=%.2f%% threshold=%.2f%% benchmarks=%s", - average_change, + "Total-time regression detected: total_time_change=%.2f%% threshold=%.2f%% benchmarks=%s geomean=%.2f%% median=%.2f%%", + summary.total_time_change, self.regression_threshold, - compared_count, + summary.compared_count, + summary.geomean_change, + summary.median_change, ) return True - if individual_regressions > 0: + if summary.individual_regressions > 0: + if summary.failure_policy == "total-time": + f.write( + f"⚠️ INDIVIDUAL REGRESSION WARNING: {summary.individual_regressions} benchmark(s) exceeded " + f"{self.regression_threshold}% threshold while total matched time changed by {summary.total_time_change:.1f}%\n", + ) + logger.warning( + "Individual regressions warning under total-time policy: individual_regressions=%s " + "total_time_change=%.2f%% threshold=%.2f%% benchmarks=%s", + summary.individual_regressions, + summary.total_time_change, + self.regression_threshold, + summary.compared_count, + ) + return False + f.write( - f"⚠️ INDIVIDUAL REGRESSION: {individual_regressions} benchmark(s) exceeded " - f"{self.regression_threshold}% threshold while average changed by {average_change:.1f}%\n", + f"⚠️ INDIVIDUAL REGRESSION: {summary.individual_regressions} benchmark(s) exceeded " + f"{self.regression_threshold}% threshold while total matched time changed by {summary.total_time_change:.1f}%\n", ) logger.warning( - "Individual regression detected: individual_regressions=%s average_change=%.2f%% threshold=%.2f%% benchmarks=%s", - individual_regressions, - average_change, + "Individual regression detected: individual_regressions=%s total_time_change=%.2f%% threshold=%.2f%% benchmarks=%s", + summary.individual_regressions, + summary.total_time_change, self.regression_threshold, - compared_count, + summary.compared_count, ) return True - if average_change < -self.regression_threshold: + if summary.total_time_change < -self.regression_threshold: f.write( - f"πŸŽ‰ OVERALL IMPROVEMENT: Average performance improved by {abs(average_change):.1f}% " + f"πŸŽ‰ OVERALL IMPROVEMENT: Total matched benchmark time improved by {abs(summary.total_time_change):.1f}% " f"(exceeds {self.regression_threshold}% threshold)\n", ) logger.info( - "Average improvement detected: average_change=%.2f%% threshold=%.2f%% benchmarks=%s", - average_change, + "Total-time improvement detected: total_time_change=%.2f%% threshold=%.2f%% benchmarks=%s", + summary.total_time_change, self.regression_threshold, - compared_count, + summary.compared_count, ) return False - f.write(f"βœ… OVERALL OK: Average change within acceptable range (Β±{self.regression_threshold}%)\n") + f.write(f"βœ… OVERALL OK: Total matched time change within acceptable range (Β±{self.regression_threshold}%)\n") logger.debug( - "Average change within threshold: average_change=%.2f%% threshold=%.2f%% benchmarks=%s", - average_change, + "Total-time change within threshold: total_time_change=%.2f%% threshold=%.2f%% benchmarks=%s", + summary.total_time_change, self.regression_threshold, - compared_count, + summary.compared_count, ) return False + @staticmethod + def _comparison_label(benchmark: BenchmarkData) -> str: + """Return a stable label for summary timing change lists.""" + return benchmark.benchmark_id or f"{benchmark.points}_{benchmark.dimension}" + + @staticmethod + def _mean_time_us(benchmark: BenchmarkData) -> float | None: + """Return the benchmark mean time in microseconds when its unit is supported.""" + unit = benchmark.time_unit or "Β΅s" + scale = TIME_UNIT_TO_MICROSECONDS.get(unit) + if scale is None: + return None + return benchmark.time_mean * scale + + def _mean_times_us(self, current: BenchmarkData, baseline: BenchmarkData) -> tuple[float, float] | None: + """Return normalized current and baseline mean times for a valid comparison.""" + if baseline.time_mean <= 0: + return None + cur_mean_us = self._mean_time_us(current) + base_mean_us = self._mean_time_us(baseline) + if cur_mean_us is None or base_mean_us is None or base_mean_us <= 0: + return None + return cur_mean_us, base_mean_us + def _write_benchmark_header(self, f, benchmark: BenchmarkData) -> None: """Write benchmark section header.""" f.write(f"{benchmark.header_line()}\n") @@ -2794,19 +3334,16 @@ def _write_time_comparison(self, f, current: BenchmarkData, baseline: BenchmarkD if baseline.time_mean <= 0: f.write("Time Change: N/A (baseline mean is 0)\n") return None, False - # Normalize to microseconds when units differ (supports ns, Β΅s/ΞΌs/us, ms, s) - # Note: Both Β΅ (micro sign U+00B5) and ΞΌ (Greek mu U+03BC) symbols are supported - unit_scale = {"ns": 1e-3, "Β΅s": 1.0, "ΞΌs": 1.0, "us": 1.0, "ms": 1e3, "s": 1e6} cur_unit = current.time_unit or "Β΅s" base_unit = baseline.time_unit or "Β΅s" - if cur_unit not in unit_scale or base_unit not in unit_scale: + if cur_unit not in TIME_UNIT_TO_MICROSECONDS or base_unit not in TIME_UNIT_TO_MICROSECONDS: f.write(f"Time Change: N/A (unit mismatch: {cur_unit} vs {base_unit})\n") return None, False - cur_mean_us = current.time_mean * unit_scale[cur_unit] - base_mean_us = baseline.time_mean * unit_scale[base_unit] - if base_mean_us <= 0: + mean_times = self._mean_times_us(current, baseline) + if mean_times is None: f.write("Time Change: N/A (baseline mean is 0)\n") return None, False + cur_mean_us, base_mean_us = mean_times time_change_pct = ((cur_mean_us - base_mean_us) / base_mean_us) * 100 is_individual_regression = time_change_pct > self.regression_threshold @@ -3438,12 +3975,12 @@ def generate_summary() -> None: print(f"Skip reason: {skip_reason}") if baseline_exists == "true" and skip_benchmarks == "false": - results_file = Path("benches/compare_results.txt") + results_file = Path("benches") / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE if results_file.exists(): with results_file.open("r", encoding="utf-8") as f: content = f.read() if "❌ Error:" in content: - print("Result: ❌ Benchmark comparison failed (see benches/compare_results.txt for details)") + print(f"Result: ❌ Benchmark comparison failed (see {results_file} for details)") elif "REGRESSION" in content: print("Result: ⚠️ Performance regressions detected") # Set environment variable for machine consumption by CI systems @@ -3812,6 +4349,31 @@ def _add_benchmark_subcommands(subparsers: "argparse._SubParsersAction[argparse. ref_parser.add_argument("--project-root", type=Path, help="Project root containing the git repo (directory containing Cargo.toml)") ref_parser.set_defaults(validate_bench_timeout=True) + ensure_ref_parser = subparsers.add_parser("ensure-ref-baseline", help="Ensure a cached same-machine baseline exists for a git ref") + ensure_ref_parser.add_argument("--ref", dest="ref_name", type=str, default="main", help="Git ref to benchmark/cache (default: main)") + ensure_ref_parser.add_argument("--remote", type=str, default="origin", help="Git remote used to resolve/fetch the ref (default: origin)") + ensure_ref_parser.add_argument( + "--cache-root", + type=Path, + help="Cache root for local same-machine baselines (default: baseline-artifacts/perf-no-regressions)", + ) + ensure_ref_parser.add_argument( + "--required-benchmark-id", + default=PERF_NO_REGRESSIONS_REQUIRED_BENCHMARK_ID, + help=f"Benchmark ID required before reusing a cache entry (default: {PERF_NO_REGRESSIONS_REQUIRED_BENCHMARK_ID})", + ) + ensure_ref_parser.add_argument( + "--dev", action="store_true", help=f"Use faster Criterion settings while retaining the {TRUSTED_BENCH_PROFILE} Cargo profile" + ) + ensure_ref_parser.add_argument( + "--bench-timeout", + type=int, + default=get_default_bench_timeout(), + help="Timeout for cargo bench in seconds when refreshing the cache (from BENCHMARK_TIMEOUT env, default: 1800)", + ) + ensure_ref_parser.add_argument("--project-root", type=Path, help="Project root containing the git repo (directory containing Cargo.toml)") + ensure_ref_parser.set_defaults(validate_bench_timeout=True) + cmp_parser = subparsers.add_parser("compare", help="Compare current performance against baseline") cmp_parser.add_argument("--baseline", type=Path, required=True, help="Path to baseline file") cmp_parser.add_argument( @@ -3823,7 +4385,11 @@ def _add_benchmark_subcommands(subparsers: "argparse._SubParsersAction[argparse. cmp_parser.add_argument( "--dev", action="store_true", help=f"Use faster Criterion settings while retaining the {TRUSTED_BENCH_PROFILE} Cargo profile" ) - cmp_parser.add_argument("--output", type=Path, help="Output file path") + cmp_parser.add_argument( + "--output", + type=Path, + help=f"Output file path (default: benches/{MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE})", + ) cmp_parser.add_argument("--project-root", type=Path, help="Project root to benchmark (directory containing Cargo.toml)") cmp_parser.add_argument( "--bench-timeout", @@ -3833,6 +4399,42 @@ def _add_benchmark_subcommands(subparsers: "argparse._SubParsersAction[argparse. ) cmp_parser.set_defaults(validate_bench_timeout=True) + cmp_ref_parser = subparsers.add_parser("compare-ref", help="Compare current performance against a cached same-machine git-ref baseline") + cmp_ref_parser.add_argument("--ref", dest="ref_name", type=str, default="main", help="Git ref to benchmark/cache (default: main)") + cmp_ref_parser.add_argument("--remote", type=str, default="origin", help="Git remote used to resolve/fetch the ref (default: origin)") + cmp_ref_parser.add_argument( + "--cache-root", + type=Path, + help="Cache root for local same-machine baselines (default: baseline-artifacts/perf-no-regressions)", + ) + cmp_ref_parser.add_argument( + "--required-benchmark-id", + default=PERF_NO_REGRESSIONS_REQUIRED_BENCHMARK_ID, + help=f"Benchmark ID required before reusing a cache entry (default: {PERF_NO_REGRESSIONS_REQUIRED_BENCHMARK_ID})", + ) + cmp_ref_parser.add_argument( + "--threshold", + type=float, + default=DEFAULT_REGRESSION_THRESHOLD, + help=f"Regression threshold percentage for marking regressions (default: {DEFAULT_REGRESSION_THRESHOLD})", + ) + cmp_ref_parser.add_argument( + "--dev", action="store_true", help=f"Use faster Criterion settings while retaining the {TRUSTED_BENCH_PROFILE} Cargo profile" + ) + cmp_ref_parser.add_argument( + "--bench-timeout", + type=int, + default=get_default_bench_timeout(), + help="Timeout for cargo bench in seconds (from BENCHMARK_TIMEOUT env, default: 1800)", + ) + cmp_ref_parser.add_argument( + "--output", + type=Path, + help="Output file path (default: benches/worktree_vs__compare_results.txt)", + ) + cmp_ref_parser.add_argument("--project-root", type=Path, help="Project root containing the git repo (directory containing Cargo.toml)") + cmp_ref_parser.set_defaults(validate_bench_timeout=True) + def _add_local_baseline_subcommands(subparsers: "argparse._SubParsersAction[argparse.ArgumentParser]") -> None: """Add subcommands that operate on existing baseline artifacts/files.""" @@ -3925,7 +4527,12 @@ def _add_regression_subcommands(subparsers: "argparse._SubParsersAction[argparse regress_parser.set_defaults(validate_bench_timeout=True) results_parser = subparsers.add_parser("display-results", help="Display regression test results") - results_parser.add_argument("--results", type=Path, default=Path("benches/compare_results.txt"), help="Results file path") + results_parser.add_argument( + "--results", + type=Path, + default=Path("benches") / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE, + help="Results file path", + ) subparsers.add_parser("regression-summary", help="Generate regression testing summary") @@ -3974,51 +4581,116 @@ def configure_logging(*, verbose: bool) -> None: ) -def execute_baseline_commands(args: argparse.Namespace, project_root: Path) -> None: - """Execute baseline generation and comparison commands.""" - if args.command == "generate-baseline": - generator = BaselineGenerator(project_root, ref_name=args.ref_name) - success = generator.generate_baseline(dev_mode=args.dev, output_file=args.output, bench_timeout=args.bench_timeout) - sys.exit(0 if success else 1) +def _exit_called_process_error(error: subprocess.CalledProcessError) -> NoReturn: + print(f"❌ Git command failed with exit code {error.returncode}: {error.cmd}", file=sys.stderr) + if error.stderr: + print(error.stderr, file=sys.stderr) + if error.stdout: + print(error.stdout, file=sys.stderr) + sys.exit(1) - elif args.command == "generate-ref-baseline": - out_dir = args.out_dir if args.out_dir.is_absolute() else project_root / args.out_dir - try: - generator = LocalRefBaselineGenerator(project_root, remote=args.remote) - baseline_path = generator.generate_for_ref( - ref_name=args.ref_name, - out_dir=out_dir, - dev_mode=args.dev, - bench_timeout=args.bench_timeout, - ) - except subprocess.CalledProcessError as e: - print(f"❌ Git command failed with exit code {e.returncode}: {e.cmd}", file=sys.stderr) - if e.stderr: - print(e.stderr, file=sys.stderr) - if e.stdout: - print(e.stdout, file=sys.stderr) - sys.exit(1) - except _RECOVERABLE_CLI_ERRORS as e: - print(f"❌ {e}", file=sys.stderr) - sys.exit(1) - print(baseline_path) - sys.exit(0) +def _local_ref_cache_options_from_args(args: argparse.Namespace) -> LocalRefBaselineCacheOptions: + return LocalRefBaselineCacheOptions( + ref_name=args.ref_name, + remote=args.remote, + cache_root=args.cache_root, + dev_mode=args.dev, + bench_timeout=args.bench_timeout, + required_benchmark_id=args.required_benchmark_id, + ) + + +def _cmd_generate_baseline(args: argparse.Namespace, project_root: Path) -> None: + generator = BaselineGenerator(project_root, ref_name=args.ref_name) + success = generator.generate_baseline(dev_mode=args.dev, output_file=args.output, bench_timeout=args.bench_timeout) + sys.exit(0 if success else 1) - elif args.command == "compare": - comparator = PerformanceComparator(project_root) - comparator.regression_threshold = args.threshold - success, regression_found = comparator.compare_with_baseline( - args.baseline, + +def _cmd_generate_ref_baseline(args: argparse.Namespace, project_root: Path) -> None: + out_dir = args.out_dir if args.out_dir.is_absolute() else project_root / args.out_dir + try: + generator = LocalRefBaselineGenerator(project_root, remote=args.remote) + baseline_path = generator.generate_for_ref( + ref_name=args.ref_name, + out_dir=out_dir, dev_mode=args.dev, - output_file=args.output, bench_timeout=args.bench_timeout, ) + except subprocess.CalledProcessError as e: + _exit_called_process_error(e) + except _RECOVERABLE_CLI_ERRORS as e: + print(f"❌ {e}", file=sys.stderr) + sys.exit(1) - if not success: - sys.exit(1) + print(baseline_path) + sys.exit(0) - sys.exit(1 if regression_found else 0) + +def _cmd_ensure_ref_baseline(args: argparse.Namespace, project_root: Path) -> None: + options = _local_ref_cache_options_from_args(args) + try: + cache_result = ensure_cached_ref_baseline_for_ref(project_root, options) + except subprocess.CalledProcessError as e: + _exit_called_process_error(e) + except _RECOVERABLE_CLI_ERRORS as e: + print(f"❌ {e}", file=sys.stderr) + sys.exit(1) + + print(cache_result.baseline_path) + sys.exit(0) + + +def _cmd_compare(args: argparse.Namespace, project_root: Path) -> None: + comparator = PerformanceComparator(project_root) + comparator.regression_threshold = args.threshold + output_file = args.output or release_comparison_results_path(project_root) + success, regression_found = comparator.compare_with_baseline( + args.baseline, + dev_mode=args.dev, + output_file=output_file, + bench_timeout=args.bench_timeout, + ) + _display_comparison_result(output_file, success=success, regression_found=regression_found) + + if not success: + sys.exit(1) + + sys.exit(1 if regression_found else 0) + + +def _cmd_compare_ref(args: argparse.Namespace, project_root: Path) -> None: + options = _local_ref_cache_options_from_args(args) + try: + exit_code = compare_with_cached_ref_baseline( + project_root, + options, + threshold=args.threshold, + output_file=args.output, + ) + except subprocess.CalledProcessError as e: + _exit_called_process_error(e) + except _RECOVERABLE_CLI_ERRORS as e: + print(f"❌ {e}", file=sys.stderr) + sys.exit(1) + + sys.exit(exit_code) + + +def execute_baseline_commands(args: argparse.Namespace, project_root: Path) -> None: + """Execute baseline generation and comparison commands.""" + handlers = { + "generate-baseline": _cmd_generate_baseline, + "generate-ref-baseline": _cmd_generate_ref_baseline, + "ensure-ref-baseline": _cmd_ensure_ref_baseline, + "compare": _cmd_compare, + "compare-ref": _cmd_compare_ref, + } + handler = handlers.get(args.command) + if handler is None: + msg = f"Unknown baseline command: {args.command}" + raise ValueError(msg) + handler(args, project_root) def _write_optional_report(output_path: Path | None, report_text: str) -> None: @@ -4217,7 +4889,7 @@ def execute_regression_commands(args: argparse.Namespace) -> None: def execute_command(args: argparse.Namespace, project_root: Path) -> None: """Execute the selected command based on parsed arguments.""" # Try baseline commands first - if args.command in ("generate-baseline", "generate-ref-baseline", "compare"): + if args.command in ("generate-baseline", "generate-ref-baseline", "ensure-ref-baseline", "compare", "compare-ref"): execute_baseline_commands(args, project_root) return diff --git a/scripts/tests/test_benchmark_utils.py b/scripts/tests/test_benchmark_utils.py index 5d5f12cd..ef8b155b 100644 --- a/scripts/tests/test_benchmark_utils.py +++ b/scripts/tests/test_benchmark_utils.py @@ -3,7 +3,7 @@ Test suite for benchmark_utils.py module. Tests benchmark parsing, baseline generation, and performance comparison functionality, -with special focus on the new average regression calculation logic. +with special focus on benchmark regression policy and summary calculations. Note: This test file accesses private methods (prefixed with _) which is expected and necessary for comprehensive unit testing of internal functionality. @@ -36,12 +36,16 @@ _CI_PERFORMANCE_SUITE_RUN_METADATA_FILE, DEFAULT_REGRESSION_THRESHOLD, DEV_MODE_BENCH_ARGS, + MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE, TRUSTED_BENCH_PROFILE, + WORKTREE_VS_REF_COMPARISON_RESULTS_TEMPLATE, BaselineArtifactMetadata, BaselineGenerator, BaselineParseError, BenchmarkRegressionHelper, CriterionParser, + LocalRefBaselineCacheOptions, + LocalRefBaselineCacheResult, LocalRefBaselineGenerator, PerformanceComparator, PerformanceSummaryGenerator, @@ -51,8 +55,10 @@ _load_ci_performance_metrics, _parse_ci_performance_metrics, _write_ci_performance_metrics, + compare_with_cached_ref_baseline, configure_logging, create_argument_parser, + ensure_cached_ref_baseline, find_project_root, main, ) @@ -116,6 +122,23 @@ def write_ci_performance_metrics(target_dir: Path, metrics: dict[str, dict[str, ) +def cached_ref_baseline_content(*, commit: str = "abc123def456", benchmark_id: str = "tds_new_2d/tds_new/4000") -> str: + """Return a minimal parseable local ref baseline fixture.""" + return f"""Date: 2026-05-14 10:00:00 UTC +Git commit: {commit} +Ref: main +Hardware Information: + OS: macOS + CPU: Apple M4 Max + Memory: 64.0 GB + +=== 4000 Points (2D) === +Benchmark ID: {benchmark_id} +Time: [100.0, 110.0, 120.0] Β΅s +Throughput: [18.0, 19.0, 20.0] Kelem/s +""" + + def compute_average_time_change(current_results, baseline_results) -> float: """Replicate PerformanceComparator's geometric mean logic for tests.""" time_changes = [] @@ -491,6 +514,47 @@ def test_find_criterion_results_preserves_ci_suite_ids(self) -> None: assert roundtrip.dimension == "4D" assert roundtrip.throughput_mean is None + def test_find_criterion_results_skips_stale_ci_suite_metrics(self) -> None: + """Test ci_performance_suite simplex counts require matching current inputs.""" + with tempfile.TemporaryDirectory() as temp_dir: + target_dir = Path(temp_dir) / "target" + benchmark_id = "tds_new_2d/tds_new/10" + write_estimate(target_dir, tuple(benchmark_id.split("/")), 10_000.0) + write_ci_performance_metrics( + target_dir, + { + benchmark_id: { + "vertices": 25, + "simplices": 999, + }, + }, + ) + + results = CriterionParser.find_criterion_results(target_dir) + + assert len(results) == 1 + assert results[0].comparison_key == benchmark_id + assert results[0].points == 10 + assert results[0].simplices is None + + def test_find_criterion_results_skips_malformed_ci_suite_metrics(self) -> None: + """Test malformed ci_performance_suite simplex counts are not applied.""" + with tempfile.TemporaryDirectory() as temp_dir: + target_dir = Path(temp_dir) / "target" + benchmark_id = "tds_new_2d/tds_new/10" + write_estimate(target_dir, tuple(benchmark_id.split("/")), 10_000.0) + metrics_path = target_dir / "criterion" / _CI_PERFORMANCE_SUITE_METRICS_FILE + metrics_path.write_text( + json.dumps({benchmark_id: {"vertices": 10, "simplices": "stale"}}), + encoding="utf-8", + ) + + results = CriterionParser.find_criterion_results(target_dir) + + assert len(results) == 1 + assert results[0].comparison_key == benchmark_id + assert results[0].simplices is None + def test_find_criterion_results_filters_stale_ci_suite_ids_with_manifest(self) -> None: """Test ci_performance_suite parsing ignores stale Criterion files outside the manifest.""" with tempfile.TemporaryDirectory() as temp_dir: @@ -845,7 +909,7 @@ def test_compare_omits_quiet_flag(self, mock_cargo, dev_mode) -> None: assert mock_cargo.call_args.kwargs.get("capture_output") is True def test_write_performance_comparison_detects_individual_regression(self, comparator) -> None: - """Test performance comparison fails for individual regressions even when average is fine.""" + """Test strict performance comparison fails for individual regressions even when total time is fine.""" # Create current results with mixed performance changes current_results = [ # Big regression: +20% @@ -866,19 +930,52 @@ def test_write_performance_comparison_detects_individual_regression(self, compar output = StringIO() regression_found = comparator._write_performance_comparison(output, current_results, baseline_results) - # Average change using geometric mean: ~0.0%, but the +20% individual - # regression is still a workflow failure. + # Total matched time improves, but strict comparisons still fail on the +20% + # individual regression. assert regression_found result = output.getvalue() assert "SUMMARY" in result assert "Total benchmarks compared: 3" in result assert f"Individual regressions (>{THRESHOLD_PERCENT}): 1" in result # Only the +20% one - assert re.search(r"Average time change:\s*-?0\.0%", result) + assert "Total time change: -3.3%" in result + assert re.search(r"Geomean time change:\s*-?0\.0%", result) assert "⚠️ INDIVIDUAL REGRESSION" in result - def test_write_performance_comparison_with_average_regression(self, comparator) -> None: - """Test performance comparison with average regression exceeding threshold.""" + def test_total_time_policy_warns_on_individual_regressions_when_total_is_ok(self, comparator) -> None: + """Test local ref comparisons gate on total matched time while preserving warnings.""" + current_results = [ + BenchmarkData(1000, "2D").with_timing(45.0, 50.0, 55.0, "Β΅s"), + BenchmarkData(2000, "2D").with_timing(140.0, 150.0, 160.0, "Β΅s"), + ] + baseline_results = { + "1000_2D": BenchmarkData(1000, "2D").with_timing(95.0, 100.0, 105.0, "Β΅s"), + "2000_2D": BenchmarkData(2000, "2D").with_timing(95.0, 100.0, 105.0, "Β΅s"), + } + + output = StringIO() + regression_found = comparator._write_performance_comparison( + output, + current_results, + baseline_results, + failure_policy="total-time", + ) + + assert not regression_found + result = output.getvalue() + assert "Total time change: +0.0%" in result + assert "Geomean time change: -13.4%" in result + assert "Median time change: +0.0%" in result + assert f"Individual regressions (>{THRESHOLD_PERCENT}): 1" in result + assert f"Individual improvements (>{THRESHOLD_PERCENT}): 1" in result + assert "⚠️ INDIVIDUAL REGRESSION WARNING" in result + assert "Top regressions:" in result + assert "- 2000_2D: +50.0%" in result + assert "Top improvements:" in result + assert "- 1000_2D: -50.0%" in result + + def test_write_performance_comparison_with_total_regression(self, comparator) -> None: + """Test performance comparison with total matched-time regression exceeding threshold.""" # Create current results with overall performance degradation current_results = [ # Regression: +20% @@ -899,19 +996,19 @@ def test_write_performance_comparison_with_average_regression(self, comparator) output = StringIO() regression_found = comparator._write_performance_comparison(output, current_results, baseline_results) - # Average change using geometric mean: 11.0% - # This exceeds DEFAULT_REGRESSION_THRESHOLD, so overall regression found + # Total time change exceeds DEFAULT_REGRESSION_THRESHOLD, so overall regression is found. assert regression_found result = output.getvalue() assert "SUMMARY" in result assert "Total benchmarks compared: 3" in result assert f"Individual regressions (>{THRESHOLD_PERCENT}): 2" in result # The +20% and +15% ones - assert "Average time change: 11.0%" in result + assert "Total time change: +9.2%" in result + assert "Geomean time change: +11.0%" in result assert "🚨 OVERALL REGRESSION" in result - def test_write_performance_comparison_with_average_improvement(self, comparator) -> None: - """Test performance comparison with significant average improvement.""" + def test_write_performance_comparison_with_improvement(self, comparator) -> None: + """Test performance comparison with mixed improvements and no strict regression.""" # Create current results with overall performance improvement current_results = [ # Improvement: -10% @@ -932,8 +1029,7 @@ def test_write_performance_comparison_with_average_improvement(self, comparator) output = StringIO() regression_found = comparator._write_performance_comparison(output, current_results, baseline_results) - # Average change using geometric mean: -5.5% - # This is significant improvement, so no regression found + # Total matched time does not exceed the regression threshold. assert not regression_found result = output.getvalue() @@ -941,7 +1037,7 @@ def test_write_performance_comparison_with_average_improvement(self, comparator) assert "Total benchmarks compared: 3" in result assert f"Individual regressions (>{THRESHOLD_PERCENT}): 0" in result expected_average_change = compute_average_time_change(current_results, baseline_results) - expected_average_line = f"Average time change: {expected_average_change:.1f}%" + expected_average_line = f"Geomean time change: {expected_average_change:+.1f}%" assert expected_average_line in result assert "βœ… OVERALL OK" in result @@ -1229,6 +1325,145 @@ def fake_generate_baseline(self, *, dev_mode=False, output_file=None, bench_time assert calls[1] == ["fetch", "--depth", "1", "origin", "main"] assert calls[2] == ["checkout", "--detach", "FETCH_HEAD"] + def test_cached_ref_baseline_reuses_valid_cache(self, tmp_path) -> None: + """Test that a valid same-machine ref baseline is reused without benchmarking.""" + commit = "abc123def456" + options = LocalRefBaselineCacheOptions( + ref_name="main", + cache_root=tmp_path / "cache", + dev_mode=True, + ) + baseline_dir = tmp_path / "cache" / "main" / commit / "dev" / "rustc_1.95.0" + baseline_dir.mkdir(parents=True) + baseline_path = baseline_dir / "baseline_results.txt" + baseline_path.write_text(cached_ref_baseline_content(commit=commit), encoding="utf-8") + + with ( + patch("benchmark_utils.run_safe_command", return_value=completed_process(stdout="rustc 1.95.0\n")), + patch.object(LocalRefBaselineGenerator, "generate_for_ref") as mock_generate, + ): + result = ensure_cached_ref_baseline(tmp_path, options, resolved_commit=commit) + + assert result.baseline_path == baseline_path + assert result.resolved_commit == commit + assert result.reused is True + mock_generate.assert_not_called() + + def test_cached_ref_baseline_reuses_same_commit_alias(self, tmp_path) -> None: + """Test that refs resolving to the same commit can share a cached baseline.""" + commit = "abc123def456" + options = LocalRefBaselineCacheOptions( + ref_name="main", + cache_root=tmp_path / "cache", + dev_mode=True, + ) + alias_dir = tmp_path / "cache" / "v0.7.7" / commit / "dev" / "rustc_1.95.0" + alias_dir.mkdir(parents=True) + alias_path = alias_dir / "baseline_results.txt" + alias_path.write_text(cached_ref_baseline_content(commit=commit), encoding="utf-8") + + with ( + patch("benchmark_utils.run_safe_command", return_value=completed_process(stdout="rustc 1.95.0\n")), + patch.object(LocalRefBaselineGenerator, "generate_for_ref") as mock_generate, + ): + result = ensure_cached_ref_baseline(tmp_path, options, resolved_commit=commit) + + assert result.baseline_path == alias_path + assert result.resolved_commit == commit + assert result.reused is True + mock_generate.assert_not_called() + + def test_cached_ref_baseline_regenerates_stale_cache(self, tmp_path) -> None: + """Test that stale cache entries are refreshed and revalidated before reuse.""" + commit = "abc123def456" + options = LocalRefBaselineCacheOptions( + ref_name="main", + cache_root=tmp_path / "cache", + dev_mode=True, + bench_timeout=42, + ) + baseline_dir = tmp_path / "cache" / "main" / commit / "dev" / "rustc_1.95.0" + baseline_dir.mkdir(parents=True) + baseline_path = baseline_dir / "baseline_results.txt" + baseline_path.write_text(cached_ref_baseline_content(commit="stale-baseline"), encoding="utf-8") + + def fake_generate_for_ref( + self, + *, + ref_name: str, + out_dir: Path, + dev_mode: bool = False, + bench_timeout: int = 1800, + ) -> Path: + assert self.project_root == tmp_path + assert ref_name == "main" + assert out_dir == baseline_dir + assert dev_mode is True + assert bench_timeout == 42 + baseline_path.write_text(cached_ref_baseline_content(commit=commit), encoding="utf-8") + return baseline_path + + with ( + patch("benchmark_utils.run_safe_command", return_value=completed_process(stdout="rustc 1.95.0\n")), + patch.object(LocalRefBaselineGenerator, "generate_for_ref", fake_generate_for_ref), + ): + result = ensure_cached_ref_baseline(tmp_path, options, resolved_commit=commit) + + assert result.baseline_path == baseline_path + assert result.resolved_commit == commit + assert result.reused is False + + def test_compare_ref_skips_same_clean_commit(self, tmp_path) -> None: + """Test that compare-ref skips before generating a same-commit clean baseline.""" + options = LocalRefBaselineCacheOptions(ref_name="main", dev_mode=True) + + with ( + patch("benchmark_utils.get_git_commit_hash", return_value="abc123def456"), + patch("benchmark_utils.relevant_perf_worktree_dirty", return_value=False), + patch("benchmark_utils.resolve_ref_commit", return_value="abc123def456"), + patch("benchmark_utils.ensure_cached_ref_baseline") as mock_ensure, + ): + exit_code = compare_with_cached_ref_baseline(tmp_path, options, threshold=7.5) + + assert exit_code == 0 + mock_ensure.assert_not_called() + + def test_compare_ref_uses_ref_named_results_file(self, tmp_path, capsys) -> None: + """Test that compare-ref writes a worktree-vs-ref report by default.""" + commit = "abc123def456" + baseline_path = tmp_path / "baseline_results.txt" + baseline_path.write_text(cached_ref_baseline_content(commit=commit), encoding="utf-8") + options = LocalRefBaselineCacheOptions(ref_name="feature/perf-work", dev_mode=True) + + with ( + patch("benchmark_utils.get_git_commit_hash", return_value="current123"), + patch("benchmark_utils.relevant_perf_worktree_dirty", return_value=True), + patch("benchmark_utils.resolve_ref_commit", return_value=commit), + patch( + "benchmark_utils.ensure_cached_ref_baseline", + return_value=LocalRefBaselineCacheResult( + baseline_path=baseline_path, + resolved_commit=commit, + reused=True, + ), + ), + patch.object(PerformanceComparator, "compare_with_baseline", return_value=(True, False)) as mock_compare, + ): + exit_code = compare_with_cached_ref_baseline(tmp_path, options, threshold=7.5) + + expected_name = WORKTREE_VS_REF_COMPARISON_RESULTS_TEMPLATE.format(ref="feature_perf-work") + captured = capsys.readouterr() + assert exit_code == 0 + assert "No significant performance regressions detected" in captured.out + assert expected_name in captured.out + mock_compare.assert_called_once_with( + baseline_path, + dev_mode=True, + output_file=tmp_path / "benches" / expected_name, + failure_policy="total-time", + bench_timeout=1800, + ) + @patch("benchmark_utils.get_git_commit_hash", return_value="abc123") def test_written_baseline_round_trips_through_parser(self, mock_git, tmp_path) -> None: """Test the baseline writer/parser contract for representative records.""" @@ -1297,15 +1532,15 @@ def test_realistic_mixed_performance_scenario(self, comparator) -> None: output = StringIO() regression_found = comparator._write_performance_comparison(output, current_results, baseline_results) - # Average change using geometric mean: -0.2%, but the +8% individual - # regression is still a workflow failure. + # Strict comparisons fail on the +8% individual regression even though + # total matched time improves. assert regression_found result = output.getvalue() assert "Total benchmarks compared: 5" in result assert f"Individual regressions (>{THRESHOLD_PERCENT}): 1" in result # Only the +8% one expected_average_change = compute_average_time_change(current_results, baseline_results) - expected_average_line = f"Average time change: {expected_average_change:.1f}%" + expected_average_line = f"Geomean time change: {expected_average_change:+.1f}%" assert expected_average_line in result assert "⚠️ INDIVIDUAL REGRESSION" in result @@ -1333,14 +1568,13 @@ def test_gradual_performance_degradation_scenario(self, comparator) -> None: output = StringIO() regression_found = comparator._write_performance_comparison(output, current_results, baseline_results) - # Average change: 9.0% - # Should detect overall regression even though individual ones are mixed + # Should detect overall total-time regression. assert regression_found result = output.getvalue() assert "Total benchmarks compared: 5" in result assert f"Individual regressions (>{THRESHOLD_PERCENT}): 5" in result # All regressions exceed DEFAULT_REGRESSION_THRESHOLD - assert "Average time change: 9.0%" in result + assert "Total time change: +9.0%" in result assert "🚨 OVERALL REGRESSION" in result def test_noisy_benchmarks_scenario(self, comparator) -> None: @@ -1369,14 +1603,13 @@ def test_noisy_benchmarks_scenario(self, comparator) -> None: output = StringIO() regression_found = comparator._write_performance_comparison(output, current_results, baseline_results) - # Average change using geometric mean: 4.9%, but the one big outlier - # still needs to fail the benchmark comparison. + # Strict comparisons fail on the one big individual outlier. assert regression_found result = output.getvalue() assert "Total benchmarks compared: 5" in result assert f"Individual regressions (>{THRESHOLD_PERCENT}): 1" in result # Only the 40% outlier - assert "Average time change: 4.9%" in result + assert "Geomean time change: +4.9%" in result assert "⚠️ INDIVIDUAL REGRESSION" in result @@ -2183,7 +2416,7 @@ def test_display_results_file_missing(self, capsys) -> None: def test_generate_summary_with_regression(self, temp_chdir, capsys) -> None: """Test generating summary when regression is detected.""" with tempfile.TemporaryDirectory() as temp_dir: - results_file = Path(temp_dir) / "benches" / "compare_results.txt" + results_file = Path(temp_dir) / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE results_file.parent.mkdir(parents=True) results_file.write_text("REGRESSION detected in benchmark xyz") @@ -2196,7 +2429,7 @@ def test_generate_summary_with_regression(self, temp_chdir, capsys) -> None: "SKIP_REASON": "changes_detected", } - # Change working directory to temp_dir so Path("benches/compare_results.txt") works + # Change working directory to temp_dir so the default release comparison path resolves. with patch.dict(os.environ, env_vars), temp_chdir(temp_dir): BenchmarkRegressionHelper.generate_summary() @@ -2240,7 +2473,7 @@ def test_generate_summary_no_baseline(self, capsys) -> None: def test_generate_summary_sets_regression_environment_variable(self, temp_chdir, capsys) -> None: """Test that generate_summary sets BENCHMARK_REGRESSION_DETECTED environment variable when regressions are found.""" with tempfile.TemporaryDirectory() as temp_dir: - results_file = Path(temp_dir) / "benches" / "compare_results.txt" + results_file = Path(temp_dir) / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE results_file.parent.mkdir(parents=True) results_file.write_text("REGRESSION detected in benchmark xyz") @@ -2266,7 +2499,7 @@ def test_generate_summary_sets_regression_environment_variable(self, temp_chdir, def test_generate_summary_github_env_export(self, temp_chdir) -> None: """Test that BENCHMARK_REGRESSION_DETECTED is also exported to GITHUB_ENV when available.""" with tempfile.TemporaryDirectory() as temp_dir: - results_file = Path(temp_dir) / "benches" / "compare_results.txt" + results_file = Path(temp_dir) / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE results_file.parent.mkdir(parents=True) results_file.write_text("REGRESSION detected in benchmark xyz") @@ -2288,7 +2521,7 @@ def test_generate_summary_github_env_export(self, temp_chdir) -> None: def test_generate_summary_with_error_file(self, temp_chdir, capsys) -> None: """Test generating summary when comparison failed with error file.""" with tempfile.TemporaryDirectory() as temp_dir: - results_file = Path(temp_dir) / "benches" / "compare_results.txt" + results_file = Path(temp_dir) / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE results_file.parent.mkdir(parents=True) # Simulate error file content (as written by _write_error_file) results_file.write_text( @@ -2318,7 +2551,7 @@ def test_generate_summary_with_error_file(self, temp_chdir, capsys) -> None: # Should detect error and report failure, not "no regressions" assert "Result:" in captured.out assert "Benchmark comparison failed" in captured.out - assert "(see benches/compare_results.txt for details)" in captured.out + assert f"(see benches/{MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE} for details)" in captured.out # Should NOT say "no regressions" when there was an error assert "No significant performance regressions" not in captured.out @@ -2445,7 +2678,7 @@ def test_timeout_error_handling_performance_comparator(self, capsys) -> None: assert "Consider increasing --bench-timeout" in captured.err # Verify error file contains full exception message with command context - error_file = project_root / "benches" / "compare_results.txt" + error_file = project_root / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE assert error_file.exists() error_content = error_file.read_text() assert "❌ Error: Benchmark execution timeout" in error_content @@ -2507,6 +2740,25 @@ def test_parser_accepts_local_ref_baseline_generation(self) -> None: assert args.ref_name == "main" assert args.dev + def test_parser_accepts_cached_local_ref_baseline_commands(self) -> None: + """Test that cached local ref baseline commands expose reusable CLI options.""" + parser = create_argument_parser() + + ensure_args = parser.parse_args(["ensure-ref-baseline", "--ref", "v0.7.7", "--dev", "--cache-root", "cache"]) + compare_args = parser.parse_args( + ["compare-ref", "--ref", "main", "--threshold", "5.0", "--dev", "--output", "benches/worktree_vs_main_compare_results.txt"], + ) + + assert ensure_args.command == "ensure-ref-baseline" + assert ensure_args.ref_name == "v0.7.7" + assert ensure_args.cache_root == Path("cache") + assert ensure_args.dev + assert compare_args.command == "compare-ref" + assert compare_args.ref_name == "main" + assert compare_args.threshold == 5.0 + assert compare_args.output == Path("benches/worktree_vs_main_compare_results.txt") + assert compare_args.dev + def test_configure_logging_uses_debug_when_verbose(self) -> None: """Test that verbose mode configures debug-level CLI logging.""" with patch("benchmark_utils.logging.basicConfig") as mock_basic_config: @@ -2540,7 +2792,7 @@ def test_init(self) -> None: assert generator.project_root == project_root assert generator.baseline_file == project_root / "baseline-artifact" / "baseline_results.txt" assert generator._baseline_fallback == project_root / "benches" / "baseline_results.txt" - assert generator.comparison_file == project_root / "benches" / "compare_results.txt" + assert generator.comparison_file == project_root / "benches" / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE assert generator.circumsphere_results_dir == project_root / "target" / "criterion" assert isinstance(generator.current_version, str) assert isinstance(generator.current_date, str) @@ -2804,7 +3056,7 @@ def test_parse_comparison_results_with_regression(self) -> None: project_root = Path(temp_dir) benches_dir = project_root / "benches" benches_dir.mkdir(parents=True) - comparison_file = benches_dir / "compare_results.txt" + comparison_file = benches_dir / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE comparison_file.write_text(comparison_content) generator = PerformanceSummaryGenerator(project_root) @@ -2827,7 +3079,7 @@ def test_parse_comparison_results_no_regression(self) -> None: project_root = Path(temp_dir) benches_dir = project_root / "benches" benches_dir.mkdir(parents=True) - comparison_file = benches_dir / "compare_results.txt" + comparison_file = benches_dir / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE comparison_file.write_text(comparison_content) generator = PerformanceSummaryGenerator(project_root) @@ -3341,7 +3593,8 @@ def test_get_implementation_notes(self) -> None: content = "\n".join(lines) assert "## Implementation Notes" in content - assert "### Performance Advantages of `insphere_lifted`" in content + assert "### Dimension-Dependent InSphere Predicate Performance" in content + assert "`insphere_distance`" in content assert "### Method Disagreements" not in content def test_empty_benchmark_results_edge_case(self) -> None: @@ -3461,7 +3714,7 @@ def test_full_generation_workflow_integration(self) -> None: ) # Mock comparison file - comparison_file = baseline_dir / "compare_results.txt" + comparison_file = baseline_dir / MAIN_VS_RELEASE_COMPARISON_RESULTS_FILE comparison_file.write_text("βœ… OK: All benchmarks within acceptable range\n") with ( diff --git a/src/core/algorithms/flips.rs b/src/core/algorithms/flips.rs index 1b4ee4a5..c25ef88a 100644 --- a/src/core/algorithms/flips.rs +++ b/src/core/algorithms/flips.rs @@ -33,7 +33,7 @@ use crate::core::algorithms::incremental_insertion::{ wire_cavity_neighbors, }; use crate::core::algorithms::locate::{ConflictError, LocateError, extract_cavity_boundary}; -use crate::core::cell::{Cell, CellValidationError}; +use crate::core::cell::{Cell, CellValidationError, NeighborSlot}; use crate::core::collections::{ CellKeyBuffer, FastHashMap, FastHashSet, FastHasher, MAX_PRACTICAL_DIMENSION_SIZE, SmallBuffer, }; @@ -787,7 +787,7 @@ where ), }); } - let Some(incident_cell_key) = vertex.incident_cell else { + let Some(incident_cell_key) = vertex.incident_cell() else { continue; }; let incident_cell = @@ -819,7 +819,7 @@ where U: DataType, V: DataType, { - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_slots() else { return Ok(()); }; if neighbors.len() != D + 1 { @@ -832,21 +832,32 @@ where }); } - for (facet_idx, neighbor_key_opt) in neighbors.iter().enumerate() { - let Some(neighbor_key) = neighbor_key_opt else { - continue; + for (facet_idx, neighbor_slot) in neighbors.iter().copied().enumerate() { + let neighbor_key = match neighbor_slot { + NeighborSlot::Unassigned => { + return Err(TdsValidationFailure::InvalidNeighbors { + reason: NeighborValidationError::UnassignedNeighborSlot { + cell_key, + cell_uuid: cell.uuid(), + facet_index: facet_idx, + context: "flip trial neighbor validation".to_string(), + }, + }); + } + NeighborSlot::Boundary => continue, + NeighborSlot::Neighbor(neighbor_key) => neighbor_key, }; - if removed_cells.contains(neighbor_key) { + if removed_cells.contains(&neighbor_key) { return Err(TdsValidationFailure::InvalidNeighbors { reason: NeighborValidationError::ReferencedRemovedNeighbor { cell_key, cell_uuid: cell.uuid(), facet_index: facet_idx, - neighbor_key: *neighbor_key, + neighbor_key, }, }); } - if *neighbor_key == cell_key { + if neighbor_key == cell_key { if cell_allows_periodic_self_neighbor(cell) { continue; } @@ -860,13 +871,13 @@ where } let neighbor_cell = - tds.cell(*neighbor_key) + tds.cell(neighbor_key) .ok_or_else(|| TdsValidationFailure::InvalidNeighbors { reason: NeighborValidationError::MissingNeighborCell { cell_key, cell_uuid: cell.uuid(), facet_index: facet_idx, - neighbor_key: *neighbor_key, + neighbor_key, context: "flip trial neighbor validation".to_string(), }, })?; @@ -885,14 +896,14 @@ where facet_key_from_vertices(&facet_vertices_from_cell(cell, facet_idx)), cell_key, facet_idx, - *neighbor_key, + neighbor_key, mirror_idx, )?; validate_flip_trial_neighbor_orientation( cell_key, cell, facet_idx, - *neighbor_key, + neighbor_key, neighbor_cell, mirror_idx, )?; @@ -939,12 +950,8 @@ where context: "flip trial mutual neighbor validation".to_string(), })?; - let source_neighbor = source_cell - .neighbors() - .and_then(|neighbors| neighbors.get(source_facet).copied().flatten()); - let target_neighbor = target_cell - .neighbors() - .and_then(|neighbors| neighbors.get(target_facet).copied().flatten()); + let source_neighbor = source_cell.neighbor_key(source_facet).flatten(); + let target_neighbor = target_cell.neighbor_key(target_facet).flatten(); if source_neighbor != Some(target_cell_key) || target_neighbor != Some(source_cell_key) { return Err(TdsValidationFailure::InvalidNeighbors { @@ -1438,7 +1445,7 @@ fn debug_ridge_context( .map(|cells| cells.into_iter().collect::>()); let global_cells = cells_containing_vertices(tds, &ridge_vertices); let neighbor_snapshot: Option, MAX_PRACTICAL_DIMENSION_SIZE>> = - cell.neighbors().map(|ns| ns.iter().copied().collect()); + cell.neighbor_keys().map(Iterator::collect); let global_cell_details: Vec = global_cells .iter() .copied() @@ -1499,15 +1506,11 @@ where V: DataType, { let mut ridge_neighbors: SmallBuffer = SmallBuffer::new(); - let Some(neighbors) = cell.neighbors() else { - return ridge_neighbors; - }; - for (idx, &vertex_key) in cell.vertices().iter().enumerate() { if ridge_vertices.contains(&vertex_key) { continue; } - if let Some(neighbor_key) = neighbors.get(idx).copied().flatten() { + if let Some(neighbor_key) = cell.neighbor_key(idx).flatten() { ridge_neighbors.push(neighbor_key); } } @@ -1637,7 +1640,7 @@ where .filter(|vkey| !facet_vertices.contains(vkey)) .collect(); let neighbor_snapshot: Option, MAX_PRACTICAL_DIMENSION_SIZE>> = - cell.neighbors().map(|ns| ns.iter().copied().collect()); + cell.neighbor_keys().map(Iterator::collect); format!( "{cell_key:?}: vertices={:?} opposite_vertices={opposite_vertices:?} neighbors={neighbor_snapshot:?}", @@ -3944,8 +3947,8 @@ where } let neighbor_key = cell_a - .neighbors() - .and_then(|n| n.get(facet_index_a).copied().flatten()) + .neighbor_key(facet_index_a) + .flatten() .ok_or(FlipError::BoundaryFacet { facet })?; let cell_b = tds.cell(neighbor_key).ok_or(FlipError::MissingNeighbor { @@ -4015,9 +4018,8 @@ fn back_reference_facet_index( neighbor_cell: &Cell, ) -> Option { neighbor_cell - .neighbors()? - .iter() - .position(|neighbor| *neighbor == Some(source_cell)) + .neighbor_keys()? + .position(|neighbor| neighbor == Some(source_cell)) } /// Increments a small vertex-incidence count buffer without allocating a hash map @@ -7996,20 +7998,18 @@ where return Ok(cells); } - if let Some(neighbors) = cell.neighbors() { - for &omit_idx in &omit_indices { - if let Some(neighbor_key) = neighbors.get(omit_idx).copied().flatten() { - let Some(neighbor_cell) = tds.cell(neighbor_key) else { - return Err(FlipError::DanglingRidgeNeighbor { - cell_key, - neighbor_key, - }); - }; - if !ridge.iter().all(|v| neighbor_cell.contains_vertex(*v)) { - return Err(FlipError::InvalidRidgeAdjacency { cell_key }); - } - queue.push(neighbor_key); + for &omit_idx in &omit_indices { + if let Some(neighbor_key) = cell.neighbor_key(omit_idx).flatten() { + let Some(neighbor_cell) = tds.cell(neighbor_key) else { + return Err(FlipError::DanglingRidgeNeighbor { + cell_key, + neighbor_key, + }); + }; + if !ridge.iter().all(|v| neighbor_cell.contains_vertex(*v)) { + return Err(FlipError::InvalidRidgeAdjacency { cell_key }); } + queue.push(neighbor_key); } } } @@ -8748,8 +8748,8 @@ fn enqueue_facet( } let Some(_neighbor_key) = cell - .neighbors() - .and_then(|n| n.get(facet_index).copied().flatten()) + .neighbor_key(facet_index) + .flatten() .filter(|&nk| tds.contains_cell(nk)) else { return; @@ -9677,10 +9677,9 @@ mod tests { let facet_idx_glue_edge = facet_index_for_edge_2d(&tds, cell_external_left, v_left_bottom, v_left_top); let external_cell = tds.cell(cell_external_left).unwrap(); - let neighbors = external_cell - .neighbors() - .expect("external neighbors should exist"); - let neighbor_key_glue = neighbors[usize::from(facet_idx_glue_edge)] + let neighbor_key_glue = external_cell + .neighbor_key(usize::from(facet_idx_glue_edge)) + .expect("external neighbors should exist") .expect("external cell should have a neighbor across the glue edge after the flip"); assert!(tds.contains_cell(neighbor_key_glue)); @@ -9697,18 +9696,16 @@ mod tests { let mirror_idx = external_cell .mirror_facet_index(usize::from(facet_idx_glue_edge), neighbor_cell) .expect("mirror facet index should exist"); - let neighbor_back = neighbor_cell - .neighbors() - .and_then(|ns| ns.get(mirror_idx).copied().flatten()); + let neighbor_back = neighbor_cell.neighbor_key(mirror_idx).flatten(); assert_eq!(neighbor_back, Some(cell_external_left)); // Ensure flip did not leave any dangling neighbor pointers in the newly inserted cells. for &cell_key in &info.new_cells { let cell = tds.cell(cell_key).unwrap(); if let Some(ns) = cell.neighbors() { - for neighbor_key in ns.iter().flatten() { + for neighbor_key in ns.flatten() { assert!( - tds.contains_cell(*neighbor_key), + tds.contains_cell(neighbor_key), "dangling neighbor pointer from {cell_key:?} to {neighbor_key:?}" ); } @@ -10066,10 +10063,9 @@ mod tests { let glue_face_facet_index = facet_index_for_face_3d(&tds, cell_external, v_edge_start, v_cycle_0, v_cycle_1); let external_cell = tds.cell(cell_external).unwrap(); - let neighbors = external_cell - .neighbors() - .expect("external cell should have neighbors after repair"); - let glued_neighbor = neighbors[usize::from(glue_face_facet_index)] + let glued_neighbor = external_cell + .neighbor_key(usize::from(glue_face_facet_index)) + .expect("external cell should have neighbors after repair") .expect("external cell should have a neighbor across the glue face"); assert!(tds.contains_cell(glued_neighbor)); @@ -10086,18 +10082,16 @@ mod tests { let mirror_idx = external_cell .mirror_facet_index(usize::from(glue_face_facet_index), neighbor_cell) .expect("mirror facet index should exist"); - let neighbor_back = neighbor_cell - .neighbors() - .and_then(|ns| ns.get(mirror_idx).copied().flatten()); + let neighbor_back = neighbor_cell.neighbor_key(mirror_idx).flatten(); assert_eq!(neighbor_back, Some(cell_external)); // Ensure the newly inserted cells do not reference removed cells. for &cell_key in &info.new_cells { let cell = tds.cell(cell_key).unwrap(); if let Some(ns) = cell.neighbors() { - for neighbor_key in ns.iter().flatten() { + for neighbor_key in ns.flatten() { assert!( - tds.contains_cell(*neighbor_key), + tds.contains_cell(neighbor_key), "dangling neighbor pointer from {cell_key:?} to {neighbor_key:?}" ); } @@ -10283,7 +10277,6 @@ mod tests { .neighbors() .map(|neighbor_keys| { neighbor_keys - .iter() .map(|neighbor| { neighbor .and_then(|neighbor_key| tds.cell(neighbor_key).map(Cell::uuid)) @@ -10306,7 +10299,7 @@ mod tests { ( vertex.uuid(), vertex - .incident_cell + .incident_cell() .and_then(|cell_key| tds.cell(cell_key).map(Cell::uuid)), ) }) @@ -10350,7 +10343,9 @@ mod tests { tds.assign_incident_cells().unwrap(); let isolated_vertex = tds.insert_vertex_with_mapping(vertex!([20.0; D])).unwrap(); - tds.vertex_mut(isolated_vertex).unwrap().incident_cell = Some(second_cell); + tds.vertex_mut(isolated_vertex) + .unwrap() + .set_incident_cell(Some(second_cell)); let before = snapshot_topology(&tds); let before_incidence = snapshot_incidence(&tds); @@ -10392,6 +10387,33 @@ mod tests { gen_trial_validation_rollback_tests!(2, 3, 4, 5); + #[test] + fn test_flip_trial_validation_rejects_unassigned_neighbor_slot() { + let mut tds: Tds = Tds::empty(); + let v0 = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); + let v1 = tds.insert_vertex_with_mapping(vertex!([1.0, 0.0])).unwrap(); + let v2 = tds.insert_vertex_with_mapping(vertex!([0.0, 1.0])).unwrap(); + let cell_key = tds + .insert_cell_with_mapping(Cell::new(vec![v0, v1, v2], None).unwrap()) + .unwrap(); + tds.assign_neighbors().unwrap(); + + { + let cell = tds.cell_mut(cell_key).unwrap(); + cell.ensure_neighbors_buffer_mut()[0] = NeighborSlot::Unassigned; + } + + let cell = tds.cell(cell_key).unwrap(); + let err = validate_flip_trial_cell_neighbors(&tds, cell_key, cell, &[]).unwrap_err(); + + assert!(matches!( + err, + TdsValidationFailure::InvalidNeighbors { + reason: NeighborValidationError::UnassignedNeighborSlot { facet_index: 0, .. }, + } + )); + } + /// Checks that a k=2 flip and its inverse preserve topology in dimension `D`. #[expect( clippy::too_many_lines, @@ -11524,11 +11546,10 @@ mod tests { .unwrap(); assert_eq!(tds.remove_cells_by_keys(&[dangling_neighbor]), 1); - let neighbors = tds - .cell_mut(cell) + tds.cell_mut(cell) .expect("test cell should exist") - .ensure_neighbors_buffer_mut(); - neighbors[0] = Some(dangling_neighbor); + .set_neighbors_from_keys([Some(dangling_neighbor), None, None, None]) + .unwrap(); let ridge = RidgeHandle::new(cell, 0, 1); let err = build_k3_flip_context(&tds, ridge).unwrap_err(); diff --git a/src/core/algorithms/incremental_insertion.rs b/src/core/algorithms/incremental_insertion.rs index fec3abe3..62a81df5 100644 --- a/src/core/algorithms/incremental_insertion.rs +++ b/src/core/algorithms/incremental_insertion.rs @@ -30,7 +30,7 @@ use crate::core::algorithms::flips::DelaunayRepairError; use crate::core::algorithms::locate::{ ConflictError, LocateError, LocateResult, extract_cavity_boundary, }; -use crate::core::cell::{Cell, CellValidationError}; +use crate::core::cell::{Cell, CellValidationError, NeighborSlot}; use crate::core::collections::{ CellKeyBuffer, FastHashMap, FastHashSet, FastHasher, MAX_PRACTICAL_DIMENSION_SIZE, SmallBuffer, VertexKeyBuffer, @@ -2282,14 +2282,14 @@ where let Some(cell) = tds.cell(cell_key) else { continue; }; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for (facet_idx, neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_opt) in neighbors.enumerate() { let Some(neighbor_key) = neighbor_opt else { continue; }; - let Some(neighbor_cell) = tds.cell(*neighbor_key) else { + let Some(neighbor_cell) = tds.cell(neighbor_key) else { continue; }; let Some(mirror_idx) = cell.mirror_facet_index(facet_idx, neighbor_cell) else { @@ -2302,9 +2302,7 @@ where ); continue; }; - let neighbor_back = neighbor_cell - .neighbors() - .and_then(|ns| ns.get(mirror_idx).copied().flatten()); + let neighbor_back = neighbor_cell.neighbor_key(mirror_idx).flatten(); if neighbor_back != Some(cell_key) { mismatches += 1; tracing::warn!( @@ -2343,30 +2341,30 @@ where let vertex_count = cell.number_of_vertices(); total_slots = total_slots.saturating_add(vertex_count); - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { neighbor_none = neighbor_none.saturating_add(vertex_count); continue; }; - for (facet_idx, neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_opt) in neighbors.enumerate() { match neighbor_opt { None => { neighbor_none = neighbor_none.saturating_add(1); } Some(neighbor_key) => { - if new_cells_set.contains(neighbor_key) { + if new_cells_set.contains(&neighbor_key) { neighbor_new = neighbor_new.saturating_add(1); - } else if conflict_set.contains(neighbor_key) { + } else if conflict_set.contains(&neighbor_key) { neighbor_conflict = neighbor_conflict.saturating_add(1); if anomaly_samples.len() < 10 { anomaly_samples.push(( cell_key, facet_idx, - Some(*neighbor_key), + Some(neighbor_key), "CONFLICT".to_string(), )); } - } else if tds.contains_cell(*neighbor_key) { + } else if tds.contains_cell(neighbor_key) { neighbor_existing = neighbor_existing.saturating_add(1); } else { neighbor_missing = neighbor_missing.saturating_add(1); @@ -2374,7 +2372,7 @@ where anomaly_samples.push(( cell_key, facet_idx, - Some(*neighbor_key), + Some(neighbor_key), "MISSING".to_string(), )); } @@ -2470,11 +2468,11 @@ where let cell = tds .cell(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { let Some(neighbor_key) = neighbor_opt else { continue; }; @@ -2531,18 +2529,35 @@ where let cell = tds .cell_mut(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; + let facet_idx = usize::from(facet_idx); + if facet_idx >= cell.number_of_vertices() { + return Err(NeighborWiringError::InvalidFacetIndex { + cell_key, + facet_index: facet_idx, + vertex_count: cell.number_of_vertices(), + } + .into()); + } - let mut neighbors = cell.neighbors().map_or_else( - || SmallBuffer::from_elem(None, D + 1), - |n| n.iter().copied().collect(), - ); - - neighbors[usize::from(facet_idx)] = neighbor; - cell.neighbors = Some(neighbors); + if cell.neighbor_slots().is_none() { + set_cell_neighbors_from_keys(cell, (0..=D).map(|_| None))?; + } + let neighbors = cell.ensure_neighbors_buffer_mut(); + neighbors[facet_idx] = NeighborSlot::from_neighbor_key(neighbor); Ok(()) } +/// Installs a fully assigned neighbor buffer and preserves typed cell context on arity errors. +fn set_cell_neighbors_from_keys( + cell: &mut Cell, + neighbors: impl IntoIterator>, +) -> Result<(), InsertionError> { + let cell_id = cell.uuid(); + cell.set_neighbors_from_keys(neighbors) + .map_err(|source| TdsError::InvalidCell { cell_id, source }.into()) +} + /// Hash a facet from sorted vertex keys. /// /// Uses [`FastHasher`] for deterministic hashing consistent with other @@ -2658,7 +2673,7 @@ where /// let (cell_key, facet_idx, neighbor_key) = tds /// .cells() /// .find_map(|(cell_key, cell)| { -/// cell.neighbors()?.iter().enumerate().find_map(|(facet_idx, neighbor)| { +/// cell.neighbors()?.enumerate().find_map(|(facet_idx, neighbor)| { /// neighbor.map(|neighbor_key| (cell_key, facet_idx, neighbor_key)) /// }) /// }) @@ -2670,10 +2685,7 @@ where /// assert!(repaired >= 2); /// assert_eq!( /// tds.cell(cell_key) -/// .and_then(|cell| cell.neighbors()) -/// .and_then(|neighbors| neighbors.get(facet_idx)) -/// .copied() -/// .flatten(), +/// .and_then(|cell| cell.neighbor_key(facet_idx).flatten()), /// Some(neighbor_key) /// ); /// # Ok(()) @@ -2719,10 +2731,10 @@ where let Some(cell) = tds.cell(cell_key) else { continue; }; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &neighbor_key in neighbors.iter().flatten() { + for neighbor_key in neighbors.flatten() { if tds.contains_cell(neighbor_key) { affected_cells.insert(neighbor_key); } @@ -2776,13 +2788,13 @@ where .ok_or(NeighborWiringError::MissingCell { cell_key })?; let vertex_count = cell.number_of_vertices(); let old_neighbors: SmallBuffer, MAX_PRACTICAL_DIMENSION_SIZE> = - cell.neighbors().map_or_else( + cell.neighbor_keys().map_or_else( || SmallBuffer::from_elem(None, vertex_count), |neighbors| { - let mut copied: SmallBuffer, MAX_PRACTICAL_DIMENSION_SIZE> = - neighbors.iter().copied().collect(); - copied.resize(vertex_count, None); - copied + let mut old_neighbors = SmallBuffer::new(); + old_neighbors.extend(neighbors); + old_neighbors.resize(vertex_count, None); + old_neighbors }, ); @@ -2852,11 +2864,7 @@ where let cell = tds .cell_mut(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; - if rebuilt_neighbors.iter().all(Option::is_none) { - cell.neighbors = None; - } else { - cell.neighbors = Some(rebuilt_neighbors); - } + set_cell_neighbors_from_keys(cell, rebuilt_neighbors)?; } #[cfg(debug_assertions)] @@ -3026,9 +3034,9 @@ where .cell(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; - cell.neighbors().map_or_else( + cell.neighbor_keys().map_or_else( || SmallBuffer::from_elem(None, rebuilt.len()), - |ns| ns.iter().copied().collect(), + Iterator::collect, ) }; @@ -3044,11 +3052,7 @@ where .cell_mut(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; - if rebuilt.iter().all(Option::is_none) { - cell.neighbors = None; - } else { - cell.neighbors = Some(rebuilt); - } + set_cell_neighbors_from_keys(cell, rebuilt)?; } #[cfg(debug_assertions)] @@ -3101,11 +3105,11 @@ where .cell(current) .ok_or(NeighborWiringError::MissingCell { cell_key: current })?; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { let Some(neighbor_key) = neighbor_opt else { continue; }; @@ -3163,11 +3167,11 @@ where .cell(current) .ok_or(NeighborWiringError::MissingCell { cell_key: current })?; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { let Some(neighbor_key) = neighbor_opt else { continue; }; @@ -3215,11 +3219,11 @@ where let cell = tds .cell(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for (facet_idx, &neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_opt) in neighbors.enumerate() { let Some(neighbor_key) = neighbor_opt else { continue; }; @@ -3237,13 +3241,8 @@ where }; let mirror_facet_index = cell.mirror_facet_index(facet_idx, neighbor_cell); - let neighbor_back = mirror_facet_index.and_then(|mirror_idx| { - neighbor_cell - .neighbors() - .and_then(|neighbor_neighbors| neighbor_neighbors.get(mirror_idx)) - .copied() - .flatten() - }); + let neighbor_back = mirror_facet_index + .and_then(|mirror_idx| neighbor_cell.neighbor_key(mirror_idx).flatten()); if neighbor_back != Some(cell_key) { return Err(NeighborWiringError::AsymmetricNeighbor { @@ -4155,10 +4154,10 @@ mod tests { V: DataType, { for (cell_key, cell) in tds.cells() { - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for (facet_idx, &neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_opt) in neighbors.enumerate() { let Some(neighbor_key) = neighbor_opt else { continue; }; @@ -4514,8 +4513,10 @@ mod tests { .insert_cell_with_mapping(Cell::new(vec![v0, v1, v2], None).unwrap()) .unwrap(); let missing_neighbor = CellKey::from(KeyData::from_ffi(u64::MAX - 1)); - tds.cell_mut(cell_key).unwrap().neighbors = - Some(vec![Some(missing_neighbor), None, None].into()); + tds.cell_mut(cell_key) + .unwrap() + .set_neighbors_from_keys(vec![Some(missing_neighbor), None, None]) + .unwrap(); let mut internal_cells = CellKeyBuffer::new(); internal_cells.push(cell_key); @@ -5400,7 +5401,7 @@ mod tests { // Verify all neighbor pointers are initially valid for (_, cell) in tds.cells() { if let Some(neighbors) = cell.neighbors() { - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { if let Some(neighbor_key) = neighbor_opt { assert!(tds.contains_cell(neighbor_key), "Neighbor should exist"); } @@ -5415,7 +5416,7 @@ mod tests { // Verify all pointers still valid after repair for (_, cell) in tds.cells() { if let Some(neighbors) = cell.neighbors() { - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { if let Some(neighbor_key) = neighbor_opt { assert!(tds.contains_cell(neighbor_key), "Neighbor should still exist after repair"); } @@ -5496,9 +5497,10 @@ mod tests { "Expected at least one neighbor pointer to be repaired" ); - let any_internal_neighbor = tds - .cells() - .any(|(_, c)| c.neighbors().is_some_and(|n| n.iter().any(Option::is_some))); + let any_internal_neighbor = tds.cells().any(|(_, c)| { + c.neighbors() + .is_some_and(|mut n| n.any(|neighbor| neighbor.is_some())) + }); assert!( any_internal_neighbor, "Expected at least one internal neighbor after repair" @@ -5530,10 +5532,7 @@ mod tests { assert_eq!(repaired, 1); let cell = tds.cell(cell_key).unwrap(); assert_eq!( - cell.neighbors() - .and_then(|neighbors| neighbors.get(usize::from(facet_idx))) - .copied() - .flatten(), + cell.neighbor_key(usize::from(facet_idx)).flatten(), Some(neighbor_key) ); assert!(tds.is_valid().is_ok()); @@ -5683,10 +5682,7 @@ mod tests { assert_eq!(repaired, 1); let cell = tds.cell(cell_key).unwrap(); assert_eq!( - cell.neighbors() - .and_then(|neighbors| neighbors.get(usize::from(facet_idx))) - .copied() - .flatten(), + cell.neighbor_key(usize::from(facet_idx)).flatten(), Some(neighbor_key) ); assert!(tds.is_valid().is_ok()); @@ -5732,13 +5728,7 @@ mod tests { assert_eq!(repaired, 1); let cell = tds.cell(c1).unwrap(); - assert_eq!( - cell.neighbors() - .and_then(|neighbors| neighbors.get(shared_facet_idx)) - .copied() - .flatten(), - Some(c2) - ); + assert_eq!(cell.neighbor_key(shared_facet_idx).flatten(), Some(c2)); } #[test] @@ -5761,13 +5751,7 @@ mod tests { assert_eq!(repaired, 0); let cell = tds.cell(cell_key).unwrap(); - assert_eq!( - cell.neighbors() - .and_then(|neighbors| neighbors.get(usize::from(facet_idx))) - .copied() - .flatten(), - None - ); + assert_eq!(cell.neighbor_key(usize::from(facet_idx)).flatten(), None); // The global repair still sees the missing slot, proving the local path was scoped. assert!(repair_neighbor_pointers(tds).unwrap() > 0); @@ -5891,6 +5875,29 @@ mod tests { )); } + #[test] + fn test_set_neighbor_errors_on_invalid_facet_index() { + let mut tds: Tds = Tds::empty(); + let v0 = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); + let v1 = tds.insert_vertex_with_mapping(vertex!([1.0, 0.0])).unwrap(); + let v2 = tds.insert_vertex_with_mapping(vertex!([0.0, 1.0])).unwrap(); + let cell_key = tds + .insert_cell_with_mapping(Cell::new(vec![v0, v1, v2], None).unwrap()) + .unwrap(); + + let err = set_neighbor(&mut tds, cell_key, 3, None).unwrap_err(); + assert!(matches!( + err, + InsertionError::NeighborWiring { + reason: NeighborWiringError::InvalidFacetIndex { + facet_index: 3, + vertex_count: 3, + .. + }, + } + )); + } + #[test] fn test_external_facets_for_boundary_empty_inputs_returns_empty() { let tds: Tds = Tds::empty(); diff --git a/src/core/algorithms/locate.rs b/src/core/algorithms/locate.rs index c4cee74d..26b66c98 100644 --- a/src/core/algorithms/locate.rs +++ b/src/core/algorithms/locate.rs @@ -896,11 +896,7 @@ where for facet_idx in 0..facet_count { if is_point_outside_facet(tds, kernel, current_cell, facet_idx, point)? == Some(true) { - if let Some(neighbor_key) = cell - .neighbors() - .and_then(|neighbors| neighbors.get(facet_idx)) - .and_then(|&opt_key| opt_key) - { + if let Some(neighbor_key) = cell.neighbor_key(facet_idx).flatten() { current_cell = neighbor_key; found_outside_facet = true; break; @@ -1253,8 +1249,8 @@ where } // Add neighbors to queue for exploration - if let Some(neighbors) = cell.neighbors() { - for &neighbor_opt in neighbors { + if let Some(neighbors) = cell.neighbor_keys() { + for neighbor_opt in neighbors { if let Some(neighbor_key) = neighbor_opt && !visited.contains_key(neighbor_key) { @@ -1273,8 +1269,8 @@ where #[cfg(debug_assertions)] if debug_config.log_conflict { let neighbor_keys: SmallBuffer, MAX_PRACTICAL_DIMENSION_SIZE> = - cell.neighbors() - .map(|ns| ns.iter().copied().collect()) + cell.neighbor_keys() + .map(Iterator::collect) .unwrap_or_default(); tracing::debug!( cell_key = ?cell_key, @@ -1393,14 +1389,21 @@ where // - Unreachable: NO neighbors are in bfs_set, indicating // broken neighbor pointers or a disconnected pocket let (neighbor_in_bfs, neighbor_total, neighbor_none) = - cell.neighbors().map_or((0, 0, 0), |neighbors| { + cell.neighbor_keys().map_or((0, 0, 0), |neighbors| { let total = neighbors.len(); - let none_count = neighbors.iter().filter(|n| n.is_none()).count(); - let in_bfs = neighbors - .iter() - .filter_map(|n| *n) - .filter(|nk| bfs_set.contains(nk)) - .count(); + let mut none_count = 0; + let mut in_bfs = 0; + for neighbor in neighbors { + match neighbor { + Some(nk) if bfs_set.contains(&nk) => { + in_bfs += 1; + } + Some(_) => {} + None => { + none_count += 1; + } + } + } (in_bfs, total, none_count) }); @@ -2353,7 +2356,7 @@ mod tests { let cell = dt.tds_mut().cell_mut(cell_key).unwrap(); let mut neighbors = NeighborBuffer::>::new(); neighbors.resize(3, Some(cell_key)); - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); // Point outside the simplex: walking will attempt to cross a facet, hit the self-loop, // detect a cycle, and fall back to scan. @@ -3038,8 +3041,8 @@ mod tests { let ghost = CellKey::from(KeyData::from_ffi(777_777)); { let cell = tds.cell_mut(cell_key).unwrap(); - let buf = cell.ensure_neighbors_buffer_mut(); - buf[0] = Some(ghost); + cell.set_neighbors_from_keys([Some(ghost), None, None]) + .unwrap(); } let kernel = FastKernel::::new(); diff --git a/src/core/algorithms/pl_manifold_repair.rs b/src/core/algorithms/pl_manifold_repair.rs index 1203f780..56486c3d 100644 --- a/src/core/algorithms/pl_manifold_repair.rs +++ b/src/core/algorithms/pl_manifold_repair.rs @@ -445,7 +445,7 @@ fn remove_orphaned_vertices( { let mut orphaned: Vec<(VertexKey, Uuid)> = tds .vertices() - .filter(|(_, v)| v.incident_cell.is_none()) + .filter(|(_, v)| v.incident_cell().is_none()) .map(|(k, v)| (k, v.uuid())) .collect(); orphaned.sort_by_key(|(_, uuid)| *uuid); diff --git a/src/core/cell.rs b/src/core/cell.rs index a642d190..f290d74f 100644 --- a/src/core/cell.rs +++ b/src/core/cell.rs @@ -141,6 +141,14 @@ pub enum CellValidationError { /// The dimension D. dimension: usize, }, + /// An assigned neighbor buffer contains an unassigned slot. + #[error( + "Unassigned neighbor slot at facet {facet_index}; assigned neighbor buffers must contain only boundary or neighbor slots" + )] + UnassignedNeighborSlot { + /// Facet slot that is still unassigned. + facet_index: usize, + }, /// The periodic offset list is not aligned with the cell's vertex list. #[error("Periodic offset length mismatch: got {found}, expected {expected}")] PeriodicOffsetLengthMismatch { @@ -187,6 +195,77 @@ where cmp::Ordering::Equal } +/// A typed neighbor slot for a cell facet. +/// +/// This distinguishes the TDS states that nested `Option` storage used to blur: +/// a neighbor buffer can be unassigned, a facet can be assigned as a boundary, +/// or a facet can point at a neighboring cell. +/// +/// # Examples +/// +/// ```rust +/// use delaunay::prelude::tds::{CellKey, NeighborSlot}; +/// use slotmap::KeyData; +/// +/// let key = CellKey::from(KeyData::from_ffi(1)); +/// +/// assert_eq!( +/// NeighborSlot::from_neighbor_key(Some(key)), +/// NeighborSlot::Neighbor(key) +/// ); +/// assert!(NeighborSlot::from_neighbor_key(None).is_boundary()); +/// assert!(NeighborSlot::Unassigned.is_unassigned()); +/// assert_eq!(NeighborSlot::Boundary.cell_key(), None); +/// ``` +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub enum NeighborSlot { + /// The facet slot has not been assigned by the TDS neighbor builder. + Unassigned, + /// The facet is assigned and lies on the boundary. + Boundary, + /// The facet is assigned and has a neighboring cell. + Neighbor(CellKey), +} + +impl NeighborSlot { + /// Converts an optional neighbor key into an assigned neighbor slot. + /// + /// `Some(key)` becomes [`Neighbor`](Self::Neighbor); `None` becomes + /// [`Boundary`](Self::Boundary), not [`Unassigned`](Self::Unassigned). + #[inline] + #[must_use] + pub const fn from_neighbor_key(neighbor: Option) -> Self { + match neighbor { + Some(cell_key) => Self::Neighbor(cell_key), + None => Self::Boundary, + } + } + + /// Returns the neighboring cell key when this slot has one. + #[inline] + #[must_use] + pub const fn cell_key(self) -> Option { + match self { + Self::Neighbor(cell_key) => Some(cell_key), + Self::Boundary | Self::Unassigned => None, + } + } + + /// Returns whether the slot is an assigned boundary facet. + #[inline] + #[must_use] + pub const fn is_boundary(self) -> bool { + matches!(self, Self::Boundary) + } + + /// Returns whether the slot has not been assigned by the TDS. + #[inline] + #[must_use] + pub const fn is_unassigned(self) -> bool { + matches!(self, Self::Unassigned) + } +} + // ============================================================================= // CONVENIENCE MACROS AND HELPERS // ============================================================================= @@ -251,8 +330,7 @@ pub struct Cell { /// The unique identifier of the cell. uuid: Uuid, - /// Keys to neighboring cells, indexed by opposite vertex. - /// Phase 3A: Changed from `Option>>` to `Option>>`. + /// Typed neighboring-cell slots, indexed by opposite vertex. /// /// Positional semantics: `neighbors[i]` is the neighbor opposite `vertices[i]`. /// @@ -264,8 +342,8 @@ pub struct Cell { /// - `neighbors[3]` is opposite `vertices[3]` (shares vertices 0, 1, 2) /// /// Note: Not serialized β€” neighbors are reconstructed during deserialization by the TDS. - /// Access via `neighbors()` method. Writable by TDS for neighbor assignment. - pub(crate) neighbors: Option>>, + /// Access via `neighbor_slots()` or `neighbors()`. Mutation goes through TDS-owned helpers. + neighbors: Option>, /// The optional data associated with the cell. pub(crate) data: Option, @@ -569,16 +647,21 @@ impl Cell { self.vertices.iter().enumerate() } - /// Returns the neighbor keys for this cell. + /// Returns the neighbor keys for this cell without allocating. /// /// # Phase 3A /// /// Neighbors are stored as keys (not UUIDs) for direct TDS access. - /// The positional semantics: `neighbors()[i]` is the neighbor opposite `vertices()[i]`. + /// The positional semantics: `neighbor_key(i)` is the neighbor opposite `vertices()[i]`. /// /// # Returns /// - /// An `Option` containing neighbor keys if they have been assigned, or `None` otherwise. + /// An `Option` containing an iterator over assigned neighbor keys, or + /// `None` if neighbor slots have not been assigned. Inside an assigned + /// buffer, `Some(key)` is a neighboring cell and `None` is an assigned + /// boundary facet. Use [`neighbor_slots`](Self::neighbor_slots) when + /// callers need to distinguish an unassigned neighbor buffer from assigned + /// boundary facets explicitly. /// /// # Example /// @@ -595,19 +678,92 @@ impl Cell { /// let tds = dt.tds(); /// /// if let Some(neighbors) = cell.neighbors() { - /// for (i, neighbor_key_opt) in neighbors.iter().enumerate() { + /// for (i, neighbor_key_opt) in neighbors.enumerate() { /// if let Some(neighbor_key) = neighbor_key_opt { - /// let neighbor_cell = &tds.cell(*neighbor_key).unwrap(); + /// let neighbor_cell = &tds.cell(neighbor_key).unwrap(); /// // neighbor_cell is opposite to vertex i /// } /// } /// } /// ``` #[inline] - pub const fn neighbors(&self) -> Option<&NeighborBuffer>> { + #[must_use] + pub fn neighbors(&self) -> Option> + '_> { + self.neighbor_keys() + } + + /// Returns neighbor keys without allocating an owned compatibility buffer. + /// + /// The iterator yields one entry per assigned neighbor slot. `Some(key)` is + /// a neighboring cell and `None` is an assigned boundary facet. A return + /// value of `None` means neighbor assignment has not run or has been cleared. + #[inline] + #[must_use] + pub(crate) fn neighbor_keys( + &self, + ) -> Option> + '_> { + self.neighbors + .as_ref() + .map(|slots| slots.iter().map(|slot| slot.cell_key())) + } + + /// Returns one assigned neighbor key by facet index without allocating. + /// + /// The outer `Option` is `None` when neighbor slots are unassigned or when + /// `facet_idx` is out of bounds. The inner `Option` is `None` for an + /// assigned boundary facet. + #[inline] + #[must_use] + pub fn neighbor_key(&self, facet_idx: usize) -> Option> { + self.neighbors + .as_ref() + .and_then(|slots| slots.get(facet_idx)) + .map(|slot| slot.cell_key()) + } + + /// Returns the typed neighbor slots for this cell. + /// + /// `None` means neighbor assignment has not run or has been explicitly + /// cleared. `Some` means each facet slot is assigned as either boundary or + /// neighboring-cell state. + /// + /// # Examples + /// + /// ```rust + /// use delaunay::prelude::tds::NeighborSlot; + /// use delaunay::prelude::triangulation::*; + /// + /// let vertices = vec![ + /// vertex!([0.0, 0.0]), + /// vertex!([1.0, 0.0]), + /// vertex!([0.0, 1.0]), + /// ]; + /// let dt = DelaunayTriangulation::new(&vertices).unwrap(); + /// let mut tds = dt.tds().clone(); + /// let cell_key = tds.cell_keys().next().unwrap(); + /// + /// tds.set_neighbors_by_key(cell_key, &[None, None, None]).unwrap(); + /// let cell = tds.cell(cell_key).unwrap(); + /// + /// let slots = cell + /// .neighbor_slots() + /// .expect("set_neighbors_by_key assigns boundary slots"); + /// + /// assert_eq!(slots.len(), 3); + /// assert!(slots.iter().all(|slot| matches!(*slot, NeighborSlot::Boundary))); + /// ``` + #[inline] + #[must_use] + pub const fn neighbor_slots(&self) -> Option<&NeighborBuffer> { self.neighbors.as_ref() } + /// Returns mutable typed neighbor slots for TDS-owned mutation paths. + #[inline] + pub(crate) const fn neighbor_slots_mut(&mut self) -> Option<&mut NeighborBuffer> { + self.neighbors.as_mut() + } + /// Returns the vertex keys for this cell. /// /// # Phase 3A @@ -778,34 +934,37 @@ impl Cell { } } - /// Ensures the cell has a properly initialized neighbors buffer of size D+1. - /// - /// This helper centralizes neighbor buffer initialization logic to avoid code duplication - /// and reduce the error surface for off-by-one bugs. - /// - /// Note: This is currently only used by unit tests, but is kept as a small internal building - /// block for future insertion/repair code that needs to mutate neighbor buffers in-place. - /// - /// # Returns - /// - /// A mutable reference to the neighbors buffer, guaranteed to be sized D+1 with all None values. - /// - /// # Performance + /// Replaces this cell's assigned neighbor slots from optional neighbor keys. + #[inline] + pub(crate) fn set_neighbors_from_keys( + &mut self, + neighbors: impl IntoIterator>, + ) -> Result<(), CellValidationError> { + let mut slots = NeighborBuffer::new(); + slots.extend(neighbors.into_iter().map(NeighborSlot::from_neighbor_key)); + if slots.len() != D + 1 { + return Err(CellValidationError::InvalidNeighborsLength { + actual: slots.len(), + expected: D + 1, + dimension: D, + }); + } + self.neighbors = Some(slots); + Ok(()) + } + + /// Ensures this cell has an assigned neighbor-slot buffer. /// - /// Inline to zero cost in release builds. Only allocates if the buffer doesn't exist. + /// If the buffer does not exist, it is initialized with D+1 unassigned slots. #[inline] - #[cfg_attr( - not(test), - expect(dead_code, reason = "Currently only used by unit tests") - )] - pub(crate) fn ensure_neighbors_buffer_mut(&mut self) -> &mut NeighborBuffer> { + pub(crate) fn ensure_neighbors_buffer_mut(&mut self) -> &mut NeighborBuffer { debug_assert!( self.neighbors.as_ref().is_none_or(|buf| buf.len() == D + 1), "neighbors buffer must always have length D+1" ); self.neighbors.get_or_insert_with(|| { let mut buffer = NeighborBuffer::new(); - buffer.resize(D + 1, None); + buffer.resize(D + 1, NeighborSlot::Unassigned); buffer }) } @@ -1173,8 +1332,9 @@ impl Cell { /// Returns `CellValidationError::InvalidVertex` if any vertex is invalid, /// `CellValidationError::InvalidUuid` if the cell's UUID is nil, /// `CellValidationError::DuplicateVertices` if the cell contains duplicate vertices, - /// `CellValidationError::InsufficientVertices` if the cell doesn't have exactly D+1 vertices, or - /// `CellValidationError::InvalidNeighborsLength` if neighbors are provided but don't have D+1 entries. + /// `CellValidationError::InsufficientVertices` if the cell doesn't have exactly D+1 vertices, + /// `CellValidationError::InvalidNeighborsLength` if neighbors are provided but don't have D+1 entries, or + /// `CellValidationError::UnassignedNeighborSlot` if an assigned neighbor buffer still has an unassigned slot. /// /// # Example /// @@ -1226,6 +1386,13 @@ impl Cell { dimension: D, }); } + if let Some(ref neighbors) = self.neighbors { + for (facet_index, slot) in neighbors.iter().enumerate() { + if slot.is_unassigned() { + return Err(CellValidationError::UnassignedNeighborSlot { facet_index }); + } + } + } Ok(()) @@ -3361,7 +3528,9 @@ mod tests { ); // Cell with correct neighbors length is valid - cell_2d.neighbors = Some(vec![None, None, None].into()); + cell_2d + .set_neighbors_from_keys(vec![None, None, None]) + .unwrap(); assert!( cell_2d.is_valid().is_ok(), "Cell with correct neighbors length should be valid" @@ -3408,29 +3577,33 @@ mod tests { let dt = DelaunayTriangulation::new(&vertices_2d).unwrap(); let (_, cell_ref) = dt.cells().next().unwrap(); let mut cell_wrong_neighbors = cell_ref.clone(); - cell_wrong_neighbors.neighbors = Some(vec![None, None].into()); + let err = cell_wrong_neighbors + .set_neighbors_from_keys(vec![None, None]) + .unwrap_err(); assert!( matches!( - cell_wrong_neighbors.is_valid(), - Err(CellValidationError::InvalidNeighborsLength { + err, + CellValidationError::InvalidNeighborsLength { actual: 2, expected: 3, dimension: 2 - }) + } ), "Wrong neighbors count should fail validation" ); // Invalid neighbors length (too many) - cell_wrong_neighbors.neighbors = Some(vec![None, None, None, None].into()); + let err = cell_wrong_neighbors + .set_neighbors_from_keys(vec![None, None, None, None]) + .unwrap_err(); assert!( matches!( - cell_wrong_neighbors.is_valid(), - Err(CellValidationError::InvalidNeighborsLength { + err, + CellValidationError::InvalidNeighborsLength { actual: 4, expected: 3, dimension: 2 - }) + } ), "Wrong neighbors count should fail validation" ); @@ -3504,16 +3677,68 @@ mod tests { let (cell_key, cell_ref) = dt.cells().next().unwrap(); let mut cell = cell_ref.clone(); + cell.clear_neighbors(); assert!(cell.neighbors.is_none()); let buf = cell.ensure_neighbors_buffer_mut(); assert_eq!(buf.len(), 3); - assert!(buf.iter().all(Option::is_none)); + assert!(buf.iter().all(|slot| slot.is_unassigned())); // Mutate through the returned buffer and ensure it's preserved - buf[0] = Some(cell_key); + buf[0] = NeighborSlot::Neighbor(cell_key); let buf2 = cell.ensure_neighbors_buffer_mut(); - assert_eq!(buf2[0], Some(cell_key)); + assert_eq!(buf2[0], NeighborSlot::Neighbor(cell_key)); + } + + #[test] + fn cell_neighbor_views_distinguish_unassigned_boundary_and_neighbor_slots() { + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), + ]; + let dt = DelaunayTriangulation::new(&vertices).unwrap(); + let (cell_key, cell_ref) = dt.cells().next().unwrap(); + + let mut cell = cell_ref.clone(); + cell.clear_neighbors(); + assert!(cell.neighbor_slots().is_none()); + assert!(cell.neighbors().is_none()); + + cell.set_neighbors_from_keys([None, Some(cell_key), None]) + .unwrap(); + + let slots = cell.neighbor_slots().expect("assigned slots should exist"); + assert_eq!(slots.len(), 3); + assert_eq!(slots[0], NeighborSlot::Boundary); + assert_eq!(slots[1], NeighborSlot::Neighbor(cell_key)); + assert_eq!(slots[2], NeighborSlot::Boundary); + + let neighbor_keys: Vec<_> = cell + .neighbors() + .expect("neighbor iterator should exist") + .collect(); + assert_eq!(neighbor_keys, &[None, Some(cell_key), None]); + } + + #[test] + fn cell_validation_rejects_unassigned_slot_inside_assigned_neighbors() { + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), + ]; + let dt = DelaunayTriangulation::new(&vertices).unwrap(); + let (_, cell_ref) = dt.cells().next().unwrap(); + + let mut cell = cell_ref.clone(); + let slots = cell.ensure_neighbors_buffer_mut(); + slots[0] = NeighborSlot::Unassigned; + + assert!(matches!( + cell.is_valid(), + Err(CellValidationError::UnassignedNeighborSlot { facet_index: 0 }) + )); } #[test] @@ -3527,12 +3752,13 @@ mod tests { let (cell_key, cell_ref) = dt.cells().next().unwrap(); let mut cell = cell_ref.clone(); - cell.neighbors = Some(vec![Some(cell_key), None, Some(cell_key)].into()); + cell.set_neighbors_from_keys(vec![Some(cell_key), None, Some(cell_key)]) + .unwrap(); cell.set_periodic_vertex_offsets(vec![[1, 0], [2, 0], [3, 0]]) .unwrap(); let before_vertices = cell.vertices().to_vec(); - let before_neighbors = cell.neighbors().unwrap().to_vec(); + let before_neighbors: Vec<_> = cell.neighbors().unwrap().collect(); let before_offsets = cell.periodic_vertex_offsets().unwrap().to_vec(); cell.swap_vertex_slots(0, 2); @@ -3540,9 +3766,8 @@ mod tests { assert_eq!(cell.vertices()[0], before_vertices[2]); assert_eq!(cell.vertices()[2], before_vertices[0]); - let neighbors = cell.neighbors().unwrap(); - assert_eq!(neighbors[0], before_neighbors[2]); - assert_eq!(neighbors[2], before_neighbors[0]); + assert_eq!(cell.neighbor_key(0).flatten(), before_neighbors[2]); + assert_eq!(cell.neighbor_key(2).flatten(), before_neighbors[0]); let offsets = cell.periodic_vertex_offsets().unwrap(); assert_eq!(offsets[0], before_offsets[2]); @@ -3561,7 +3786,9 @@ mod tests { let (_, cell_ref) = dt.cells().next().unwrap(); let mut cell = cell_ref.clone(); - cell.neighbors = Some(vec![None, None].into()); + let mut neighbors = NeighborBuffer::::new(); + neighbors.resize(2, NeighborSlot::Boundary); + cell.neighbors = Some(neighbors); cell.swap_vertex_slots(0, 2); } diff --git a/src/core/tds.rs b/src/core/tds.rs index fd86f8c6..bbfc64d6 100644 --- a/src/core/tds.rs +++ b/src/core/tds.rs @@ -230,7 +230,7 @@ #![forbid(unsafe_code)] use super::{ - cell::{Cell, CellValidationError}, + cell::{Cell, CellValidationError, NeighborSlot}, facet::{FacetHandle, facet_key_from_vertices}, traits::data_type::DataType, util::{ @@ -442,6 +442,20 @@ pub enum NeighborValidationError { /// Validation context. context: String, }, + /// A neighbor buffer contains an unassigned facet slot. + #[error( + "Cell {cell_uuid} (key {cell_key:?}) has unassigned neighbor slot at facet {facet_index} during {context}" + )] + UnassignedNeighborSlot { + /// Cell containing the unassigned slot. + cell_key: CellKey, + /// UUID of the cell containing the unassigned slot. + cell_uuid: Uuid, + /// Facet slot that has not been assigned as boundary or neighbor. + facet_index: usize, + /// Validation context. + context: String, + }, /// A non-periodic cell points to itself as a neighbor. #[error( "Cell {cell_uuid} (key {cell_key:?}) has non-periodic self-neighbor at facet {facet_index}" @@ -1903,16 +1917,14 @@ impl Tds { })?[vertex_index2] = Some(cell_key1); } - // Apply updates + // Apply updates. Even cells with only boundary facets receive an + // assigned boundary buffer so assigned-boundary and unassigned states + // remain distinct. for (cell_key, neighbors) in &cell_neighbors { if let Some(cell) = self.cells.get_mut(*cell_key) { - if neighbors.iter().all(Option::is_none) { - cell.neighbors = None; - } else { - let mut neighbor_buffer = SmallBuffer::new(); - neighbor_buffer.extend(neighbors.iter().copied()); - cell.neighbors = Some(neighbor_buffer); - } + let cell_id = cell.uuid(); + cell.set_neighbors_from_keys(neighbors.iter().copied()) + .map_err(|source| TdsError::InvalidCell { cell_id, source })?; } } @@ -2373,10 +2385,10 @@ impl Tds { let Some(cell) = self.cells.get(ck) else { continue; }; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &n_opt in neighbors { + for n_opt in neighbors { let Some(nk) = n_opt else { continue; }; @@ -3453,11 +3465,10 @@ impl Tds { continue; } - if cell.neighbors().is_some_and(|neighbors| { + if cell.neighbor_keys().is_some_and(|neighbors| { neighbors - .iter() .flatten() - .any(|&neighbor_key| !self.cells.contains_key(neighbor_key)) + .any(|neighbor_key| !self.cells.contains_key(neighbor_key)) }) { to_remove.push(cell_key); } @@ -3489,9 +3500,9 @@ impl Tds { SmallBuffer::with_capacity(vertices.len()); neighbors.resize(vertices.len(), None); - if let Some(cell_neighbors) = cell.neighbors() { - for (slot, neighbor_opt) in neighbors.iter_mut().zip(cell_neighbors.iter()) { - *slot = *neighbor_opt; + if let Some(cell_neighbors) = cell.neighbor_keys() { + for (slot, neighbor_opt) in neighbors.iter_mut().zip(cell_neighbors) { + *slot = neighbor_opt; } } @@ -3525,21 +3536,16 @@ impl Tds { let Some(neighbor_cell) = self.cells.get_mut(*neighbor_key) else { continue; }; - let Some(neighbors_buf) = neighbor_cell.neighbors.as_mut() else { + let Some(neighbors_buf) = neighbor_cell.neighbor_slots_mut() else { continue; }; // Clear the back-reference in the neighbor cell's neighbor buffer. for slot in neighbors_buf.iter_mut() { - if *slot == Some(cell_key) { - *slot = None; + if slot.cell_key() == Some(cell_key) { + *slot = NeighborSlot::Boundary; } } - - // Normalize: if all neighbor slots are None, store `None`. - if neighbors_buf.iter().all(Option::is_none) { - neighbor_cell.neighbors = None; - } } } @@ -3574,7 +3580,7 @@ impl Tds { return false; }; - match v.incident_cell { + match v.incident_cell() { None => true, Some(cell_key) => { cells_to_remove.contains(&cell_key) || !self.cells.contains_key(cell_key) @@ -3608,7 +3614,7 @@ impl Tds { for (vk, new_incident) in incident_updates { if let Some(vertex) = self.vertices.get_mut(vk) { - vertex.incident_cell = new_incident; + vertex.set_incident_cell(new_incident); } } } @@ -3651,7 +3657,7 @@ impl Tds { return fallback_scan(); }; - let Some(start_cell_key) = vertex.incident_cell else { + let Some(start_cell_key) = vertex.incident_cell() else { return fallback_scan(); }; @@ -3659,7 +3665,12 @@ impl Tds { return fallback_scan(); }; - if !start_cell.contains_vertex(vertex_key) || start_cell.neighbors().is_none() { + let Some(start_neighbor_slots) = start_cell.neighbor_slots() else { + return fallback_scan(); + }; + if !start_cell.contains_vertex(vertex_key) + || start_neighbor_slots.iter().any(|slot| slot.is_unassigned()) + { return fallback_scan(); } @@ -3674,15 +3685,18 @@ impl Tds { result.push(cell_key); let Some(cell) = self.cells.get(cell_key) else { - continue; + return fallback_scan(); }; - let Some(neighbors) = cell.neighbors() else { - continue; + let Some(neighbors) = cell.neighbor_slots() else { + return fallback_scan(); }; + if neighbors.iter().any(|slot| slot.is_unassigned()) { + return fallback_scan(); + } // Traverse only across facets that still contain the target vertex. - for (facet_idx, neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_slot) in neighbors.iter().copied().enumerate() { if cell .vertices() .get(facet_idx) @@ -3693,24 +3707,24 @@ impl Tds { continue; } - let Some(neighbor_key) = neighbor_opt else { + let NeighborSlot::Neighbor(neighbor_key) = neighbor_slot else { continue; }; - if visited.contains(neighbor_key) { + if visited.contains(&neighbor_key) { continue; } - let Some(neighbor_cell) = self.cells.get(*neighbor_key) else { - continue; + let Some(neighbor_cell) = self.cells.get(neighbor_key) else { + return fallback_scan(); }; if !neighbor_cell.contains_vertex(vertex_key) { - continue; + return fallback_scan(); } - visited.insert(*neighbor_key); - stack.push(*neighbor_key); + visited.insert(neighbor_key); + stack.push(neighbor_key); } } @@ -3818,7 +3832,7 @@ impl Tds { return; }; // Only remove if truly isolated. - if vertex.incident_cell.is_some() { + if vertex.incident_cell().is_some() { return; } let uuid = vertex.uuid(); @@ -3878,10 +3892,10 @@ impl Tds { }; // Phase 3A: Cell now stores neighbors directly - if let Some(ref neighbors_from_cell) = cell.neighbors { + if let Some(neighbors_from_cell) = cell.neighbor_keys() { // Use zip to avoid potential OOB if neighbors_from_cell.len() > D+1 (malformed data) - for (slot, neighbor_key_opt) in neighbors.iter_mut().zip(neighbors_from_cell.iter()) { - *slot = *neighbor_key_opt; + for (slot, neighbor_key_opt) in neighbors.iter_mut().zip(neighbors_from_cell) { + *slot = neighbor_key_opt; } } @@ -4110,37 +4124,24 @@ impl Tds { Ok(()) } - fn normalized_neighbor_buffer( + fn set_cell_neighbors_normalized( + cell: &mut Cell, neighbors: &[Option], - ) -> Option, MAX_PRACTICAL_DIMENSION_SIZE>> { - if neighbors.iter().all(Option::is_none) { - None - } else { - let mut neighbor_buffer = SmallBuffer::new(); - neighbor_buffer.extend(neighbors.iter().copied()); - Some(neighbor_buffer) - } - } - - fn set_cell_neighbors_normalized(cell: &mut Cell, neighbors: &[Option]) { - cell.neighbors = Self::normalized_neighbor_buffer(neighbors); + ) -> Result<(), TdsError> { + let cell_id = cell.uuid(); + cell.set_neighbors_from_keys(neighbors.iter().copied()) + .map_err(|source| TdsError::InvalidCell { cell_id, source }) } fn ensure_neighbor_buffer( cell: &mut Cell, - ) -> Result<&mut SmallBuffer, MAX_PRACTICAL_DIMENSION_SIZE>, TdsError> { - if cell.neighbors.is_none() { - let mut neighbors = SmallBuffer::with_capacity(D + 1); - neighbors.resize(D + 1, None); - cell.neighbors = Some(neighbors); - } - - let neighbors = - cell.neighbors - .as_mut() - .ok_or_else(|| TdsError::InconsistentDataStructure { - message: "neighbor buffer missing after initialization".to_string(), - })?; + ) -> Result<&mut SmallBuffer, TdsError> { + if cell.neighbor_slots().is_none() { + let cell_id = cell.uuid(); + cell.set_neighbors_from_keys((0..=D).map(|_| None)) + .map_err(|source| TdsError::InvalidCell { cell_id, source })?; + } + let neighbors = cell.ensure_neighbors_buffer_mut(); if neighbors.len() != D + 1 { return Err(TdsError::InvalidNeighbors { reason: NeighborValidationError::LengthMismatch { @@ -4167,10 +4168,7 @@ impl Tds { }, }); }; - *slot = neighbor; - if neighbors.iter().all(Option::is_none) { - cell.neighbors = None; - } + *slot = NeighborSlot::from_neighbor_key(neighbor); Ok(()) } @@ -4187,8 +4185,8 @@ impl Tds { context: "set_neighbors_by_key".to_string(), })?; let old_neighbors: Vec> = cell - .neighbors() - .map_or_else(|| vec![None; D + 1], |old| old.iter().copied().collect()); + .neighbor_keys() + .map_or_else(|| vec![None; D + 1], Iterator::collect); let mut reciprocal_updates = Vec::new(); self.collect_stale_reciprocal_neighbor_updates( @@ -4250,11 +4248,7 @@ impl Tds { context: "clearing old back-reference".to_string(), }, })?; - let back_ref = old_neighbor_cell - .neighbors() - .and_then(|neighbor_neighbors| neighbor_neighbors.get(mirror_idx)) - .copied() - .flatten(); + let back_ref = old_neighbor_cell.neighbor_key(mirror_idx).flatten(); if back_ref == Some(cell_key) { reciprocal_updates.push((old_neighbor_key, mirror_idx, None)); } @@ -4300,11 +4294,7 @@ impl Tds { context: "setting back-reference".to_string(), }, })?; - let existing_back_ref = neighbor_cell - .neighbors() - .and_then(|neighbor_neighbors| neighbor_neighbors.get(mirror_idx)) - .copied() - .flatten(); + let existing_back_ref = neighbor_cell.neighbor_key(mirror_idx).flatten(); if let Some(existing_back_ref) = existing_back_ref && existing_back_ref != cell_key { @@ -4375,7 +4365,11 @@ impl Tds { /// let cell_key = tds.cell_keys().next().unwrap(); /// let neighbors = vec![None; 3]; /// tds.set_neighbors_by_key(cell_key, &neighbors).unwrap(); - /// assert!(tds.cell(cell_key).unwrap().neighbors().is_none()); + /// assert!(tds + /// .cell(cell_key) + /// .unwrap() + /// .neighbors() + /// .is_some_and(|mut neighbors| neighbors.all(|neighbor| neighbor.is_none()))); /// ``` pub fn set_neighbors_by_key( &mut self, @@ -4397,7 +4391,7 @@ impl Tds { context: "set_neighbors_by_key".to_string(), })?; let cell_uuid = cell.uuid(); - Self::set_cell_neighbors_normalized(cell, neighbors); + Self::set_cell_neighbors_normalized(cell, neighbors)?; cell_uuid }; @@ -4509,7 +4503,7 @@ impl Tds { /// let dt = DelaunayTriangulation::new(&vertices).unwrap(); /// let mut tds = dt.tds().clone(); /// tds.assign_incident_cells().unwrap(); - /// let all_assigned = tds.vertices().all(|(_, v)| v.incident_cell.is_some()); + /// let all_assigned = tds.vertices().all(|(_, v)| v.incident_cell().is_some()); /// assert!(all_assigned); /// ``` pub fn assign_incident_cells(&mut self) -> Result<(), TdsMutationError> { @@ -4517,7 +4511,7 @@ impl Tds { // No cells remain; all vertices must have incident_cell cleared to avoid // dangling pointers to previously removed cells. for vertex in self.vertices.values_mut() { - vertex.incident_cell = None; + vertex.set_incident_cell(None); } self.bump_generation(); return Ok(()); @@ -4527,7 +4521,7 @@ impl Tds { // ensures vertices that no longer belong to any cell do not retain stale // incident_cell pointers after topology changes (e.g., vertex or cell removal). for vertex in self.vertices.values_mut() { - vertex.incident_cell = None; + vertex.set_incident_cell(None); } // Single-pass rebuild: assign the first cell encountered for each vertex. @@ -4544,8 +4538,8 @@ impl Tds { .into()); }; - if vertex.incident_cell.is_none() { - vertex.incident_cell = Some(cell_key); + if vertex.incident_cell().is_none() { + vertex.set_incident_cell(Some(cell_key)); } } } @@ -4678,12 +4672,12 @@ impl Tds { cell_key, context: "orientation normalization traversal".to_string(), })?; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for (facet_idx, neighbor_key_opt) in neighbors.iter().enumerate() { - let Some(neighbor_key) = *neighbor_key_opt else { + for (facet_idx, neighbor_key_opt) in neighbors.enumerate() { + let Some(neighbor_key) = neighbor_key_opt else { continue; }; if neighbor_key == cell_key && Self::allows_periodic_self_neighbor(cell) { @@ -4785,12 +4779,12 @@ impl Tds { cell_key, context: "local orientation validation scope".to_string(), })?; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for (facet_idx, neighbor_key_opt) in neighbors.iter().enumerate() { - let Some(neighbor_key) = *neighbor_key_opt else { + for (facet_idx, neighbor_key_opt) in neighbors.enumerate() { + let Some(neighbor_key) = neighbor_key_opt else { continue; }; if neighbor_key == cell_key && Self::allows_periodic_self_neighbor(cell) { @@ -4823,11 +4817,7 @@ impl Tds { context: "local orientation validation".to_string(), }, })?; - let observed_back_reference = neighbor_cell - .neighbors() - .and_then(|neighbors| neighbors.get(mirror_idx)) - .copied() - .flatten(); + let observed_back_reference = neighbor_cell.neighbor_key(mirror_idx).flatten(); if observed_back_reference != Some(cell_key) { return Err(TdsError::InvalidNeighbors { reason: NeighborValidationError::BackReferenceMismatch { @@ -5221,7 +5211,7 @@ impl Tds { /// - reference a cell that actually contains the vertex. fn validate_vertex_incidence(&self) -> Result<(), TdsError> { for (vertex_key, vertex) in &self.vertices { - let Some(incident_cell_key) = vertex.incident_cell else { + let Some(incident_cell_key) = vertex.incident_cell() else { continue; }; @@ -5425,12 +5415,12 @@ impl Tds { /// β€” facet-extraction helpers encounter invalid indices or periodic-offset count mismatches. fn validate_coherent_orientation(&self) -> Result<(), TdsError> { for (cell_key, cell) in &self.cells { - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for (facet_idx, neighbor_key_opt) in neighbors.iter().enumerate() { - let Some(neighbor_key) = *neighbor_key_opt else { + for (facet_idx, neighbor_key_opt) in neighbors.enumerate() { + let Some(neighbor_key) = neighbor_key_opt else { continue; // Boundary facet }; @@ -5461,10 +5451,8 @@ impl Tds { }, })?; let back_neighbor = neighbor_cell - .neighbors() - .and_then(|neighbor_neighbors| { - neighbor_neighbors.get(mirror_idx).copied().flatten() - }) + .neighbor_key(mirror_idx) + .flatten() .ok_or_else(|| TdsError::InvalidNeighbors { reason: NeighborValidationError::BackReferenceMismatch { cell_key, @@ -6086,100 +6074,8 @@ impl Tds { ) -> Result<(), TdsError> { for (facet_key, cell_facet_pairs) in facet_to_cells { match cell_facet_pairs.as_slice() { - [handle] => { - // Boundary facet: must not have a neighbor across this facet. - let cell_key = handle.cell_key(); - let facet_index = handle.facet_index() as usize; - - let cell = self - .cells - .get(cell_key) - .ok_or_else(|| TdsError::CellNotFound { - cell_key, - context: "neighbor validation (boundary facet)".to_string(), - })?; - - if let Some(neighbors) = cell.neighbors() { - let neighbor = neighbors.get(facet_index).and_then(|n| *n); - if let Some(neighbor_key) = neighbor { - // Periodic quotient triangulations may encode this as self-adjacency. - if neighbor_key == cell_key { - if Self::allows_periodic_self_neighbor(cell) { - continue; - } - return Err(TdsError::InvalidNeighbors { - reason: - NeighborValidationError::BoundaryFacetHasNonPeriodicSelfNeighbor { - facet_key: *facet_key, - cell_key, - cell_uuid: cell.uuid(), - facet_index, - }, - }); - } - return Err(TdsError::InvalidNeighbors { - reason: NeighborValidationError::BoundaryFacetHasNeighbor { - facet_key: *facet_key, - cell_key, - cell_uuid: cell.uuid(), - facet_index, - neighbor_key, - }, - }); - } - } - } - [a, b] => { - // Interior facet: both cells must be neighbors across the corresponding facet indices. - let first_cell_key = a.cell_key(); - let first_facet_index = a.facet_index() as usize; - let second_cell_key = b.cell_key(); - let second_facet_index = b.facet_index() as usize; - - let first_cell = - self.cells - .get(first_cell_key) - .ok_or_else(|| TdsError::CellNotFound { - cell_key: first_cell_key, - context: "neighbor validation (interior facet, first cell)" - .to_string(), - })?; - let second_cell = - self.cells - .get(second_cell_key) - .ok_or_else(|| TdsError::CellNotFound { - cell_key: second_cell_key, - context: "neighbor validation (interior facet, second cell)" - .to_string(), - })?; - - let first_neighbor = first_cell - .neighbors() - .and_then(|n| n.get(first_facet_index)) - .and_then(|n| *n); - let second_neighbor = second_cell - .neighbors() - .and_then(|n| n.get(second_facet_index)) - .and_then(|n| *n); - - if first_neighbor != Some(second_cell_key) - || second_neighbor != Some(first_cell_key) - { - return Err(TdsError::InvalidNeighbors { - reason: NeighborValidationError::InteriorFacetNeighborMismatch { - facet_key: *facet_key, - first_cell_key, - first_cell_uuid: first_cell.uuid(), - first_facet_index, - first_neighbor, - second_cell_key, - second_cell_uuid: second_cell.uuid(), - second_facet_index, - second_neighbor, - }, - }); - } - } + [handle] => self.validate_boundary_facet_neighbor_pointer(*facet_key, *handle)?, + [a, b] => self.validate_interior_facet_neighbor_pointer(*facet_key, *a, *b)?, _ => { // Non-manifold facet multiplicity should have been caught by facet-sharing validation. return Err(TdsError::InconsistentDataStructure { @@ -6195,18 +6091,130 @@ impl Tds { Ok(()) } + fn validate_boundary_facet_neighbor_pointer( + &self, + facet_key: u64, + handle: FacetHandle, + ) -> Result<(), TdsError> { + let cell_key = handle.cell_key(); + let facet_index = handle.facet_index() as usize; + let cell = self + .cells + .get(cell_key) + .ok_or_else(|| TdsError::CellNotFound { + cell_key, + context: "neighbor validation (boundary facet)".to_string(), + })?; + + let Some(neighbor_slots) = cell.neighbor_slots() else { + return Ok(()); + }; + let Some(neighbor_slot) = neighbor_slots.get(facet_index).copied() else { + return Err(TdsError::InvalidNeighbors { + reason: NeighborValidationError::LengthMismatch { + actual: neighbor_slots.len(), + expected: D + 1, + context: "neighbor validation (boundary facet)".to_string(), + }, + }); + }; + + match neighbor_slot { + NeighborSlot::Unassigned => Err(TdsError::InvalidNeighbors { + reason: NeighborValidationError::UnassignedNeighborSlot { + cell_key, + cell_uuid: cell.uuid(), + facet_index, + context: "neighbor validation (boundary facet)".to_string(), + }, + }), + NeighborSlot::Boundary => Ok(()), + NeighborSlot::Neighbor(neighbor) if neighbor == cell_key => { + if Self::allows_periodic_self_neighbor(cell) { + return Ok(()); + } + Err(TdsError::InvalidNeighbors { + reason: NeighborValidationError::BoundaryFacetHasNonPeriodicSelfNeighbor { + facet_key, + cell_key, + cell_uuid: cell.uuid(), + facet_index, + }, + }) + } + NeighborSlot::Neighbor(neighbor) => Err(TdsError::InvalidNeighbors { + reason: NeighborValidationError::BoundaryFacetHasNeighbor { + facet_key, + cell_key, + cell_uuid: cell.uuid(), + facet_index, + neighbor_key: neighbor, + }, + }), + } + } + + fn validate_interior_facet_neighbor_pointer( + &self, + facet_key: u64, + first: FacetHandle, + second: FacetHandle, + ) -> Result<(), TdsError> { + let first_cell_key = first.cell_key(); + let first_facet_index = first.facet_index() as usize; + let second_cell_key = second.cell_key(); + let second_facet_index = second.facet_index() as usize; + + let first_cell = self + .cells + .get(first_cell_key) + .ok_or_else(|| TdsError::CellNotFound { + cell_key: first_cell_key, + context: "neighbor validation (interior facet, first cell)".to_string(), + })?; + let second_cell = + self.cells + .get(second_cell_key) + .ok_or_else(|| TdsError::CellNotFound { + cell_key: second_cell_key, + context: "neighbor validation (interior facet, second cell)".to_string(), + })?; + + let first_neighbor = first_cell.neighbor_key(first_facet_index).flatten(); + let second_neighbor = second_cell.neighbor_key(second_facet_index).flatten(); + + if first_neighbor == Some(second_cell_key) && second_neighbor == Some(first_cell_key) { + return Ok(()); + } + + Err(TdsError::InvalidNeighbors { + reason: NeighborValidationError::InteriorFacetNeighborMismatch { + facet_key, + first_cell_key, + first_cell_uuid: first_cell.uuid(), + first_facet_index, + first_neighbor, + second_cell_key, + second_cell_uuid: second_cell.uuid(), + second_facet_index, + second_neighbor, + }, + }) + } + fn validate_neighbors_with_precomputed_vertex_sets( &self, cell_vertices: &CellVerticesMap, ) -> Result<(), TdsError> { for (cell_key, cell) in &self.cells { // Phase 3A: Use neighbors (CellKey-based) instead of neighbor UUIDs - let Some(neighbors_buf) = &cell.neighbors else { + let Some(neighbor_keys) = cell.neighbor_keys() else { continue; // Skip cells without neighbors }; + let neighbors_buf: NeighborBuffer> = neighbor_keys.collect(); // Validate topological invariant (neighbor[i] opposite vertex[i]) - self.validate_neighbor_topology(cell_key, neighbors_buf)?; + self.validate_neighbor_topology(cell_key, &neighbors_buf)?; let this_vertices = cell_vertices.get(&cell_key).ok_or_else(|| { TdsError::InconsistentDataStructure { @@ -6217,14 +6225,14 @@ impl Tds { } })?; - for (facet_idx, neighbor_key_opt) in neighbors_buf.iter().enumerate() { + for (facet_idx, neighbor_key_opt) in neighbors_buf.iter().copied().enumerate() { // Skip None neighbors (missing neighbors) let Some(neighbor_key) = neighbor_key_opt else { continue; }; // Self-adjacency is valid for periodic quotient triangulations. - if *neighbor_key == cell_key { + if neighbor_key == cell_key { if Self::allows_periodic_self_neighbor(cell) { continue; } @@ -6238,19 +6246,19 @@ impl Tds { } // Early termination: check if neighbor exists - let Some(neighbor_cell) = self.cells.get(*neighbor_key) else { + let Some(neighbor_cell) = self.cells.get(neighbor_key) else { return Err(TdsError::InvalidNeighbors { reason: NeighborValidationError::MissingNeighborCell { cell_key, cell_uuid: cell.uuid(), facet_index: facet_idx, - neighbor_key: *neighbor_key, + neighbor_key, context: "precomputed neighbor validation".to_string(), }, }); }; - let neighbor_vertices = cell_vertices.get(neighbor_key).ok_or_else(|| { + let neighbor_vertices = cell_vertices.get(&neighbor_key).ok_or_else(|| { TdsError::InconsistentDataStructure { message: format!( "Neighbor cell {} (key {neighbor_key:?}) missing from precomputed vertex set map during neighbor validation", @@ -6286,7 +6294,7 @@ impl Tds { cell_key, cell, facet_idx, - *neighbor_key, + neighbor_key, neighbor_cell, mirror_idx, )?; @@ -6455,7 +6463,7 @@ impl Tds { neighbor_cell: &Cell, mirror_idx: usize, ) -> Result<(), TdsError> { - let Some(neighbor_neighbors) = &neighbor_cell.neighbors else { + let Some(back_ref) = neighbor_cell.neighbor_key(mirror_idx) else { return Err(TdsError::InvalidNeighbors { reason: NeighborValidationError::BackReferenceMismatch { cell_key, @@ -6470,7 +6478,6 @@ impl Tds { }); }; - let back_ref = neighbor_neighbors.get(mirror_idx).copied().flatten(); if back_ref != Some(cell_key) { return Err(TdsError::InvalidNeighbors { reason: NeighborValidationError::BackReferenceMismatch { @@ -7338,7 +7345,7 @@ mod tests { neighbors.push(Some(removed_target_key)); neighbors.push(None); neighbors.push(None); - bad_cell_mut.neighbors = Some(neighbors); + bad_cell_mut.set_neighbors_from_keys(neighbors).unwrap(); } let removed_count = tds.repair_degenerate_cells(); @@ -7522,10 +7529,10 @@ mod tests { // This is the key test for the bug fix for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (i, neighbor_opt) in neighbors.iter().enumerate() { + for (i, neighbor_opt) in neighbors.enumerate() { if let Some(neighbor_key) = neighbor_opt { assert!( - dt.as_triangulation().tds.cells.contains_key(*neighbor_key), + dt.as_triangulation().tds.cells.contains_key(neighbor_key), "Cell {cell_key:?} has dangling neighbor reference at index {i}: {neighbor_key:?}" ); } @@ -7727,7 +7734,7 @@ mod tests { // CRITICAL CHECK 3: All remaining vertices should have valid incident_cell pointers for (vertex_key, vertex) in dt.vertices() { - if let Some(incident_cell_key) = vertex.incident_cell { + if let Some(incident_cell_key) = vertex.incident_cell() { assert!( dt.as_triangulation() .tds @@ -7899,7 +7906,7 @@ mod tests { tds.cell(cell1) .unwrap() .neighbors() - .is_some_and(|n| n.iter().any(Option::is_some)) + .is_some_and(|mut n| n.any(|neighbor| neighbor.is_some())) ); let gen_before = tds.generation(); @@ -7910,7 +7917,7 @@ mod tests { for (_, cell) in tds.cells() { if let Some(neighbors) = cell.neighbors() { for neighbor_opt in neighbors { - assert_ne!(*neighbor_opt, Some(cell2)); + assert_ne!(neighbor_opt, Some(cell2)); } } } @@ -7937,22 +7944,22 @@ mod tests { tds.assign_neighbors().unwrap(); // Force deterministic incident_cell pointers that require repair. - tds.vertex_mut(a).unwrap().incident_cell = Some(cell1); - tds.vertex_mut(b).unwrap().incident_cell = Some(cell1); - tds.vertex_mut(c).unwrap().incident_cell = Some(cell1); - tds.vertex_mut(d).unwrap().incident_cell = Some(cell2); + tds.vertex_mut(a).unwrap().set_incident_cell(Some(cell1)); + tds.vertex_mut(b).unwrap().set_incident_cell(Some(cell1)); + tds.vertex_mut(c).unwrap().set_incident_cell(Some(cell1)); + tds.vertex_mut(d).unwrap().set_incident_cell(Some(cell2)); assert_eq!(tds.remove_cells_by_keys(&[cell1]), 1); // A is now isolated (no remaining cells contain it) => incident_cell must be None. - assert!(tds.vertex(a).unwrap().incident_cell.is_none()); + assert!(tds.vertex(a).unwrap().incident_cell().is_none()); // B, C, D are still in cell2 => their incident_cell must be valid and contain them. for vk in [b, c, d] { let incident = tds .vertex(vk) .unwrap() - .incident_cell + .incident_cell() .expect("vertex should have an incident cell after repair"); assert!(tds.cells.contains_key(incident)); let cell = tds.cell(incident).unwrap(); @@ -7964,8 +7971,8 @@ mod tests { // Neighbor pointers in the surviving cell must not reference the removed cell. let cell2_ref = tds.cell(cell2).unwrap(); - if let Some(neighbors) = cell2_ref.neighbors() { - assert!(neighbors.iter().all(|n| *n != Some(cell1))); + if let Some(mut neighbors) = cell2_ref.neighbors() { + assert!(neighbors.all(|n| n != Some(cell1))); } } @@ -7992,7 +7999,9 @@ mod tests { // Manually set an incident cell so the vertex appears non-isolated. let fake_cell_key = CellKey::from(KeyData::from_ffi(1)); - tds.vertex_mut(vk).unwrap().incident_cell = Some(fake_cell_key); + tds.vertex_mut(vk) + .unwrap() + .set_incident_cell(Some(fake_cell_key)); let gen_before = tds.generation(); tds.remove_isolated_vertex(vk); @@ -8011,7 +8020,7 @@ mod tests { let uuid = tds.vertex(vk).unwrap().uuid(); // No incident cell set β†’ truly isolated. - assert!(tds.vertex(vk).unwrap().incident_cell.is_none()); + assert!(tds.vertex(vk).unwrap().incident_cell().is_none()); let gen_before = tds.generation(); tds.remove_isolated_vertex(vk); @@ -8053,10 +8062,18 @@ mod tests { // - ORIGIN points at cell1 so star discovery can use the neighbor-walk fast path. // - EAST/NORTH point at cell1 so removal must repair them to cell2. // - DIAGONAL points at cell2 and should remain valid. - tds.vertex_mut(origin_key).unwrap().incident_cell = Some(cell1); - tds.vertex_mut(east_key).unwrap().incident_cell = Some(cell1); - tds.vertex_mut(north_key).unwrap().incident_cell = Some(cell1); - tds.vertex_mut(diagonal_key).unwrap().incident_cell = Some(cell2); + tds.vertex_mut(origin_key) + .unwrap() + .set_incident_cell(Some(cell1)); + tds.vertex_mut(east_key) + .unwrap() + .set_incident_cell(Some(cell1)); + tds.vertex_mut(north_key) + .unwrap() + .set_incident_cell(Some(cell1)); + tds.vertex_mut(diagonal_key) + .unwrap() + .set_incident_cell(Some(cell2)); let origin_uuid = tds.vertex(origin_key).unwrap().uuid(); let removed = tds.remove_vertex(origin_key).unwrap(); @@ -8069,14 +8086,14 @@ mod tests { // cell2 should remain and must not reference cell1 as a neighbor. assert!(tds.cells.contains_key(cell2)); let cell2_ref = tds.cell(cell2).unwrap(); - if let Some(neighbors) = cell2_ref.neighbors() { - assert!(neighbors.iter().all(|n| *n != Some(cell1))); + if let Some(mut neighbors) = cell2_ref.neighbors() { + assert!(neighbors.all(|n| n != Some(cell1))); } // Remaining vertices must have valid incident_cell pointers (if present). for vertex_key in [east_key, north_key, diagonal_key] { let v = tds.vertex(vertex_key).unwrap(); - let Some(incident) = v.incident_cell else { + let Some(incident) = v.incident_cell() else { panic!("vertex {vertex_key:?} should have an incident cell after removal"); }; assert!(tds.cells.contains_key(incident)); @@ -8147,10 +8164,14 @@ mod tests { tds.set_neighbors_by_key(cell1, &[None, None, Some(cell2)]) .unwrap(); - let cell1_neighbors = tds.cell(cell1).unwrap().neighbors().unwrap(); - let cell2_neighbors = tds.cell(cell2).unwrap().neighbors().unwrap(); - assert_eq!(cell1_neighbors[2], Some(cell2)); - assert_eq!(cell2_neighbors[2], Some(cell1)); + assert_eq!( + tds.cell(cell1).unwrap().neighbor_key(2).flatten(), + Some(cell2) + ); + assert_eq!( + tds.cell(cell2).unwrap().neighbor_key(2).flatten(), + Some(cell1) + ); let facet_to_cells = tds.build_facet_to_cells_map().unwrap(); tds.validate_neighbors_with_facet_to_cells_map(&facet_to_cells) .unwrap(); @@ -8689,11 +8710,10 @@ mod tests { { let cell2_mut = tds.cell_mut(cell2_key).unwrap(); let neighbors = cell2_mut - .neighbors - .as_mut() + .neighbor_slots_mut() .expect("cell2 should have neighbors after assign_neighbors()"); // For (v_a, v_b, v_d), the shared edge with cell1 is opposite v_d => index 2. - neighbors[2] = None; + neighbors[2] = NeighborSlot::Boundary; } let cell1 = tds.cell(cell1_key).unwrap(); @@ -8725,7 +8745,8 @@ mod tests { { let cell = tds.cell_mut(cell_key).unwrap(); - cell.neighbors = Some(vec![Some(cell_key), None, None].into()); + cell.set_neighbors_from_keys(vec![Some(cell_key), None, None]) + .unwrap(); } let cell = tds.cell(cell_key).unwrap(); @@ -8764,7 +8785,8 @@ mod tests { { let cell = tds.cell_mut(cell_key).unwrap(); - cell.neighbors = Some(vec![Some(cell_key), None, None].into()); + cell.set_neighbors_from_keys(vec![Some(cell_key), None, None]) + .unwrap(); cell.set_periodic_vertex_offsets(vec![[0, 0], [0, 0], [0, 0]]) .unwrap(); } @@ -8776,6 +8798,74 @@ mod tests { assert!(tds.normalize_coherent_orientation().is_ok()); } + #[test] + fn test_boundary_facet_validation_rejects_unassigned_neighbor_slot() { + let mut tds: Tds = Tds::empty(); + + let v0 = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); + let v1 = tds.insert_vertex_with_mapping(vertex!([1.0, 0.0])).unwrap(); + let v2 = tds.insert_vertex_with_mapping(vertex!([0.0, 1.0])).unwrap(); + let cell_key = tds + .insert_cell_with_mapping(Cell::new(vec![v0, v1, v2], None).unwrap()) + .unwrap(); + tds.assign_neighbors().unwrap(); + let facet_to_cells = tds.build_facet_to_cells_map().unwrap(); + + { + let cell = tds.cell_mut(cell_key).unwrap(); + cell.ensure_neighbors_buffer_mut()[1] = NeighborSlot::Unassigned; + } + + let err = tds + .validate_neighbor_pointers_match_facet_to_cells_map(&facet_to_cells) + .unwrap_err(); + + assert!(matches!( + err, + TdsError::InvalidNeighbors { + reason: NeighborValidationError::UnassignedNeighborSlot { + cell_key: key, + facet_index: 1, + .. + }, + } if key == cell_key + )); + } + + #[test] + fn test_boundary_facet_validation_rejects_non_periodic_self_neighbor() { + let mut tds: Tds = Tds::empty(); + + let v0 = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); + let v1 = tds.insert_vertex_with_mapping(vertex!([1.0, 0.0])).unwrap(); + let v2 = tds.insert_vertex_with_mapping(vertex!([0.0, 1.0])).unwrap(); + let cell_key = tds + .insert_cell_with_mapping(Cell::new(vec![v0, v1, v2], None).unwrap()) + .unwrap(); + tds.assign_neighbors().unwrap(); + let facet_to_cells = tds.build_facet_to_cells_map().unwrap(); + + { + let cell = tds.cell_mut(cell_key).unwrap(); + cell.ensure_neighbors_buffer_mut()[2] = NeighborSlot::Neighbor(cell_key); + } + + let err = tds + .validate_neighbor_pointers_match_facet_to_cells_map(&facet_to_cells) + .unwrap_err(); + + assert!(matches!( + err, + TdsError::InvalidNeighbors { + reason: NeighborValidationError::BoundaryFacetHasNonPeriodicSelfNeighbor { + cell_key: key, + facet_index: 2, + .. + }, + } if key == cell_key + )); + } + #[test] fn test_orientation_validation_rejects_one_way_neighbor_pointer() { let mut tds: Tds = Tds::empty(); @@ -8796,12 +8886,11 @@ mod tests { let mirror_idx = { let cell1 = tds.cell(cell1_key).unwrap(); - let neighbors = cell1 + let mut neighbors = cell1 .neighbors() .expect("cell1 should have neighbors after assign_neighbors()"); let facet_idx = neighbors - .iter() - .position(|n| *n == Some(cell2_key)) + .position(|n| n == Some(cell2_key)) .expect("cell1 should reference cell2"); let cell2 = tds.cell(cell2_key).unwrap(); cell1 @@ -8812,10 +8901,9 @@ mod tests { { let cell2 = tds.cell_mut(cell2_key).unwrap(); let neighbors = cell2 - .neighbors - .as_mut() + .neighbor_slots_mut() .expect("cell2 should have neighbors after assign_neighbors()"); - neighbors[mirror_idx] = None; + neighbors[mirror_idx] = NeighborSlot::Boundary; } let err = tds.validate_coherent_orientation().unwrap_err(); @@ -8945,12 +9033,13 @@ mod tests { let vkey = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); // Corrupt incident_cell and ensure it gets cleared. - tds.vertex_mut(vkey).unwrap().incident_cell = - Some(CellKey::from(KeyData::from_ffi(u64::MAX))); - assert!(tds.vertex(vkey).unwrap().incident_cell.is_some()); + tds.vertex_mut(vkey) + .unwrap() + .set_incident_cell(Some(CellKey::from(KeyData::from_ffi(u64::MAX)))); + assert!(tds.vertex(vkey).unwrap().incident_cell().is_some()); tds.assign_incident_cells().unwrap(); - assert!(tds.vertex(vkey).unwrap().incident_cell.is_none()); + assert!(tds.vertex(vkey).unwrap().incident_cell().is_none()); } #[test] @@ -9398,10 +9487,14 @@ mod tests { // Assign neighbors: shared facet is edge v1-v2. // c0[v0,v1,v2]: facet opposite v0 (index 0) = edge [v1,v2] β†’ neighbor c1 // c1[v1,v2,v3]: facet opposite v3 (index 2) = edge [v1,v2] β†’ neighbor c0 - tds.cell_mut(c0).unwrap().neighbors = - Some(NeighborBuffer::from_iter([Some(c1), None, None])); - tds.cell_mut(c1).unwrap().neighbors = - Some(NeighborBuffer::from_iter([None, None, Some(c0)])); + tds.cell_mut(c0) + .unwrap() + .set_neighbors_from_keys([Some(c1), None, None]) + .unwrap(); + tds.cell_mut(c1) + .unwrap() + .set_neighbors_from_keys([None, None, Some(c0)]) + .unwrap(); tds.normalize_coherent_orientation().unwrap(); assert!(tds.is_coherently_oriented()); @@ -9707,7 +9800,7 @@ mod tests { // Multi-cell: cells that share facets have Some(neighbor) entries let has_any_neighbor = tds.cells().any(|(_, cell)| { cell.neighbors() - .is_some_and(|nb| nb.iter().any(Option::is_some)) + .is_some_and(|mut nb| nb.any(|neighbor| neighbor.is_some())) }); assert!(has_any_neighbor); @@ -10057,7 +10150,7 @@ mod tests { let ck2 = tds .insert_cell_with_mapping(Cell::new(vec![v1, v2, v3], None).unwrap()) .unwrap(); - tds.vertex_mut(v0).unwrap().incident_cell = Some(ck2); + tds.vertex_mut(v0).unwrap().set_incident_cell(Some(ck2)); let err = tds.validate_vertex_incidence().unwrap_err(); assert!(matches!(err, TdsError::InconsistentDataStructure { .. })); @@ -10076,7 +10169,9 @@ mod tests { // Point v0 at a non-existent cell key. let dangling = CellKey::from(KeyData::from_ffi(0xDEAD)); - tds.vertex_mut(v0).unwrap().incident_cell = Some(dangling); + tds.vertex_mut(v0) + .unwrap() + .set_incident_cell(Some(dangling)); let err = tds.validate_vertex_incidence().unwrap_err(); assert!(matches!(err, TdsError::CellNotFound { .. })); @@ -10100,6 +10195,40 @@ mod tests { assert_eq!(cells.len(), 1); } + #[test] + fn test_find_cells_containing_vertex_falls_back_on_unassigned_neighbor_slot() { + let mut tds: Tds = Tds::empty(); + let v0 = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); + let v1 = tds.insert_vertex_with_mapping(vertex!([1.0, 0.0])).unwrap(); + let v2 = tds.insert_vertex_with_mapping(vertex!([0.0, 1.0])).unwrap(); + let v3 = tds + .insert_vertex_with_mapping(vertex!([-1.0, 1.0])) + .unwrap(); + + let first_cell = tds + .insert_cell_with_mapping(Cell::new(vec![v0, v1, v2], None).unwrap()) + .unwrap(); + let second_cell = tds + .insert_cell_with_mapping(Cell::new(vec![v0, v2, v3], None).unwrap()) + .unwrap(); + tds.assign_neighbors().unwrap(); + tds.assign_incident_cells().unwrap(); + tds.vertex_mut(v0) + .unwrap() + .set_incident_cell(Some(first_cell)); + + { + let cell = tds.cell_mut(first_cell).unwrap(); + cell.ensure_neighbors_buffer_mut()[0] = NeighborSlot::Unassigned; + } + + let cells = tds.find_cells_containing_vertex_by_key(v0); + + assert_eq!(cells.len(), 2); + assert!(cells.contains(&first_cell)); + assert!(cells.contains(&second_cell)); + } + // ========================================================================= // REMOVE CELLS BY KEYS: BATCH REPAIR // ========================================================================= @@ -10124,7 +10253,7 @@ mod tests { // All remaining vertex incident_cell pointers should be valid. for (vk, v) in tds.vertices() { - if let Some(ic) = v.incident_cell { + if let Some(ic) = v.incident_cell() { assert!( tds.contains_cell(ic), "Vertex {vk:?} has dangling incident_cell after batch removal" @@ -10135,8 +10264,8 @@ mod tests { // No surviving cell should have a neighbor pointer to the removed cell. for (_, cell) in tds.cells() { if let Some(neighbors) = cell.neighbors() { - for nk in neighbors.iter().flatten() { - assert_ne!(*nk, first_ck, "Dangling neighbor pointer to removed cell"); + for nk in neighbors.flatten() { + assert_ne!(nk, first_ck, "Dangling neighbor pointer to removed cell"); } } } @@ -10287,7 +10416,7 @@ mod tests { .unwrap() .neighbors() .unwrap() - .contains(&Some(removed_key)) + .any(|neighbor| neighbor == Some(removed_key)) ); let removed_uuid = tds.cells.get(removed_key).unwrap().uuid(); @@ -10299,31 +10428,27 @@ mod tests { assert!(!tds.cells.contains_key(removed_key)); assert!(tds.cells.contains_key(surviving_key)); assert!(!tds.uuid_to_cell_key.contains_key(&removed_uuid)); - if let Some(neighbors) = tds.cells.get(surviving_key).unwrap().neighbors() { - assert!( - neighbors - .iter() - .all(|neighbor| *neighbor != Some(removed_key)) - ); + if let Some(mut neighbors) = tds.cells.get(surviving_key).unwrap().neighbors() { + assert!(neighbors.all(|neighbor| neighbor != Some(removed_key))); } assert_ne!( - tds.vertices.get(v0).unwrap().incident_cell, + tds.vertices.get(v0).unwrap().incident_cell(), Some(removed_key) ); assert_ne!( - tds.vertices.get(v1).unwrap().incident_cell, + tds.vertices.get(v1).unwrap().incident_cell(), Some(removed_key) ); assert_ne!( - tds.vertices.get(v2).unwrap().incident_cell, + tds.vertices.get(v2).unwrap().incident_cell(), Some(removed_key) ); assert_eq!( - tds.vertices.get(v0).unwrap().incident_cell, + tds.vertices.get(v0).unwrap().incident_cell(), Some(surviving_key) ); assert_eq!( - tds.vertices.get(v2).unwrap().incident_cell, + tds.vertices.get(v2).unwrap().incident_cell(), Some(surviving_key) ); } diff --git a/src/core/triangulation.rs b/src/core/triangulation.rs index c07826a6..7a875403 100644 --- a/src/core/triangulation.rs +++ b/src/core/triangulation.rs @@ -1194,11 +1194,11 @@ where continue; }; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &n_opt in neighbors { + for n_opt in neighbors { let Some(nk) = n_opt else { continue; }; @@ -2013,9 +2013,11 @@ where pub fn cell_neighbors(&self, c: CellKey) -> impl Iterator + '_ { self.tds .cell(c) - .and_then(|cell| cell.neighbors()) + .and_then(Cell::neighbors) .into_iter() - .flat_map(|neighbors| neighbors.iter().copied().flatten()) + .flat_map(IntoIterator::into_iter) + .flatten() + .filter(|&neighbor_key| self.tds.contains_cell(neighbor_key)) } /// Returns an iterator over all neighbors of a cell using a precomputed [`AdjacencyIndex`]. @@ -2356,11 +2358,11 @@ where } // Cell β†’ neighbors - if let Some(neighbors) = cell.neighbors() { + if let Some(neighbors) = cell.neighbor_keys() { let mut neighs: SmallBuffer = SmallBuffer::new(); - for &n_opt in neighbors { + for n_opt in neighbors { let Some(nk) = n_opt else { continue; }; @@ -2597,8 +2599,8 @@ where let vertex_keys: SmallBuffer = cell.vertices().iter().copied().collect(); let neighbor_keys: SmallBuffer, MAX_PRACTICAL_DIMENSION_SIZE> = - cell.neighbors() - .map(|n| n.iter().copied().collect()) + cell.neighbor_keys() + .map(Iterator::collect) .unwrap_or_default(); tracing::debug!( cell_uuid = %cell.uuid(), @@ -3407,7 +3409,7 @@ where /// Build initial D-simplex from D+1 vertices with degeneracy validation. /// /// This creates a Tds with a single cell containing all D+1 vertices, - /// with no neighbor relationships (all boundary facets). The simplex is + /// with explicit boundary neighbor slots. The simplex is /// validated to ensure it is non-degenerate (vertices span full D-dimensional space). /// /// **Design Note**: This method uses [`robust_orientation`] directly for the @@ -3561,6 +3563,10 @@ where // Insert the cell let _cell_key = tds.insert_cell_with_mapping(cell)?; + // Assign explicit boundary neighbor slots for the initial simplex. + tds.assign_neighbors() + .map_err(TdsConstructionError::ValidationError)?; + // Assign incident cells to vertices (each vertex points to this one cell) // This is required for proper Tds structure tds.assign_incident_cells() @@ -4088,7 +4094,7 @@ where return true; }; - let Some(cell_key) = vertex.incident_cell else { + let Some(cell_key) = vertex.incident_cell() else { return true; }; @@ -4855,8 +4861,10 @@ where // This avoids treating one-way neighbor pointers as β€œconnected”. if let Some(neighbor_cell) = self.tds.cell(nk) && neighbor_cell - .neighbors() - .is_some_and(|ns| ns.contains(&Some(ck))) + .neighbor_keys() + .is_some_and(|mut neighbor_keys| { + neighbor_keys.any(|neighbor| neighbor == Some(ck)) + }) { touches_existing_cells = true; } @@ -5174,9 +5182,9 @@ where if !saw_ridge_fan_shrink { for &dc in disconnected_cells { if let Some(cell) = self.tds.cell(dc) - && let Some(neighbors) = cell.neighbors() + && let Some(neighbors) = cell.neighbor_keys() { - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { if let Some(nk) = neighbor_opt && !conflict_set.contains(&nk) { @@ -5582,7 +5590,7 @@ where if let Some(incident_cell) = hint && let Some(vertex) = self.tds.vertex_mut(v_key) { - vertex.incident_cell = Some(incident_cell); + vertex.set_incident_cell(Some(incident_cell)); } // Optional debug: validate neighbor pointers by forcing a full facet walk (no hint). @@ -5641,7 +5649,7 @@ where let tds = &self.tds; tds.vertices() .filter(|(vk, v)| { - !v.incident_cell.is_some_and(|cell_key| { + !v.incident_cell().is_some_and(|cell_key| { tds.cell(cell_key) .is_some_and(|cell| cell.contains_vertex(*vk)) }) @@ -5663,7 +5671,7 @@ where .find_map(|(ck, cell)| cell.contains_vertex(vk).then_some(ck)); if let Some(cell_key) = repaired_cell { if let Some(vertex) = self.tds.vertex_mut(vk) { - vertex.incident_cell = Some(cell_key); + vertex.set_incident_cell(Some(cell_key)); } } else { // Truly isolated: no cell in the TDS contains this vertex. @@ -6288,10 +6296,10 @@ where let Some(cell) = self.tds.cell(cell_key) else { continue; }; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &neighbor_opt in neighbors { + for neighbor_opt in neighbors { total_slots = total_slots.saturating_add(1); match neighbor_opt { None => { @@ -6303,8 +6311,10 @@ where } else if self .tds .cell(neighbor_key) - .and_then(|neighbor_cell| neighbor_cell.neighbors()) - .is_some_and(|ns| ns.contains(&Some(cell_key))) + .and_then(Cell::neighbors) + .is_some_and(|mut ns| { + ns.any(|neighbor| neighbor == Some(cell_key)) + }) { neighbor_mutual = neighbor_mutual.saturating_add(1); } else { @@ -6437,7 +6447,7 @@ where if let Some(incident_cell) = hint && let Some(vertex) = self.tds.vertex_mut(v_key) { - vertex.incident_cell = Some(incident_cell); + vertex.set_incident_cell(Some(incident_cell)); } #[cfg(debug_assertions)] @@ -6917,10 +6927,10 @@ where let Some(cell) = self.tds.cell(cell_key) else { continue; }; - let Some(neighbors) = cell.neighbors() else { + let Some(neighbors) = cell.neighbor_keys() else { continue; }; - for &neighbor_key in neighbors.iter().flatten() { + for neighbor_key in neighbors.flatten() { if removal_set.contains(&neighbor_key) || !self.tds.contains_cell(neighbor_key) { continue; } @@ -7048,6 +7058,7 @@ where mod tests { use super::*; use crate::core::algorithms::locate::InternalInconsistencySite; + use crate::core::cell::NeighborSlot; use crate::core::collections::NeighborBuffer; use crate::core::collections::spatial_hash_grid::HashGridIndex; use crate::core::vertex::VertexBuilder; @@ -7098,7 +7109,7 @@ mod tests { .unwrap(); for vk in [v0, v1, v2, v3] { - tri.tds.vertex_mut(vk).unwrap().incident_cell = Some(ck); + tri.tds.vertex_mut(vk).unwrap().set_incident_cell(Some(ck)); } (tri, [v0, v1, v2, v3], ck) @@ -8094,7 +8105,7 @@ mod tests { .unwrap(); { let vertex = tri.tds.vertex_mut(vkey).unwrap(); - vertex.incident_cell = Some(CellKey::default()); + vertex.set_incident_cell(Some(CellKey::default())); } let mut index: HashGridIndex = HashGridIndex::new(1.0); @@ -8361,15 +8372,19 @@ mod tests { // Verify incident cells are assigned for (_, vertex) in tds.vertices() { - assert!(vertex.incident_cell.is_some(), + assert!(vertex.incident_cell().is_some(), "{}D: All vertices should have incident cell assigned", $dim); } - // Verify initial simplex has no neighbors (all boundary facets) - if let Some(neighbors) = cell.neighbors() { - assert!(neighbors.iter().all(|n| n.is_none()), - "{}D: Initial simplex should have no neighbors (all boundary)", $dim); - } + // Verify initial simplex has explicit boundary neighbor slots. + let neighbors = cell + .neighbor_slots() + .expect("initial simplex should assign boundary neighbor slots"); + assert!( + neighbors.iter().all(|slot| *slot == NeighborSlot::Boundary), + "{}D: Initial simplex should have boundary slots", + $dim + ); } } }; @@ -8893,7 +8908,7 @@ mod tests { neighbors.resize(2, None); // e0 = [v0, v1]; across v1 is facet_index=0 neighbors[0] = Some(e1); - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); } { let cell = tds.cell_mut(e1).unwrap(); @@ -8901,7 +8916,7 @@ mod tests { neighbors.resize(2, None); // e1 = [v1, v2]; across v1 is facet_index=1 neighbors[1] = Some(e0); - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); } // Cycle neighbors: @@ -8912,7 +8927,7 @@ mod tests { // c0 = [v3, v4]; across v4 is facet_index=0, across v3 is facet_index=1 neighbors[0] = Some(c1); // at v4 neighbors[1] = Some(c2); // at v3 - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); } { let cell = tds.cell_mut(c1).unwrap(); @@ -8921,7 +8936,7 @@ mod tests { // c1 = [v4, v5]; across v5 is facet_index=0, across v4 is facet_index=1 neighbors[0] = Some(c2); // at v5 neighbors[1] = Some(c0); // at v4 - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); } { let cell = tds.cell_mut(c2).unwrap(); @@ -8930,7 +8945,7 @@ mod tests { // c2 = [v5, v3]; across v3 is facet_index=0, across v5 is facet_index=1 neighbors[0] = Some(c0); // at v3 neighbors[1] = Some(c1); // at v5 - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); } tds.assign_incident_cells().unwrap(); @@ -9003,7 +9018,7 @@ mod tests { let mut neighbors = NeighborBuffer::>::new(); neighbors.resize(4, None); neighbors[0] = Some(second_cell_key); - first_cell.neighbors = Some(neighbors); + first_cell.set_neighbors_from_keys(neighbors).unwrap(); match tds.is_valid() { Err(TdsError::InvalidNeighbors { @@ -9169,9 +9184,7 @@ mod tests { // Corrupt one cell locally: neighbors buffer with the wrong length. let cell_key = tri.tds.cell_keys().next().unwrap(); let cell = tri.tds.cell_mut(cell_key).unwrap(); - let mut bad_neighbors = NeighborBuffer::>::new(); - bad_neighbors.resize(3, None); // expected D+1 = 4 - cell.neighbors = Some(bad_neighbors); + cell.ensure_neighbors_buffer_mut().truncate(3); // expected D+1 = 4 let report = tri.validation_report().unwrap_err(); @@ -9668,12 +9681,16 @@ mod tests { let issues = tri.detect_local_facet_issues(&fake_keys).unwrap(); assert!(issues.is_none(), "{}D: Nonexistent cells should be skipped", $dim); - // Verify neighbors (all should be None for single cell) + // Verify neighbors (all should be explicit boundary slots for a single cell) let (_, cell) = tri.tds.cells().next().unwrap(); - if let Some(neighbors) = cell.neighbors() { - assert!(neighbors.iter().all(|n| n.is_none()), - "{}D: Single cell should have no neighbors", $dim); - } + let neighbors = cell + .neighbor_slots() + .expect("single cell should assign boundary neighbor slots"); + assert!( + neighbors.iter().all(|slot| *slot == NeighborSlot::Boundary), + "{}D: Single cell should have boundary slots", + $dim + ); } #[test] @@ -9740,11 +9757,11 @@ mod tests { // 2. Neighbor relationships are symmetric for (cell_key, cell) in dt.tds().cells() { if let Some(neighbors) = cell.neighbors() { - for (facet_idx, neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_opt) in neighbors.enumerate() { if let Some(neighbor_key) = neighbor_opt { // Verify neighbor exists assert!( - dt.tds().contains_cell(*neighbor_key), + dt.tds().contains_cell(neighbor_key), "{}D: Cell {cell_key:?} has neighbor pointer to non-existent cell {neighbor_key:?}", $dim ); @@ -9752,12 +9769,11 @@ mod tests { // Verify symmetry: neighbor should point back to us let neighbor_cell = dt .tds() - .cell(*neighbor_key) + .cell(neighbor_key) .expect("Neighbor cell should exist"); - if let Some(neighbor_neighbors) = neighbor_cell.neighbors() { + if let Some(mut neighbor_neighbors) = neighbor_cell.neighbors() { let points_back = neighbor_neighbors - .iter() - .any(|n| n.as_ref() == Some(&cell_key)); + .any(|neighbor| neighbor == Some(cell_key)); assert!( points_back, "{}D: Cell {cell_key:?} has neighbor {neighbor_key:?} at facet {facet_idx}, but neighbor doesn't point back", @@ -10234,8 +10250,8 @@ mod tests { { let cell = tri.tds.cell_mut(cell_key).unwrap(); - let neighbors = cell.ensure_neighbors_buffer_mut(); - neighbors[0] = Some(missing_neighbor); + cell.set_neighbors_from_keys([Some(missing_neighbor), None, None]) + .unwrap(); } match tri.build_adjacency_index() { @@ -10250,6 +10266,30 @@ mod tests { } } + #[test] + fn test_cell_neighbors_filters_missing_neighbor_cell() { + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), + ]; + let tds = + Triangulation::, (), (), 2>::build_initial_simplex(&vertices).unwrap(); + let mut tri = + Triangulation::, (), (), 2>::new_with_tds(FastKernel::new(), tds); + let cell_key = tri.tds.cell_keys().next().unwrap(); + let missing_neighbor = CellKey::from(KeyData::from_ffi(u64::MAX)); + assert!(!tri.tds.contains_cell(missing_neighbor)); + + tri.tds + .cell_mut(cell_key) + .unwrap() + .set_neighbors_from_keys([Some(missing_neighbor), None, None]) + .unwrap(); + + assert_eq!(tri.cell_neighbors(cell_key).count(), 0); + } + #[test] fn test_build_adjacency_index_errors_on_missing_vertex_key() { let vertices = vec![ @@ -11031,13 +11071,13 @@ mod tests { // Wire neighbours so EXPAND discovers cell_c via cell_a and cell_d via cell_b. { let cell = tri.tds.cell_mut(cell_a).unwrap(); - let nb = cell.ensure_neighbors_buffer_mut(); - nb[0] = Some(cell_c); + cell.set_neighbors_from_keys([Some(cell_c), None, None]) + .unwrap(); } { let cell = tri.tds.cell_mut(cell_b).unwrap(); - let nb = cell.ensure_neighbors_buffer_mut(); - nb[0] = Some(cell_d); + cell.set_neighbors_from_keys([Some(cell_d), None, None]) + .unwrap(); } let new_v = tri @@ -11277,7 +11317,7 @@ mod tests { // Pointers unchanged. for vk in [v0, v1, v2, v3] { - assert_eq!(tri.tds.vertex_mut(vk).unwrap().incident_cell, Some(ck)); + assert_eq!(tri.tds.vertex_mut(vk).unwrap().incident_cell(), Some(ck)); } } @@ -11286,11 +11326,11 @@ mod tests { let (mut tri, [_, _, _, v3], ck) = build_single_tet(); // Corrupt v3 to have no incident cell. - tri.tds.vertex_mut(v3).unwrap().incident_cell = None; + tri.tds.vertex_mut(v3).unwrap().set_incident_cell(None); assert!(tri.repair_stale_incident_cells().is_ok()); assert_eq!( - tri.tds.vertex_mut(v3).unwrap().incident_cell, + tri.tds.vertex_mut(v3).unwrap().incident_cell(), Some(ck), "v3 should be repaired to point to the tetrahedron" ); @@ -11302,11 +11342,14 @@ mod tests { // Point v3 to a non-existent cell key (simulates a deleted conflict cell). let stale = CellKey::from(KeyData::from_ffi(0xDEAD_BEEF)); - tri.tds.vertex_mut(v3).unwrap().incident_cell = Some(stale); + tri.tds + .vertex_mut(v3) + .unwrap() + .set_incident_cell(Some(stale)); assert!(tri.repair_stale_incident_cells().is_ok()); assert_eq!( - tri.tds.vertex_mut(v3).unwrap().incident_cell, + tri.tds.vertex_mut(v3).unwrap().incident_cell(), Some(ck), "stale pointer should be repaired to the valid cell" ); @@ -11721,9 +11764,7 @@ mod tests { // Corrupt a cell's neighbor buffer length to trigger CellValidity violation. let cell = tri.tds.cell_mut(ck).unwrap(); - let mut bad_neighbors = NeighborBuffer::>::new(); - bad_neighbors.resize(2, None); // wrong: should be D+1 = 4 - cell.neighbors = Some(bad_neighbors); + cell.ensure_neighbors_buffer_mut().truncate(2); // wrong: should be D+1 = 4 let report = tri.validation_report().unwrap_err(); assert!( @@ -11999,10 +12040,7 @@ mod tests { ) -> Option { let cell = tri.tds.cell(cell_key).unwrap(); let facet_idx = shared_edge_facet_index(cell, v_a, v_b); - cell.neighbors() - .and_then(|neighbors| neighbors.get(facet_idx)) - .copied() - .flatten() + cell.neighbor_key(facet_idx).flatten() } #[test] @@ -12034,7 +12072,7 @@ mod tests { let mut neighbors = NeighborBuffer::>::new(); neighbors.resize(3, None); neighbors[2] = Some(neighbor_key); - cell.neighbors = Some(neighbors); + cell.set_neighbors_from_keys(neighbors).unwrap(); } tds.assign_incident_cells().unwrap(); diff --git a/src/core/util/delaunay_validation.rs b/src/core/util/delaunay_validation.rs index f4c47475..0df83407 100644 --- a/src/core/util/delaunay_validation.rs +++ b/src/core/util/delaunay_validation.rs @@ -2,7 +2,10 @@ #![forbid(unsafe_code)] +#[cfg(not(any(test, feature = "diagnostics")))] use crate::core::cell::CellValidationError; +#[cfg(any(test, feature = "diagnostics"))] +use crate::core::cell::{CellValidationError, NeighborSlot}; use crate::core::collections::ViolationBuffer; #[cfg(any(test, feature = "diagnostics"))] use crate::core::collections::{CellVertexBuffer, NeighborBuffer}; @@ -142,6 +145,12 @@ impl DelaunayViolationReport { /// [`cell_key`](Self::cell_key), [`cell_vertices`](Self::cell_vertices), and /// [`offending_vertex`](Self::offending_vertex) to recover full vertex or cell /// records from the source [`Tds`]. +/// +/// [`neighbor_cells`](Self::neighbor_cells) preserves the violating cell's raw +/// [`NeighborSlot`] state for each facet so diagnostics can distinguish +/// [`Boundary`](NeighborSlot::Boundary) hull facets, +/// [`Unassigned`](NeighborSlot::Unassigned) missing wiring, and +/// [`Neighbor`](NeighborSlot::Neighbor) cell links. #[cfg(any(test, feature = "diagnostics"))] #[cfg_attr(docsrs, doc(cfg(feature = "diagnostics")))] #[derive(Clone, Debug, PartialEq, Eq)] @@ -155,7 +164,7 @@ pub struct DelaunayViolationDetail { /// be identified. pub offending_vertex: Option, /// Neighbor slots of the violating cell, preserving facet-index order. - pub neighbor_cells: NeighborBuffer>, + pub neighbor_cells: NeighborBuffer, } // ============================================================================= @@ -246,16 +255,16 @@ where // A.neighbors()[k] = B ("vertex j opposite neighbor j"). if D >= 4 { let is_artifact = 'artifact: { - let Some(a_neighbors) = cell.neighbors() else { + let Some(a_neighbors) = cell.neighbor_keys() else { break 'artifact false; }; let mut b_points: SmallVec<[Point; 8]> = SmallVec::with_capacity(D + 1); - for (k, neighbor_opt) in a_neighbors.iter().enumerate() { + for (k, neighbor_opt) in a_neighbors.enumerate() { let Some(b_key) = neighbor_opt else { continue; }; - let Some(b_cell) = tds.cell(*b_key) else { + let Some(b_cell) = tds.cell(b_key) else { continue; }; // Is test_vkey the apex of B w.r.t. A? @@ -542,10 +551,8 @@ where let cell = tds.cell(cell_key)?; let cell_vertices = cell.vertices().iter().copied().collect(); let neighbor_cells = cell - .neighbors() - .map_or_else(NeighborBuffer::new, |neighbors| { - neighbors.iter().copied().collect() - }); + .neighbor_slots() + .map_or_else(NeighborBuffer::new, |slots| slots.iter().copied().collect()); let offending_vertex = first_offending_vertex(tds, cell_key); Some(DelaunayViolationDetail { @@ -748,24 +755,29 @@ pub fn debug_print_first_delaunay_violation( } // Neighbor information for the first violating cell. - if let Some(neighbors) = cell.neighbors() { - for (facet_idx, neighbor_key_opt) in neighbors.iter().enumerate() { - match neighbor_key_opt { - Some(neighbor_key) => { - if let Some(neighbor_cell) = tds.cell(*neighbor_key) { + if let Some(neighbors) = cell.neighbor_slots() { + for (facet_idx, neighbor_slot) in neighbors.iter().copied().enumerate() { + match neighbor_slot { + NeighborSlot::Neighbor(neighbor_key) => { + if let Some(neighbor_cell) = tds.cell(neighbor_key) { tracing::debug!( - "[Delaunay debug] facet {facet_idx}: neighbor cell {neighbor_key:?}, uuid={}", + "[Delaunay debug] facet {facet_idx}: slot=Assigned, neighbor cell {neighbor_key:?}, uuid={}", neighbor_cell.uuid() ); } else { tracing::debug!( - "[Delaunay debug] facet {facet_idx}: neighbor cell {neighbor_key:?} missing from TDS", + "[Delaunay debug] facet {facet_idx}: slot=Assigned, neighbor cell {neighbor_key:?} missing from TDS", ); } } - None => { + NeighborSlot::Boundary => { + tracing::debug!( + "[Delaunay debug] facet {facet_idx}: slot=Boundary (hull facet)" + ); + } + NeighborSlot::Unassigned => { tracing::debug!( - "[Delaunay debug] facet {facet_idx}: no neighbor (hull facet or unassigned)" + "[Delaunay debug] facet {facet_idx}: slot=Unassigned (missing neighbor wiring)" ); } } @@ -781,7 +793,7 @@ pub fn debug_print_first_delaunay_violation( mod tests { use super::*; use crate::core::algorithms::incremental_insertion::repair_neighbor_pointers; - use crate::core::cell::Cell; + use crate::core::cell::{Cell, NeighborSlot}; use crate::core::triangulation::Triangulation; use crate::core::util::make_uuid; use crate::core::vertex::Vertex; @@ -1030,9 +1042,49 @@ mod tests { assert!(detail.cell_key == cell_1 || detail.cell_key == cell_2); assert_eq!(detail.cell_vertices.len(), 3); assert_eq!(detail.neighbor_cells.len(), 3); + let expected_neighbor = if detail.cell_key == cell_1 { + cell_2 + } else { + cell_1 + }; + assert!( + detail.neighbor_cells.iter().copied().any( + |slot| matches!(slot, NeighborSlot::Neighbor(key) if key == expected_neighbor) + ), + "violation detail should preserve the assigned neighbor slot" + ); assert!(detail.offending_vertex.is_some()); } + #[test] + fn delaunay_violation_detail_preserves_neighbor_slot_state() { + let mut tds: Tds = Tds::empty(); + + let a = tds.insert_vertex_with_mapping(vertex!([0.0, 0.0])).unwrap(); + let b = tds.insert_vertex_with_mapping(vertex!([1.0, 0.0])).unwrap(); + let c = tds.insert_vertex_with_mapping(vertex!([0.0, 1.0])).unwrap(); + let cell_key = tds + .insert_cell_with_mapping(Cell::new(vec![a, b, c], None).unwrap()) + .unwrap(); + tds.assign_neighbors().unwrap(); + + { + let cell = tds.cell_mut(cell_key).unwrap(); + cell.ensure_neighbors_buffer_mut()[1] = NeighborSlot::Unassigned; + } + + let detail = build_violation_detail(&tds, cell_key).unwrap(); + + assert_eq!( + detail.neighbor_cells.as_slice(), + &[ + NeighborSlot::Boundary, + NeighborSlot::Unassigned, + NeighborSlot::Boundary + ] + ); + } + #[test] fn delaunay_violation_report_tracks_requested_subset_size() { init_tracing(); @@ -1102,7 +1154,7 @@ mod tests { Triangulation::, (), (), 2>::build_initial_simplex(&vertices).unwrap(); let cell_key = tds.cell_keys().next().unwrap(); let cell = tds.cell_mut(cell_key).unwrap(); - cell.neighbors = Some(vec![None, None].into()); // wrong length (expected 3) + cell.ensure_neighbors_buffer_mut().truncate(2); // wrong length (expected 3) let err = is_delaunay_property_only(&tds).unwrap_err(); assert!(matches!(err, DelaunayValidationError::InvalidCell { .. })); diff --git a/src/core/vertex.rs b/src/core/vertex.rs index 6eef9867..a1ed9899 100644 --- a/src/core/vertex.rs +++ b/src/core/vertex.rs @@ -311,7 +311,7 @@ pub struct Vertex { /// Note: This field is not serialized because `CellKey` is only valid within /// the current `SlotMap` instance. During deserialization, the TDS automatically /// reconstructs `incident_cell` mappings via `assign_incident_cells()`. - pub incident_cell: Option, + pub(crate) incident_cell: Option, /// Optional data associated with the vertex. pub(crate) data: Option, } @@ -351,6 +351,32 @@ impl Vertex { &self.point } + /// Returns the TDS-managed incident cell pointer for this vertex. + /// + /// The pointer is maintained by topology mutation and repair operations, + /// so callers can inspect it but cannot assign arbitrary cell keys. + /// + /// # Examples + /// + /// ```rust + /// use delaunay::prelude::triangulation::*; + /// + /// let vertices = vec![ + /// vertex!([0.0, 0.0]), + /// vertex!([1.0, 0.0]), + /// vertex!([0.0, 1.0]), + /// ]; + /// let dt = DelaunayTriangulation::new(&vertices).unwrap(); + /// let (_, vertex) = dt.vertices().next().unwrap(); + /// + /// assert!(vertex.incident_cell().is_some()); + /// ``` + #[inline] + #[must_use] + pub const fn incident_cell(&self) -> Option { + self.incident_cell + } + /// Returns a reference to the optional user data associated with this vertex. /// /// # Examples @@ -370,6 +396,12 @@ impl Vertex { pub const fn data(&self) -> Option<&U> { self.data.as_ref() } + + /// Updates the TDS-managed incident cell pointer. + #[inline] + pub(crate) const fn set_incident_cell(&mut self, incident_cell: Option) { + self.incident_cell = incident_cell; + } } // ============================================================================= diff --git a/src/lib.rs b/src/lib.rs index 121735a9..77140739 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1356,6 +1356,7 @@ pub mod prelude { #[cfg_attr(docsrs, doc(cfg(feature = "diagnostics")))] pub mod diagnostics { pub use crate::core::algorithms::locate::verify_conflict_region_completeness; + pub use crate::core::cell::NeighborSlot; pub use crate::core::util::{ DelaunayViolationDetail, DelaunayViolationReport, debug_print_first_delaunay_violation, delaunay_violation_report, diff --git a/src/triangulation/delaunay.rs b/src/triangulation/delaunay.rs index 8b13765a..e457f079 100644 --- a/src/triangulation/delaunay.rs +++ b/src/triangulation/delaunay.rs @@ -11325,7 +11325,11 @@ mod tests { // Corrupt a `Vertex::incident_cell` pointer. let vertex_key = dt.tri.tds.vertices().next().unwrap().0; - dt.tri.tds.vertex_mut(vertex_key).unwrap().incident_cell = Some(CellKey::default()); + dt.tri + .tds + .vertex_mut(vertex_key) + .unwrap() + .set_incident_cell(Some(CellKey::default())); let report = dt.validation_report().unwrap_err(); assert!( diff --git a/src/triangulation/flips.rs b/src/triangulation/flips.rs index 2a8bbd59..fc079c4b 100644 --- a/src/triangulation/flips.rs +++ b/src/triangulation/flips.rs @@ -160,7 +160,7 @@ where /// if let Some(key) = cell_key { /// let has_neighbor = dt.tds().cell(key) /// .and_then(|cell| cell.neighbors()) - /// .map(|neighbors| neighbors.iter().any(|n| n.is_some())) + /// .map(|mut neighbors| neighbors.any(|n| n.is_some())) /// .unwrap_or(false); /// if has_neighbor { /// let facet = FacetHandle::new(key, 0); diff --git a/tests/delaunay_incremental_insertion.rs b/tests/delaunay_incremental_insertion.rs index 455150a9..be05fcf8 100644 --- a/tests/delaunay_incremental_insertion.rs +++ b/tests/delaunay_incremental_insertion.rs @@ -40,7 +40,7 @@ fn assert_neighbors_valid_and_symmetric( let Some(neighbors) = cell.neighbors() else { continue; }; - for (facet_idx, &neighbor_opt) in neighbors.iter().enumerate() { + for (facet_idx, neighbor_opt) in neighbors.into_iter().enumerate() { let Some(neighbor_key) = neighbor_opt else { continue; }; @@ -56,11 +56,7 @@ fn assert_neighbors_valid_and_symmetric( "neighbor {neighbor_key:?} does not share facet {facet_idx} with {cell_key:?}" ) }); - let neighbor_back = neighbor_cell - .neighbors() - .and_then(|neighbor_neighbors| neighbor_neighbors.get(mirror_idx)) - .copied() - .flatten(); + let neighbor_back = neighbor_cell.neighbor_key(mirror_idx).flatten(); assert_eq!( neighbor_back, Some(cell_key), diff --git a/tests/delaunay_repair_fallback.rs b/tests/delaunay_repair_fallback.rs index b29e1a4b..0a2b80d3 100644 --- a/tests/delaunay_repair_fallback.rs +++ b/tests/delaunay_repair_fallback.rs @@ -61,7 +61,7 @@ fn repair_fallback_produces_valid_triangulation() { let mut candidate_facets = Vec::new(); for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (index, neighbor) in neighbors.iter().enumerate() { + for (index, neighbor) in neighbors.enumerate() { if neighbor.is_some() { let facet_index = u8::try_from(index).expect("2D facet index fits in u8"); candidate_facets.push(FacetHandle::new(cell_key, facet_index)); diff --git a/tests/delaunayize_workflow.rs b/tests/delaunayize_workflow.rs index d7f7d77e..674fe361 100644 --- a/tests/delaunayize_workflow.rs +++ b/tests/delaunayize_workflow.rs @@ -281,7 +281,7 @@ fn test_flip_breaks_delaunay_then_delaunayize_restores() { let mut candidate_facets = Vec::new(); for (ck, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (i, n) in neighbors.iter().enumerate() { + for (i, n) in neighbors.enumerate() { if let (Some(_), Ok(idx)) = (n, u8::try_from(i)) { candidate_facets.push(FacetHandle::new(ck, idx)); } @@ -536,7 +536,7 @@ fn test_flip_breaks_then_delaunayize_with_budget_restores_3d() { let mut candidate_facets = Vec::new(); for (ck, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for (i, n) in neighbors.iter().enumerate() { + for (i, n) in neighbors.enumerate() { if let (Some(_), Ok(idx)) = (n, u8::try_from(i)) { candidate_facets.push(FacetHandle::new(ck, idx)); } diff --git a/tests/prelude_exports.rs b/tests/prelude_exports.rs index 22115af1..b162a455 100644 --- a/tests/prelude_exports.rs +++ b/tests/prelude_exports.rs @@ -19,8 +19,9 @@ use delaunay::prelude::collections::{ }; #[cfg(feature = "diagnostics")] use delaunay::prelude::diagnostics::{ - DelaunayViolationDetail, DelaunayViolationReport, debug_print_first_delaunay_violation, - delaunay_violation_report, verify_conflict_region_completeness, + DelaunayViolationDetail, DelaunayViolationReport, NeighborSlot as DiagnosticNeighborSlot, + debug_print_first_delaunay_violation, delaunay_violation_report, + verify_conflict_region_completeness, }; use delaunay::prelude::generators::{RandomPointGenerationError, generate_random_points_seeded}; #[cfg(feature = "diagnostics")] @@ -35,7 +36,7 @@ use delaunay::prelude::ordering::{ use delaunay::prelude::query::ConvexHull; #[cfg(feature = "diagnostics")] use delaunay::prelude::tds::Tds; -use delaunay::prelude::tds::{InvariantErrorSummaryDetail, TdsErrorKind}; +use delaunay::prelude::tds::{InvariantErrorSummaryDetail, NeighborSlot, TdsErrorKind}; use delaunay::prelude::triangulation::construction::{ ConstructionOptions, ConstructionSkipSample, ConstructionSlowInsertionSample, DelaunayConstructionFailure, DelaunayRepairPolicy, DelaunayTriangulation, @@ -133,6 +134,7 @@ fn preludes_cover_bench_apis() -> Result<(), PreludeExportTestError> { assert_send_sync_unpin::(); assert_send_sync_unpin::(); assert_send_sync_unpin::(); + assert!(NeighborSlot::Boundary.is_boundary()); assert_eq!( DegenerateSimplexReason::ZeroOrientation.to_string(), "zero orientation" @@ -279,6 +281,7 @@ fn diagnostics_prelude_covers_opt_in_helpers() -> Result<(), PreludeExportTestEr let report = delaunay_violation_report(&tds, None)?; let _typed_report: DelaunayViolationReport = report; let _typed_detail: Option = None; + assert!(DiagnosticNeighborSlot::Boundary.is_boundary()); let kernel = AdaptiveKernel::new(); let point = Point::new([0.0, 0.0]); diff --git a/tests/proptest_serialization.rs b/tests/proptest_serialization.rs index d0052045..f1fe0d99 100644 --- a/tests/proptest_serialization.rs +++ b/tests/proptest_serialization.rs @@ -200,7 +200,7 @@ macro_rules! test_serialization_properties { let mut original_neighbor_count = 0; for (_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - original_neighbor_count += neighbors.iter().flatten().count(); + original_neighbor_count += neighbors.flatten().count(); } } @@ -216,7 +216,7 @@ macro_rules! test_serialization_properties { let mut deserialized_neighbor_count = 0; for (_key, cell) in deserialized.cells() { if let Some(neighbors) = cell.neighbors() { - deserialized_neighbor_count += neighbors.iter().flatten().count(); + deserialized_neighbor_count += neighbors.flatten().count(); } } diff --git a/tests/proptest_tds.rs b/tests/proptest_tds.rs index 5659fc99..c34e1717 100644 --- a/tests/proptest_tds.rs +++ b/tests/proptest_tds.rs @@ -128,23 +128,19 @@ macro_rules! gen_neighbor_symmetry { let tds = dt.tds(); for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for neighbor_key in neighbors.iter().flatten() { + let cell_neighbors: HashSet<_> = neighbors.flatten().collect(); + for neighbor_key in &cell_neighbors { let found_reciprocal = tds .cell(*neighbor_key) .and_then(|c| c.neighbors()) - .is_some_and(|nn| nn.iter().any(|n| n == &Some(cell_key))); + .is_some_and(|mut nn| nn.any(|n| n == Some(cell_key))); if !found_reciprocal { // Enhanced diagnostics with Jaccard similarity - let cell_neighbors: HashSet<_> = neighbors - .iter() - .flatten() - .copied() - .collect(); let neighbor_neighbors: HashSet<_> = tds .cell(*neighbor_key) .and_then(|c| c.neighbors()) - .map(|nn| nn.iter().flatten().copied().collect()) + .map(|nn| nn.flatten().collect()) .unwrap_or_default(); let similarity = jaccard_index(&cell_neighbors, &neighbor_neighbors) @@ -195,9 +191,9 @@ macro_rules! gen_neighbor_index_semantics { for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { let a_vertices = cell.vertices(); - for (i, nb) in neighbors.iter().enumerate() { + for (i, nb) in neighbors.enumerate() { if let Some(b_key) = nb { - let b_cell = tds.cell(*b_key).unwrap(); + let b_cell = tds.cell(b_key).unwrap(); let b_vertices = b_cell.vertices(); let mut a_facet: SimplexVertexBuffer<_> = a_vertices.iter().enumerate() .filter_map(|(idx, &vk)| (idx != i).then_some(vk)) @@ -212,8 +208,8 @@ macro_rules! gen_neighbor_index_semantics { if b_facet == a_facet { found_j = Some(j); break; } } prop_assert!(found_j.is_some(), "Facet mismatch between neighbor cells"); - if let Some(j) = found_j && let Some(b_neighs) = b_cell.neighbors() { - prop_assert_eq!(b_neighs.get(j).copied().flatten(), Some(cell_key), + if let Some(j) = found_j { + prop_assert_eq!(b_cell.neighbor_key(j).flatten(), Some(cell_key), "Reciprocal neighbor at correct index not found"); } } @@ -527,12 +523,12 @@ macro_rules! gen_high_dim_tds_smoke { for (cell_key, cell) in dt.cells() { if let Some(neighbors) = cell.neighbors() { - for neighbor_key in neighbors.iter().flatten() { + for neighbor_key in neighbors.flatten() { let reciprocal = tds - .cell(*neighbor_key) + .cell(neighbor_key) .and_then(|neighbor| neighbor.neighbors()) - .is_some_and(|neighbor_neighbors| { - neighbor_neighbors.iter().any(|entry| entry == &Some(cell_key)) + .is_some_and(|mut neighbor_neighbors| { + neighbor_neighbors.any(|entry| entry == Some(cell_key)) }); prop_assert!( reciprocal,