refactor!: rename public cell APIs to simplex nomenclature#393
Merged
Conversation
- Replace cell-oriented public types, methods, error variants, maps, and diagnostics with simplex terminology across TDS, triangulation, construction, repair, flips, examples, benches, docs, and tests. - Preserve typed construction and insertion error context for cavity filling, explicit construction summaries, focused preludes, and Delaunay repair diagnostics under the simplex API. - Remove redundant integration tests whose coverage is now exercised by unit, property, and public API tests. - Move Codacy SARIF filtering into a tested support script and simplify Cargo package metadata. BREAKING CHANGE: Cell-named public APIs are renamed to their simplex equivalents, including Cell/CellKey, cell iterators and counts, explicit cell construction, cell-oriented error variants, and cell-named topology maps. Resolves #323
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 223 |
🟢 Coverage 90.72% diff coverage · +0.01% coverage variation
Metric Results Coverage variation ✅ +0.01% coverage variation (-1.00%) Diff coverage ✅ 90.72% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (655ff4c) 61005 55190 90.47% Head commit (e2d4979) 61626 (+621) 55757 (+567) 90.48% (+0.01%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#393) 8576 7780 90.72% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a Codacy SARIF filtering script and workflow hook, continues the public and internal Cell-to-Simplex API migration across core geometry and triangulation code, and refreshes benchmarks, examples, tests, and repository documentation to match the renamed simplex-based interfaces. ChangesSimplex migration and workflow updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
coderabbitai
Bot
added
documentation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
========================================
Coverage 90.45% 90.46%
========================================
Files 61 61
Lines 60801 61422 +621
========================================
+ Hits 55000 55567 +567
- Misses 5801 5855 +54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/core/collections/key_maps.rs (1)
138-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd deprecated compatibility aliases for renamed public key-map types to provide a one-release migration path.
The BREAKING CHANGE in commit 9601f6b renamed public types from
Cell*nomenclature toSimplex*without deprecated aliases, violating the guideline: "New functionality must be additive; ... never silently rename or remove a public item." Downstream users have no safe migration path. Add deprecated aliases pointing to the new names:Compatibility shim
pub type UuidToSimplexKeyMap = FastHashMap<Uuid, SimplexKey>; +#[deprecated(note = "Renamed to UuidToSimplexKeyMap")] +pub type UuidToCellKeyMap = UuidToSimplexKeyMap; @@ pub type SimplexKeySet = FastHashSet<SimplexKey>; +#[deprecated(note = "Renamed to SimplexKeySet")] +pub type CellKeySet = SimplexKeySet; @@ pub type KeyBasedSimplexMap<V> = FastHashMap<SimplexKey, V>; +#[deprecated(note = "Renamed to KeyBasedSimplexMap")] +pub type KeyBasedCellMap<V> = KeyBasedSimplexMap<V>;Also applies to: 227-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/collections/key_maps.rs` around lines 138 - 170, Add deprecated compatibility aliases for the renamed public types so downstream code has a one-release migration path: add #[deprecated(note = "renamed to SimplexKeySet; use SimplexKeySet instead")] pub type CellKeySet = SimplexKeySet; and #[deprecated(note = "renamed to UuidToSimplexKeyMap; use UuidToSimplexKeyMap instead")] pub type UuidToCellKeyMap = UuidToSimplexKeyMap; place these aliases adjacent to the existing SimplexKeySet and UuidToSimplexKeyMap typedefs and ensure they are public so consumers receive deprecation warnings but their code continues to compile.src/core/util/delaunay_validation.rs (1)
740-759:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the debug witness search consistent with the validator.
This loop re-runs
robust_inspheredirectly and skips the D≥4 artifact suppression infirst_delaunay_violation_witness(). In near-degenerate cases the debug output can therefore report a different "offending" vertex than the validator would accept. Reusefirst_offending_vertex()here and only look up the point for logging.♻️ Suggested change
- let mut offending: Option<(VertexKey, Point<T, D>)> = None; - - for (test_vkey, test_vertex) in tds.vertices() { - if simplex_vertex_keys.contains(&test_vkey) { - continue; - } - - match robust_insphere(&simplex_vertex_points, test_vertex.point()) { - Ok(InSphere::INSIDE) => { - offending = Some((test_vkey, *test_vertex.point())); - break; - } - Ok(InSphere::BOUNDARY | InSphere::OUTSIDE) => {} - Err(e) => { - tracing::warn!( - "[Delaunay debug] robust_insphere error while searching for offending vertex in simplex {first_simplex_key:?}: {e}", - ); - } - } - } + let offending = first_offending_vertex(tds, first_simplex_key) + .and_then(|off_vkey| tds.vertex(off_vkey).map(|v| (off_vkey, *v.point())));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/util/delaunay_validation.rs` around lines 740 - 759, The loop re-runs robust_insphere and can disagree with the validator; instead call first_offending_vertex() (the same helper used by first_delaunay_violation_witness()) to get the offending VertexKey for the given simplex and tds, then only look up the Point for logging (use the returned VertexKey to fetch the point from tds.vertices()). Replace the manual robust_insphere iteration in this block with a call to first_offending_vertex() and set offending to Some((vkey, *point)) only after fetching the point for logging; preserve the existing warning path but avoid re-evaluating robust_insphere here.src/lib.rs (1)
996-1058:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep deprecated
Cell*aliases in the public façades for one release.These façade modules now only re-export the simplex names, so existing downstream imports like
delaunay::tds::Cell,CellKey,FacetToCellsMap, orprelude::query::Cellhard-break immediately. Please keep deprecated aliases/re-exports here for one release so downstream users get a migration window instead of an all-at-once rename.As per coding guidelines
New functionality must be additive; use crate::prelude::* (or focused preludes like prelude::triangulation, prelude::query) for ergonomic re-exports; never silently rename or remove a public item.Also applies to: 1113-1136, 1200-1206, 1488-1494, 1609-1625
src/triangulation/builder.rs (1)
1938-1950:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the Phase-2 algorithm docs to match the new quotient-selection implementation.
The code no longer just extracts a “central sub-complex” and rewires its boundary; it now normalizes lifted simplices, searches a closed candidate subset, and rebuilds quotient representatives from that selection. This block should describe that flow, and it should point back to the numbered entry in
REFERENCES.mdinstead of relying only on raw CGAL links.As per coding guidelines, "Algorithms must cite their source (Shewchuk, Bowyer–Watson, Edelsbrunner, Preparata–Shamos, etc.) in
REFERENCES.mdand document their conditioning behavior in code documentation".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/triangulation/builder.rs` around lines 1938 - 1950, Update the Phase-2 documentation block in builder.rs to reflect the new quotient-selection implementation: replace the old "extract central sub-complex and rewire boundary" description with steps that say you normalize lifted simplices, search a closed candidate subset of simplices, and rebuild quotient representatives from that selection (and then rebuild incident-simplex associations), and add a cross-reference to the specific numbered entry in REFERENCES.md (instead of only CGAL links); locate the Phase-2 doc block around the existing comments for the algorithm in builder.rs and update its text accordingly so it accurately names the normalization, candidate search, and quotient-rebuild phases.src/geometry/algorithms/convex_hull.rs (1)
223-229: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPreserve the old error variant for one deprecation cycle.
Renaming
ConvexHullConstructionError::AdjacentCellResolutionFailedtoAdjacentSimplexResolutionFailedis a source break for downstreammatches on a public enum. Please keep a deprecated compatibility variant for one release instead of removing it outright.As per coding guidelines:
New functionality must be additive; use crate::prelude::* (or focused preludes like prelude::triangulation, prelude::query) for ergonomic re-exports; never silently rename or remove a public item🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/geometry/algorithms/convex_hull.rs` around lines 223 - 229, Reintroduce the old enum variant to maintain source compatibility: add back ConvexHullConstructionError::AdjacentCellResolutionFailed with the same field signature (#[source] source: TdsError) and mark it #[deprecated(note = "Renamed to AdjacentSimplexResolutionFailed; will be removed in next release")] so downstream match arms keep working; keep the new AdjacentSimplexResolutionFailed variant as-is, ensure both variants use the same error text/structure, and (optionally) add a short doc comment pointing users to the new name.src/geometry/quality.rs (1)
474-513:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse a dimension-consistent degeneracy threshold in
normalized_volume.Line 478 and Line 507 compare a length-scale
epsilonagainstvolumeandavg_edge_length^D. InD >= 2, that makes small but well-shaped simplices fail as “degenerate”, sonormalized_volumestops being scale-invariant despite the docstring. Please compare a dimensionless quantity instead, or scale the tolerance toL^Dbefore these checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/geometry/quality.rs` around lines 474 - 513, The checks in normalized_volume use a length-scale epsilon (from scale_aware_epsilon) to compare against a D-dimensional quantity (volume and edge_length_power), causing dimension mismatch; fix by making comparisons dimensionally consistent — either raise epsilon to the Dth power (epsilon_pow = epsilon^D) before comparing to volume and edge_length_power, or normalize the volume by edge_length_power (volume / edge_length_power) and compare that dimensionless ratio to epsilon. Concretely, in the block using scale_aware_epsilon(&points) and the variables volume, avg_edge_length, and edge_length_power, compute a dimension-consistent threshold (e.g., let epsilon_pow = epsilon repeated-multiply D times or compute normalized = volume / edge_length_power) and then replace the comparisons "if volume < epsilon" and "if edge_length_power < epsilon" with comparisons against epsilon_pow or compare normalized against epsilon; keep the same QualityDegeneracyMeasure variants (Volume, AverageEdgeLength, EdgeLengthPower) and include the same diagnostic fields in the returned QualityError.
🧹 Nitpick comments (2)
src/triangulation/flips.rs (1)
19-19: ⚡ Quick winAdd a one-release deprecated alias for
CellKeyin this public re-export surface.This rename is consistent, but a temporary deprecated alias here would reduce downstream breakage during migration.
♻️ Suggested compatibility shim
pub use crate::tds::{EdgeKey, FacetHandle, SimplexKey, VertexKey}; +#[deprecated(note = "Renamed to `SimplexKey`; use `SimplexKey` instead.")] +pub type CellKey = SimplexKey;As per coding guidelines, “New functionality must be additive; ... never silently rename or remove a public item.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/triangulation/flips.rs` at line 19, Add a one-release deprecated public alias named CellKey that points to the existing SimplexKey in the same re-export surface so downstream code can continue to compile during migration; specifically, next to the existing pub use crate::tds::{EdgeKey, FacetHandle, SimplexKey, VertexKey} add a public re-export that aliases SimplexKey as CellKey and mark it #[deprecated(..., since = "<next-release>")] with a note instructing users to use SimplexKey instead, then remove the alias in the following release.src/core/vertex.rs (1)
353-377: ⚡ Quick winConsider a deprecated
incident_cell()bridge here.This new public accessor lands without a compatibility shim for the old name. A one-release deprecated alias would give downstream crates a migration window instead of forcing a flag-day rename.
As per coding guidelines, "New functionality must be additive; use
crate::prelude::*(or focused preludes likeprelude::triangulation,prelude::query) for ergonomic re-exports; never silently rename or remove a public item."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/vertex.rs` around lines 353 - 377, Add a one-release deprecated bridge named incident_cell that forwards to the new incident_simplex: create a pub const fn incident_cell(&self) -> Option<SimplexKey> that simply returns self.incident_simplex, annotate it with #[deprecated(note = "renamed to `incident_simplex`; will be removed in a future release")] plus the same #[inline] and #[must_use] attributes and a doc comment pointing callers to incident_simplex so downstream crates get a migration window while preserving the existing API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/roadmap.md`:
- Line 66: The roadmap entry incorrectly states a rename `Simplex` → `Simplex`;
update the description to reflect the intended breaking change `Cell` →
`Simplex` and adjust the surrounding sentence so it reads something like "rename
`Cell` → `Simplex` and implement unit-sphere normalization for …"; locate the
line mentioning `Simplex` → `Simplex` and replace it with `Cell` → `Simplex`,
ensuring any references to the old/new type names elsewhere in the same
paragraph are consistent with the PR objective (`#323`).
In `@src/core/algorithms/pl_manifold_repair.rs`:
- Around line 23-25: The doc comment is wrong: pick_worst_simplex() actually
sorts with b.score.total_cmp(&a.score) so higher scores win; update the
module-level comment to state the removal order is by descending quality (i.e.,
higher score first) rather than "ascending quality", and keep the rest of the
tie-breakers (canonicalized vertex tuple, simplex UUID) as-is so the description
matches pick_worst_simplex().
In `@src/core/boundary.rs`:
- Around line 170-172: The current expression using
facet_to_simplices.get(&facet_key).is_some_and(|simplices| simplices.len() == 1)
silently treats multiplicities != 1 as "not boundary"; change it to explicitly
match the entry from facet_to_simplices for facet_key and return Err(...) when
simplices.len() > 1 (a topology error), Ok(true) when len() == 1, and Ok(false)
when None; apply the same change to the other identical occurrence referenced
(the block around the second facet_to_simplices.get(&facet_key) use) so invalid
multiplicities surface as errors instead of being classified as interior.
In `@src/core/facet.rs`:
- Around line 242-246: Add the #[must_use] attribute to the public wrapper/view
types to prevent callers from accidentally ignoring returned handles/views:
annotate the FacetHandle struct and the FacetView type (or struct) with
#[must_use]; locate the declarations for FacetHandle and the public FacetView
(the second occurrence noted around the other wrapper definitions) and prepend
#[must_use] above each public type declaration so the compiler will warn when
results are discarded.
In `@src/core/traits/facet_cache.rs`:
- Around line 26-29: Update the documentation comment to use the current API
name: replace the mistaken reference to build_facet_to_simplices_hashmap() with
the actual method name build_facet_to_simplices_map() in the docs for the
FacetCache trait so readers are pointed to the correct helper.
In `@src/triangulation/builder.rs`:
- Around line 717-718: insert_simplex_with_mapping() and
assign_incident_simplices() are returning raw TriangulationConstructionError
values instead of wrapping them in the advertised explicit-construction wrapper;
change their error returns so any TriangulationConstructionError (duplicate
simplex, facet-sharing, incident-assignment failure, etc.) is converted into the
ExplicitConstruction variant (e.g.
DelaunayTriangulationConstructionError::ExplicitConstruction(...) or the
project’s ExplicitConstructionError wrapper) before being propagated. Locate
those functions and replace direct early returns or ? usages that yield
TriangulationConstructionError with mapped errors (map_err / map or equivalent)
that construct the ExplicitConstruction wrapper so callers matching on
DelaunayTriangulationConstructionError::ExplicitConstruction will observe these
cases.
In `@src/triangulation/delaunayize.rs`:
- Around line 622-635: When config.fallback_rebuild is enabled, take the
snapshot_rebuild_state from dt.as_triangulation().tds before calling
repair_facet_oversharing so the snapshot reflects the pre-repair topology; store
the (vertices, simplex_data) and then call
repair_facet_oversharing(dt.tds_mut_for_repair(), &pl_config). If
repair_facet_oversharing returns Err, use the pre-captured snapshot to drive the
fallback rebuild_preserving_data(&dt.as_triangulation().kernel, &vertices,
&simplex_data) (and return FallbackSimplexDataSnapshotFailed only if the initial
snapshot call itself failed), ensuring both the happy path and the fallback
branch reuse the same saved snapshot instead of attempting snapshotting after
mutation.
In `@src/triangulation/locality.rs`:
- Around line 19-23: Add deprecated shims for the public renames so callers
don't break: for the struct LocalConflictSeedSimplices create a
#[deprecated(note = "renamed to LocalConflictSeedSimplices; will be removed in a
future release")] pub type CellConflictSeedSimplices =
LocalConflictSeedSimplices (and analogous #[deprecated] pub fn aliases for any
renamed functions), and do the same for the other renamed public items in this
module (the later renamed types/functions referenced in the review). Ensure each
shim uses the original public name, points to the new symbol (e.g.,
LocalConflictSeedSimplices), includes a clear deprecation note, and re-export
these shims from the crate prelude (or prelude::triangulation) so downstream
callers get a one-release migration path.
In `@tests/proptest_triangulation.rs`:
- Around line 151-152: Remove the overly strict assumptions that drop valid
single-simplex cases: delete the prop_assume!(simplices_considered > 1) and
prop_assume!(matched_simplices >= 2) lines and replace them with a single
relaxed assumption prop_assume!(matched_simplices >= 1) so only at least one
matched simplex is required (refer to the prop_assume! calls and the variables
simplices_considered and matched_simplices to locate the change).
In `@tests/triangulation_builder.rs`:
- Around line 451-457: Add a precondition that the Delaunay triangulation has at
least one simplex before using Iterator::all so the test isn't vacuously true;
specifically, insert an assertion such as assert!(dt.number_of_simplices() > 0)
(or assert!(dt.simplices().count() > 0)) immediately before the existing check
that calls dt.simplices().all(...), leaving the rest of the
periodic_vertex_offsets()/number_of_vertices() assertion unchanged so the test
actually verifies per-simplex offsets were populated.
---
Outside diff comments:
In `@src/core/collections/key_maps.rs`:
- Around line 138-170: Add deprecated compatibility aliases for the renamed
public types so downstream code has a one-release migration path: add
#[deprecated(note = "renamed to SimplexKeySet; use SimplexKeySet instead")] pub
type CellKeySet = SimplexKeySet; and #[deprecated(note = "renamed to
UuidToSimplexKeyMap; use UuidToSimplexKeyMap instead")] pub type
UuidToCellKeyMap = UuidToSimplexKeyMap; place these aliases adjacent to the
existing SimplexKeySet and UuidToSimplexKeyMap typedefs and ensure they are
public so consumers receive deprecation warnings but their code continues to
compile.
In `@src/core/util/delaunay_validation.rs`:
- Around line 740-759: The loop re-runs robust_insphere and can disagree with
the validator; instead call first_offending_vertex() (the same helper used by
first_delaunay_violation_witness()) to get the offending VertexKey for the given
simplex and tds, then only look up the Point for logging (use the returned
VertexKey to fetch the point from tds.vertices()). Replace the manual
robust_insphere iteration in this block with a call to first_offending_vertex()
and set offending to Some((vkey, *point)) only after fetching the point for
logging; preserve the existing warning path but avoid re-evaluating
robust_insphere here.
In `@src/geometry/algorithms/convex_hull.rs`:
- Around line 223-229: Reintroduce the old enum variant to maintain source
compatibility: add back
ConvexHullConstructionError::AdjacentCellResolutionFailed with the same field
signature (#[source] source: TdsError) and mark it #[deprecated(note = "Renamed
to AdjacentSimplexResolutionFailed; will be removed in next release")] so
downstream match arms keep working; keep the new AdjacentSimplexResolutionFailed
variant as-is, ensure both variants use the same error text/structure, and
(optionally) add a short doc comment pointing users to the new name.
In `@src/geometry/quality.rs`:
- Around line 474-513: The checks in normalized_volume use a length-scale
epsilon (from scale_aware_epsilon) to compare against a D-dimensional quantity
(volume and edge_length_power), causing dimension mismatch; fix by making
comparisons dimensionally consistent — either raise epsilon to the Dth power
(epsilon_pow = epsilon^D) before comparing to volume and edge_length_power, or
normalize the volume by edge_length_power (volume / edge_length_power) and
compare that dimensionless ratio to epsilon. Concretely, in the block using
scale_aware_epsilon(&points) and the variables volume, avg_edge_length, and
edge_length_power, compute a dimension-consistent threshold (e.g., let
epsilon_pow = epsilon repeated-multiply D times or compute normalized = volume /
edge_length_power) and then replace the comparisons "if volume < epsilon" and
"if edge_length_power < epsilon" with comparisons against epsilon_pow or compare
normalized against epsilon; keep the same QualityDegeneracyMeasure variants
(Volume, AverageEdgeLength, EdgeLengthPower) and include the same diagnostic
fields in the returned QualityError.
In `@src/triangulation/builder.rs`:
- Around line 1938-1950: Update the Phase-2 documentation block in builder.rs to
reflect the new quotient-selection implementation: replace the old "extract
central sub-complex and rewire boundary" description with steps that say you
normalize lifted simplices, search a closed candidate subset of simplices, and
rebuild quotient representatives from that selection (and then rebuild
incident-simplex associations), and add a cross-reference to the specific
numbered entry in REFERENCES.md (instead of only CGAL links); locate the Phase-2
doc block around the existing comments for the algorithm in builder.rs and
update its text accordingly so it accurately names the normalization, candidate
search, and quotient-rebuild phases.
---
Nitpick comments:
In `@src/core/vertex.rs`:
- Around line 353-377: Add a one-release deprecated bridge named incident_cell
that forwards to the new incident_simplex: create a pub const fn
incident_cell(&self) -> Option<SimplexKey> that simply returns
self.incident_simplex, annotate it with #[deprecated(note = "renamed to
`incident_simplex`; will be removed in a future release")] plus the same
#[inline] and #[must_use] attributes and a doc comment pointing callers to
incident_simplex so downstream crates get a migration window while preserving
the existing API surface.
In `@src/triangulation/flips.rs`:
- Line 19: Add a one-release deprecated public alias named CellKey that points
to the existing SimplexKey in the same re-export surface so downstream code can
continue to compile during migration; specifically, next to the existing pub use
crate::tds::{EdgeKey, FacetHandle, SimplexKey, VertexKey} add a public re-export
that aliases SimplexKey as CellKey and mark it #[deprecated(..., since =
"<next-release>")] with a note instructing users to use SimplexKey instead, then
remove the alias in the following release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f2c9afde-c494-49d8-9242-3ace2b3e07f6
📒 Files selected for processing (114)
.github/workflows/codacy.ymlAGENTS.mdCONTRIBUTING.mdCargo.tomlREADME.mdREFERENCES.mdbenches/README.mdbenches/ci_performance_suite.rsbenches/profiling_suite.rsbenches/tds_clone.rsdocs/ORIENTATION_SPEC.mddocs/api_design.mddocs/code_organization.mddocs/dev/debug_env_vars.mddocs/dev/rust.mddocs/dev/tooling-alignment.mddocs/diagnostics.mddocs/invariants.mddocs/numerical_robustness_guide.mddocs/production_review_remediation_checklist.mddocs/property_testing_summary.mddocs/roadmap.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/convex_hull_3d_1000_points.rsexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/memory_analysis.rsexamples/numerical_robustness.rsexamples/pachner_roundtrip_4d.rsexamples/topology_editing_2d_3d.rsexamples/triangulation_3d_1000_points.rsexamples/zero_allocation_iterator_demo.rsscripts/ci/filter_codacy_sarif.pyscripts/tests/test_filter_codacy_sarif.pysrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/collections/aliases.rssrc/core/collections/buffers.rssrc/core/collections/helpers.rssrc/core/collections/key_maps.rssrc/core/collections/secondary_maps.rssrc/core/collections/triangulation_maps.rssrc/core/edge.rssrc/core/facet.rssrc/core/operations.rssrc/core/simplex.rssrc/core/tds.rssrc/core/traits/boundary_analysis.rssrc/core/traits/data_type.rssrc/core/traits/facet_cache.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/jaccard.rssrc/core/vertex.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/predicates.rssrc/geometry/quality.rssrc/geometry/robust_predicates.rssrc/geometry/traits/coordinate.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rssrc/topology/traits/global_topology_model.rssrc/topology/traits/topological_space.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rssrc/triangulation/diagnostics.rssrc/triangulation/flips.rssrc/triangulation/locality.rstests/README.mdtests/allocation_api.rstests/check_perturbation_stats.rstests/circumsphere_debug_tools.rstests/conflict_region_verification.rstests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/delaunay_public_api_coverage.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/insert_with_statistics.rstests/k3_cycle_predicate.rstests/large_scale_debug.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.proptest-regressionstests/proptest_delaunay_triangulation.rstests/proptest_euler_characteristic.rstests/proptest_facet.rstests/proptest_flips.rstests/proptest_orientation.rstests/proptest_serialization.rstests/proptest_simplex.rstests/proptest_tds.rstests/proptest_triangulation.rstests/public_topology_api.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rstests/tds_orientation.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (6)
- tests/check_perturbation_stats.rs
- Cargo.toml
- tests/tds_orientation.rs
- tests/conflict_region_verification.rs
- tests/k3_cycle_predicate.rs
- tests/delaunay_public_api_coverage.rs
- Report invalid boundary facet multiplicities as topology errors instead of classifying them as interior facets. - Preserve typed explicit-construction failures for simplex creation and TDS assembly so callers can match the documented error path. - Use pre-repair Delaunayize snapshots for fallback rebuilds and make normalized-volume degeneracy checks dimensionally consistent. - Move benchmark-like examples into tests and benchmarks, replacing the large 3D examples with focused topology-editing and triangulation/hull workflows. BREAKING CHANGE: ExplicitConstructionError::SimplexCreation now exposes a SimplexValidationError source, and example target names were consolidated to topology_editing and triangulation_and_hull.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benches/ci_performance_suite.rs`:
- Line 1080: The benchmark uses outside_point = Point::new([10.0; D]) which can
lie inside the generated hull, causing
find_visible_facets/find_nearest_visible_facet to exercise the inside-point
path; replace the hardcoded [10.0; D] with a guaranteed exterior point (e.g.,
scale coordinates beyond the input range or compute a point offset from the hull
centroid by a large radius) so outside_point is always outside the hull used by
the benchmarks, and update any related comments/tests referencing outside_point
to reflect the new construction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8db139b0-c748-4a8f-8921-9e0ff56121c1
📒 Files selected for processing (30)
README.mdbenches/ci_performance_suite.rsdocs/api_design.mddocs/code_organization.mddocs/roadmap.mdexamples/README.mdexamples/convex_hull_3d_1000_points.rsexamples/memory_analysis.rsexamples/pachner_roundtrip_4d.rsexamples/point_comparison_and_hashing.rsexamples/topology_editing.rsexamples/triangulation_3d_1000_points.rsexamples/triangulation_and_hull.rsexamples/zero_allocation_iterator_demo.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/facet.rssrc/core/traits/boundary_analysis.rssrc/core/traits/facet_cache.rssrc/core/util/delaunay_validation.rssrc/geometry/quality.rssrc/lib.rssrc/triangulation/builder.rssrc/triangulation/delaunayize.rstests/allocation_api.rstests/example_workflows.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_triangulation.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (5)
- examples/triangulation_3d_1000_points.rs
- examples/convex_hull_3d_1000_points.rs
- examples/memory_analysis.rs
- examples/zero_allocation_iterator_demo.rs
- examples/pachner_roundtrip_4d.rs
✅ Files skipped from review due to trivial changes (3)
- docs/roadmap.md
- docs/code_organization.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/allocation_api.rs
- docs/api_design.md
- src/core/algorithms/pl_manifold_repair.rs
- src/triangulation/delaunayize.rs
- src/core/traits/facet_cache.rs
- src/core/util/delaunay_validation.rs
- src/core/boundary.rs
- src/lib.rs
- src/geometry/quality.rs
- src/triangulation/builder.rs
- Derive convex hull query points from benchmark triangulation bounds instead of fixed coordinates. - Preflight the computed point with ConvexHull::is_point_outside so visibility benchmarks measure the exterior path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: Cell-named public APIs are renamed to their simplex equivalents, including Cell/CellKey, cell iterators and counts, explicit cell construction, cell-oriented error variants, and cell-named topology maps.
Resolves #323