Merged
Conversation
Bump MSRV from 1.94 to 1.95 and adopt stabilized features where they fit. Coordinates with la-stack 0.4.0 -> 0.4.1, which also requires 1.95. Toolchain and docs: - Cargo.toml (rust-version), rust-toolchain.toml (channel) and clippy.toml (msrv) all set to 1.95. - AGENTS.md and CONTRIBUTING.md MSRV references refreshed; AGENTS.md Design Principles section expanded. - CITATION.cff caught up to 0.7.5. - la-stack bumped 0.4.0 -> 0.4.1 in Cargo.toml / Cargo.lock. core::hint::cold_path adopted on the cold arms of the predicate pipeline, matching the existing Stage 1 / Stage 2 / Stage 3 layout: - src/geometry/predicates.rs: insphere_from_matrix and orientation_from_matrix -- Stage 2 exact-sign entry and Stage 3 non-finite fallback. - src/geometry/robust_predicates.rs: robust_insphere Strategy 3 (distance + SoS) and the DEGENERATE early-return in adaptive_tolerance_insphere. - src/geometry/util/circumsphere.rs: LU near-singular -> solve_exact_f64 fallback in circumcenter. if-let guards on match arms where they flatten nested patterns: - src/core/algorithms/flips.rs: repair-error match arms, plus shared log_apply_skip / log_build_skip helpers for the k=2 and k=3 skip paths. - src/triangulation/delaunayize.rs: topology repair fallback. benches/cold_path_predicates.rs: new criterion suite exercising the Stage 1 hot path and a near-boundary group that spills into Stage 2 across D = 2..=5; registered in Cargo.toml. Minor tidies enabled by the bump: - benches/large_scale_performance.rs: Duration::from_secs(120) -> Duration::from_mins(2). - benches/ci_performance_suite.rs: env-flag parsing uses Result::is_ok_and. - examples/convex_hull_3d_100_points.rs: Option::map + unwrap_or -> Option::map_or (clippy nursery). assert_matches! / debug_assert_matches! adoption is deferred to #329: rust-lang/rust#137487 was tagged milestone 1.95.0 but did not ship stable in rustc 1.95.0 (the macro still carries #[unstable(feature = "assert_matches", ...)]). select_unpredictable for the Stage 1 fast filter was benchmarked against cold_path_predicates and circumsphere_containment on ARM64; reverted because it regressed 13 cases (up to +16%) against a single 14.89% win. Numbers posted on #324. just ci passes. Closes #324 Co-Authored-By: Oz <oz-agent@warp.dev>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 24 |
🟢 Coverage 33.83% diff coverage · +0.12% coverage variation
Metric Results Coverage variation ✅ +0.12% coverage variation (-1.00%) Diff coverage ✅ 33.83% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (7de0dc1) 13956 8688 62.25% Head commit (260e176) 13981 (+25) 8720 (+32) 62.37% (+0.12%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#330) 133 45 33.83% 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. Give us feedback
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
WalkthroughRepository MSRV/toolchain bumped to Rust 1.95; cold-path optimizer hints added to rare predicate/error branches; flips repair diagnostics made lazy with centralized skip tracing; Delaunayize errors refactored to typed struct-like variants including *WithRebuild variants; new predicate cold-path benchmark and tests added; docs and metadata updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/triangulation/delaunayize.rs`:
- Around line 282-305: The fallback-rebuild failure currently stringifies the
original topology error (topo_err) into a new TdsError, losing its typed variant
and source chain; change the Err(rebuild_err) arm inside the match on
DelaunayTriangulation::with_kernel (the block that currently returns
DelaunayizeError::TopologyRepairFailed(PlManifoldRepairError::Tds(crate::core::tds::TdsError::InconsistentDataStructure{
message: format!(...)}))) to preserve topo_err as the primary source (return
DelaunayizeError::TopologyRepairFailed(topo_err) or a new TopologyRepairFailed
variant that wraps topo_err) and attach rebuild_err as contextual information
(e.g., add a sibling field or variant for rebuild_context or use an
error::Context-style wrapper) so consumers can still match on
PlManifoldRepairError and traverse source() while the rebuild failure is
included as additional context.
🪄 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: fb7a9576-20fe-4bdd-9057-521c353bfb9b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
AGENTS.mdCITATION.cffCONTRIBUTING.mdCargo.tomlbenches/ci_performance_suite.rsbenches/cold_path_predicates.rsbenches/large_scale_performance.rsclippy.tomlexamples/convex_hull_3d_100_points.rsrust-toolchain.tomlsrc/core/algorithms/flips.rssrc/geometry/predicates.rssrc/geometry/robust_predicates.rssrc/geometry/util/circumsphere.rssrc/triangulation/delaunayize.rs
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (33.83%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
==========================================
+ Coverage 62.26% 62.38% +0.11%
==========================================
Files 53 53
Lines 13949 13974 +25
==========================================
+ Hits 8686 8718 +32
+ Misses 5263 5256 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Refactor DelaunayizeError and related types to use struct variants with named fields, ensuring primary and fallback errors are preserved as typed values instead of being stringified. This change improves diagnostic traversability via Error::source and pattern matching. Additionally, add navigation tables of contents to all developer documentation and AGENTS.md, and expand the Rust style guide with explicit conventions for orthogonal and composable error taxonomies. Refs: #324
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
AGENTS.md (1)
357-357: Avoid hard-coding the proptest file count in policy docs.“(17 files)” will drift as tests are added/removed. Prefer a non-numeric phrasing to keep AGENTS.md stable.
✏️ Suggested wording tweak
-- Proptests under `tests/proptest_*.rs` (17 files) cover algebraic and +- Proptests under `tests/proptest_*.rs` cover algebraic and🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 357, The line "Proptests under `tests/proptest_*.rs` (17 files) cover..." hard-codes a test count that will drift; remove the numeric "(17 files)" and replace it with a non-numeric phrase such as "multiple files", "several files", or "a set of files" so the sentence reads like "Proptests under `tests/proptest_*.rs` cover algebraic and..." to keep AGENTS.md stable; update the exact text surrounding the symbol `tests/proptest_*.rs` accordingly.src/core/algorithms/flips.rs (1)
3255-3275: Avoid eagerly formatting skip diagnostic samples.The new split skip arms call
format!(...)unconditionally, but eachrecord_*_skipmethod stores only the first sample. In repeated repair skips, this allocates strings that are immediately discarded; making the sample lazy keeps the counters while avoiding unnecessary allocation.♻️ Proposed lazy-sample shape
- fn record_inserted_simplex_skip(&mut self, sample: String) { + fn record_inserted_simplex_skip(&mut self, sample: impl FnOnce() -> String) { self.inserted_simplex_skips = self.inserted_simplex_skips.saturating_add(1); if self.inserted_simplex_sample.is_none() { - self.inserted_simplex_sample = Some(sample); + self.inserted_simplex_sample = Some(sample()); } } - fn record_missing_cell_skip(&mut self, sample: String) { + fn record_missing_cell_skip(&mut self, sample: impl FnOnce() -> String) { self.missing_cell_skips = self.missing_cell_skips.saturating_add(1); if self.missing_cell_sample.is_none() { - self.missing_cell_sample = Some(sample); + self.missing_cell_sample = Some(sample()); } }Example call-site update:
- diagnostics.record_inserted_simplex_skip(format!( + diagnostics.record_inserted_simplex_skip(|| format!( "facet={facet:?} removed_face={:?} inserted_face={:?}", context.removed_face_vertices, context.inserted_face_vertices ));Based on learnings, "Applies to **/*.rs : Avoid introducing allocations unless necessary in Rust code."
Also applies to: 3757-3773, 3836-3856, 3922-3942, 4017-4037, 4103-4126, 4190-4210, 4280-4301, 4358-4388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/algorithms/flips.rs` around lines 3255 - 3275, The skip-recording helpers eagerly allocate because callers pass format!() results; change each method (record_inserted_simplex_skip, record_invalid_ridge_multiplicity_skip, record_missing_cell_skip and the other similar record_*_skip methods listed) to accept a lazy producer (e.g. sample: impl FnOnce() -> String or a generic F: FnOnce() -> String) instead of String, increment the saturating counter unconditionally, and only invoke sample() when the corresponding *_sample is None to set it; update all call sites that currently pass format!(...) to pass a closure (|| format!(...)) so formatting only happens when the sample will actually be stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev/rust.md`:
- Around line 416-418: The documentation incorrectly says Box<T> / Arc<T>
require T: PartialEq to derive Eq; update the Eq derivation guidance so that
types like Box<T> and Arc<T> are stated to require T: Eq (not T: PartialEq) for
Eq to be derivable, and ensure the sentence listing `PartialEq, Eq` and examples
(integers, strings, UUIDs, keys, other PartialEq enums, Arc<T> / Box<T> where T:
PartialEq) is changed to reference `Arc<T> / Box<T> where T: Eq` and any similar
occurrences are corrected.
In `@src/triangulation/delaunayize.rs`:
- Around line 172-175: Update the documentation for the `*WithRebuild` error
variants to clarify that `rebuild_error` is not returned by `Error::source`:
where the text currently says consumers can "traverse the full diagnostic chain
via pattern matching or [`Error::source`]", change it to state consumers can
"inspect both errors via pattern matching" and that the primary repair error is
exposed via [`Error::source`]; likewise replace the phrase "consumers can walk
the full diagnostic chain" with "consumers can inspect both typed errors;
`Error::source` exposes the primary repair error." Reference the `*WithRebuild`
variants, the `rebuild_error` field, and `Error::source` when making these two
phrasing changes.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 357: The line "Proptests under `tests/proptest_*.rs` (17 files) cover..."
hard-codes a test count that will drift; remove the numeric "(17 files)" and
replace it with a non-numeric phrase such as "multiple files", "several files",
or "a set of files" so the sentence reads like "Proptests under
`tests/proptest_*.rs` cover algebraic and..." to keep AGENTS.md stable; update
the exact text surrounding the symbol `tests/proptest_*.rs` accordingly.
In `@src/core/algorithms/flips.rs`:
- Around line 3255-3275: The skip-recording helpers eagerly allocate because
callers pass format!() results; change each method
(record_inserted_simplex_skip, record_invalid_ridge_multiplicity_skip,
record_missing_cell_skip and the other similar record_*_skip methods listed) to
accept a lazy producer (e.g. sample: impl FnOnce() -> String or a generic F:
FnOnce() -> String) instead of String, increment the saturating counter
unconditionally, and only invoke sample() when the corresponding *_sample is
None to set it; update all call sites that currently pass format!(...) to pass a
closure (|| format!(...)) so formatting only happens when the sample will
actually be stored.
🪄 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: 0887aafd-2dbc-499c-a117-06309188b5b2
📒 Files selected for processing (9)
AGENTS.mddocs/dev/commands.mddocs/dev/debug_env_vars.mddocs/dev/rust.mddocs/dev/testing.mdsrc/core/algorithms/flips.rssrc/geometry/util/circumsphere.rssrc/triangulation/delaunayize.rstests/delaunayize_workflow.rs
✅ Files skipped from review due to trivial changes (3)
- docs/dev/commands.md
- docs/dev/debug_env_vars.md
- docs/dev/testing.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/geometry/util/circumsphere.rs
- Switch RepairDiagnostics skip-sample helpers to accept deferred format closures so repeated skip paths do not allocate unused diagnostic strings. - Add a regression test covering the lazy-sample contract for inserted-simplex, ridge-multiplicity, and missing-cell skip recording. - Clarify DelaunayizeError and rust development docs so Error::source() is documented as exposing the primary repair error, while rebuild_error remains available via pattern matching. - Remove the stale proptest file-count wording from AGENTS.md.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev/rust.md`:
- Line 19: The TOC link target '#structwithnamedfields-throughout' likely
doesn't match the generated heading slug for "Struct‑with‑named‑fields
throughout"; update the link or the heading so they match exactly (e.g., change
the TOC entry to use the actual slug produced by the markdown generator or
normalize the heading to match '#structwithnamedfields-throughout'), ensuring
the anchor in the table of contents and the heading text
("Struct‑with‑named‑fields throughout") are identical so in-page navigation
works.
🪄 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: 959d0a15-24de-422e-bf5e-80fbb5b8d714
📒 Files selected for processing (4)
AGENTS.mddocs/dev/rust.mdsrc/core/algorithms/flips.rssrc/triangulation/delaunayize.rs
Verify that `delaunayize_by_flips` correctly handles explicit flip budgets through the advanced repair path. Added regression tests for `FlipError` and `DelaunayRepairError` equality, and ensured default configuration coverage for the new flip budget limit.
Owner
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
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.
Bump MSRV from 1.94 to 1.95 and adopt stabilized features where they fit. Coordinates with la-stack 0.4.0 -> 0.4.1, which also requires 1.95.
Toolchain and docs:
core::hint::cold_path adopted on the cold arms of the predicate pipeline, matching the existing Stage 1 / Stage 2 / Stage 3 layout:
solve_exact_f64 fallback in circumcenter.
if-let guards on match arms where they flatten nested patterns:
benches/cold_path_predicates.rs: new criterion suite exercising the Stage 1 hot path and a near-boundary group that spills into Stage 2 across D = 2..=5; registered in Cargo.toml.
Minor tidies enabled by the bump:
assert_matches! / debug_assert_matches! adoption is deferred to #329: rust-lang/rust#137487 was tagged milestone 1.95.0 but did not ship stable in rustc 1.95.0 (the macro still carries #[unstable(feature = "assert_matches", ...)]).
select_unpredictable for the Stage 1 fast filter was benchmarked against cold_path_predicates and circumsphere_containment on ARM64; reverted because it regressed 13 cases (up to +16%) against a single 14.89% win. Numbers posted on #324.
just ci passes.
Closes #324