perf: switch FastKernel to insphere_lifted and enable LTO#234
Merged
acgetchell merged 4 commits intomainfrom Mar 8, 2026
Merged
perf: switch FastKernel to insphere_lifted and enable LTO#234acgetchell merged 4 commits intomainfrom
acgetchell merged 4 commits intomainfrom
Conversation
Switch FastKernel::in_sphere() to use insphere_lifted() for 5.3x speedup in 3D. Add release profile optimization with thin LTO and codegen-units=1. Benchmarks across dimensions (2D-5D): - insphere_lifted is 5.3x faster in 3D (15.5 ns vs 81.7 ns) - Random query test: 3.75x faster (20.0 µs vs 75.0 µs for 1000 queries) - insphere_lifted consistently fastest across all dimensions Performance gains attributed to la-stack v0.2.0's closed-form determinants for D=1-4. Changes: - Cargo.toml: Add [profile.release] with lto="thin" and codegen-units=1 - src/geometry/kernel.rs: Switch FastKernel to use insphere_lifted() Expected impact: 5-6x improvement in incremental insertion hot path. All tests pass (1279 unit tests, 505 doctests, full CI green). Resolves: #206 Co-Authored-By: Oz <oz-agent@warp.dev>
WalkthroughSwitches FastKernel to call Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
rust
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/geometry/kernel.rs`:
- Around line 250-257: The current map_err on insphere_lifted in
Kernel::in_sphere stringifies every error into
CoordinateConversionError::ConversionFailed, losing original
CellValidationError::CoordinateConversion metadata; change the error mapping to
match the insphere_lifted error and if it is
CellValidationError::CoordinateConversion { source, .. } forward that source
unchanged (propagate the original conversion error), otherwise synthesize
CoordinateConversionError::ConversionFailed (preserving
coordinate_index/coordinate_value/from_type/to_type as before) so only
non-conversion variants are wrapped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e2cd9915-4225-4a8e-a606-6829db2a5a9d
📒 Files selected for processing (2)
Cargo.tomlsrc/geometry/kernel.rs
Update insphere predicate documentation with comprehensive benchmark results and improve error propagation in FastKernel::in_sphere. Documentation updates: - Add performance sections to insphere(), insphere_distance(), and insphere_lifted() - Document benchmark results showing insphere_lifted is 5.3x faster in 3D - Mark insphere_lifted as the recommended high-performance implementation - Explain performance gains from relative coordinates and la-stack v0.2.0's closed-form determinants for D=1-4 Error handling improvement: - Fix FastKernel::in_sphere to preserve CoordinateConversionError metadata - When insphere_lifted returns CellValidationError::CoordinateConversion, forward the original source error instead of stringifying it - Only wrap non-conversion errors in new CoordinateConversionError
coderabbitai
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/geometry/predicates.rs (1)
669-683: Add a regression that exercises this parity branch under both simplex orientations.This is the behavioral change in the PR, but the current insphere tests still cover only one orientation per dimension, and the 2D comprehensive test still leaves
std vs lifteddisabled. A badparity_sign * orient_signcombination for reversed vertex order would still slip through here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/geometry/predicates.rs` around lines 669 - 683, Add tests that exercise the parity branch for both Orientation::POSITIVE and Orientation::NEGATIVE so the parity_sign * orient_sign combination is validated; specifically add insphere/regression cases that call the code path computing dimension_is_even, parity_sign, orient_sign and det_norm for even and odd D with both vertex orderings. Update the existing 2D comprehensive test to enable the "std vs lifted" comparison (it is currently disabled) and ensure each test asserts identical results between the lifted formulation and the standard predicate for both orientations. Ensure test names reference the predicate (insphere) and orientation so failures point to the parity branch.
🤖 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/geometry/predicates.rs`:
- Around line 185-193: The docs recommending insphere_lifted should include a
robustness caveat: update the docblocks that currently endorse insphere_lifted
(the Performance/recommendation paragraphs in predicates.rs that compare
insphere, insphere_distance and insphere_lifted) to add a short sentence noting
that insphere_lifted is a fast floating-point predicate and may be less robust
than robust_insphere, and that callers requiring numerical robustness should use
robust_insphere or RobustKernel (follow the same wording/pattern used in the
kernel.rs FastKernel caveat for 3D+ triangulation); make this change in each
recommendation block where insphere_lifted is promoted so users are warned about
correctness vs performance tradeoffs.
---
Nitpick comments:
In `@src/geometry/predicates.rs`:
- Around line 669-683: Add tests that exercise the parity branch for both
Orientation::POSITIVE and Orientation::NEGATIVE so the parity_sign * orient_sign
combination is validated; specifically add insphere/regression cases that call
the code path computing dimension_is_even, parity_sign, orient_sign and det_norm
for even and odd D with both vertex orderings. Update the existing 2D
comprehensive test to enable the "std vs lifted" comparison (it is currently
disabled) and ensure each test asserts identical results between the lifted
formulation and the standard predicate for both orientations. Ensure test names
reference the predicate (insphere) and orientation so failures point to the
parity branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0af3d387-32e8-4194-ba28-d38426944c31
📒 Files selected for processing (2)
src/geometry/kernel.rssrc/geometry/predicates.rs
Add robustness warnings to insphere_lifted documentation noting it's a fast floating-point predicate that may be less robust than robust_insphere for nearly-degenerate configurations. Direct users to RobustKernel for 3D+ triangulations requiring numerical robustness. Documentation updates: - Add robustness caveat to insphere_distance() recommendation (lines 192-196) - Add robustness caveat to insphere() performance section (lines 297-302) - Add new "Robustness" section to insphere_lifted() docs (lines 500-505) - All warnings reference RobustKernel and follow FastKernel warning pattern Testing improvements: - Add test_insphere_lifted_parity_branch_positive_orientation Tests parity_sign * orient_sign path for 2D, 3D, 4D, 5D with POSITIVE orientation - Add test_insphere_lifted_parity_branch_negative_orientation Tests parity_sign * orient_sign path for 2D, 3D, 4D, 5D with NEGATIVE orientation - Each test validates orientation and asserts insphere_lifted matches insphere - Ensures correctness for even dimensions (2D, 4D) and odd dimensions (3D, 5D) All 20 predicate tests pass. Documentation builds without warnings. Addresses inline code review comments for issue #206.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/geometry/predicates.rs (2)
1614-1720: Consider table-driving the parity fixtures.The two tests repeat nearly identical setup for 2D–5D and both orientations. A small helper over
(simplex, expected_orientation, point)would make this regression much easier to extend and less error-prone.Also applies to: 1723-1828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/geometry/predicates.rs` around lines 1614 - 1720, The test test_insphere_lifted_parity_branch_positive_orientation duplicates nearly identical setups across dimensions and orientations; refactor by creating a small table-driven helper (e.g., a function like run_insphere_parity_test or a struct fixture holding (simplex, expected_orientation, test_point)) that iterates over rows for 2D–5D and both Orientation::POSITIVE/NEGATIVE, and inside each iteration call simplex_orientation, insphere_lifted and insphere and assert equality and expected InSphere; replace the repeated blocks in test_insphere_lifted_parity_branch_positive_orientation (and the similar test at 1723-1828) with calls into this helper to reduce duplication and make it easier to extend.
1614-1720: Add oneFastKernel::in_sphere()regression alongside these parity tests.These assertions prove the predicate-level parity behavior, but the PR’s actual hot-path change is the enum-to-
i32mapping insrc/geometry/kernel.rs:244-267. One positive/negative-orientation regression there would protect the production path from a future sign-mapping slip.Also applies to: 1723-1828
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/geometry/predicates.rs`:
- Around line 1614-1720: The test
test_insphere_lifted_parity_branch_positive_orientation duplicates nearly
identical setups across dimensions and orientations; refactor by creating a
small table-driven helper (e.g., a function like run_insphere_parity_test or a
struct fixture holding (simplex, expected_orientation, test_point)) that
iterates over rows for 2D–5D and both Orientation::POSITIVE/NEGATIVE, and inside
each iteration call simplex_orientation, insphere_lifted and insphere and assert
equality and expected InSphere; replace the repeated blocks in
test_insphere_lifted_parity_branch_positive_orientation (and the similar test at
1723-1828) with calls into this helper to reduce duplication and make it easier
to extend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: aa83b403-b8e4-4da3-810a-2b03ee3db0bc
📒 Files selected for processing (1)
src/geometry/predicates.rs
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
==========================================
+ Coverage 61.85% 61.87% +0.02%
==========================================
Files 49 49
Lines 13238 13245 +7
==========================================
+ Hits 8188 8196 +8
+ Misses 5050 5049 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Introduce a `check_insphere_parity` helper function to deduplicate test logic for parity sign checks across 2D-5D. Add coverage for the `InsufficientVertices` and `DegenerateSimplex` error paths in the `FastKernel::in_sphere` implementation. This is an internal change. Refs: #206
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/geometry/predicates.rs`:
- Around line 187-193: The current doc comment overstates performance by saying
insphere_lifted is “significantly faster across all dimensions”; change the
phrasing to soften the claim—e.g., say insphere_lifted is the “fastest overall”
or “preferred default for performance” and explicitly note that in 2D
insphere_lifted is roughly on par with insphere (and insphere_distance is slower
in 2D). Update the three affected doc comment blocks that mention
insphere_lifted/insphere/insphere_distance (the block around the diff shown and
the other occurrences at the ranges you noted) to use the softened wording and
the explicit 2D parity note, keeping the benchmark bullets intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ffbe0e4a-7174-4a10-b38b-4bdf7fd171b9
📒 Files selected for processing (2)
src/geometry/kernel.rssrc/geometry/predicates.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/geometry/kernel.rs
Comment on lines
+187
to
+193
| /// Benchmarks show that [`insphere_lifted`] is significantly faster across all dimensions: | ||
| /// - **3D**: 5.3x faster than [`insphere`], 2.5x faster than `insphere_distance` | ||
| /// - **4D-5D**: 1.6-2.9x faster than [`insphere`], comparable to `insphere_distance` | ||
| /// - **2D**: `insphere_distance` is 2x slower than [`insphere`] or [`insphere_lifted`] | ||
| /// | ||
| /// **Recommendation**: Use [`insphere_lifted`] for optimal performance in production code. | ||
| /// Note that `insphere_lifted` is a fast floating-point predicate that may be less robust |
There was a problem hiding this comment.
Soften the “across all dimensions” performance claim.
The bullets here already say 2D is roughly on par with insphere, so phrases like “significantly faster across all dimensions” / “superior performance across all dimensions” overstate the measurements. I’d reword this as “fastest overall” or “preferred default,” with an explicit note that 2D is near parity.
Also applies to: 293-298, 494-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/geometry/predicates.rs` around lines 187 - 193, The current doc comment
overstates performance by saying insphere_lifted is “significantly faster across
all dimensions”; change the phrasing to soften the claim—e.g., say
insphere_lifted is the “fastest overall” or “preferred default for performance”
and explicitly note that in 2D insphere_lifted is roughly on par with insphere
(and insphere_distance is slower in 2D). Update the three affected doc comment
blocks that mention insphere_lifted/insphere/insphere_distance (the block around
the diff shown and the other occurrences at the ranges you noted) to use the
softened wording and the explicit 2D parity note, keeping the benchmark bullets
intact.
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.
Switch FastKernel::in_sphere() to use insphere_lifted() for 5.3x speedup in 3D. Add release profile optimization with thin LTO and codegen-units=1.
Benchmarks across dimensions (2D-5D):
Performance gains attributed to la-stack v0.2.0's closed-form determinants for D=1-4.
Changes:
Expected impact: 5-6x improvement in incremental insertion hot path.
All tests pass (1279 unit tests, 505 doctests, full CI green).
Resolves: #206