Implement edge triangulation in rust, setup repository for build packages #1
Conversation
|
Passing tests: https://github.com/Czaki/bermuda/actions/runs/12420895098 |
|
I have validated a few things and it looks impossible to add type annotation to compiled extensions, But dostrings are available. Added |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive setup for a Rust-Python package called "bermuda", focusing on path triangulation functionality. The changes include establishing a Rust workspace with multiple crates ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Python
participant Rust
participant NumPy
User->>Python: Call triangulate_path_edge()
Python->>Rust: Invoke triangulation function
Rust->>Rust: Process path points
Rust->>NumPy: Generate triangulation arrays
Rust-->>Python: Return triangulation result
Python-->>User: Return triangles, centers, offsets
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (17)
crates/bermuda/src/lib.rs (3)
1-3: Consider minimal imports for clarity.
While importing multiple items from the "numpy" crate is valid, consider explicitly importing only what is used to keep dependencies concise, especially if the codebase grows.
52-63: Optimize memory usage by constructing arrays in one pass.
Instead of creating a two-dimensionalVec<Vec<u32>>and then converting to aPyArray2, consider building the final 2D array in a single pass to reduce intermediate allocations.
75-79: Use a more descriptive module name.
The module name "_bermuda" might be confusing if future modules are added. Consider a name that better describes this module’s purpose, such as "_triangulation" or "_geometry".crates/triangulation/src/point.rs (2)
34-40: Prefer trait-based vector addition for uniform usage.
You're providing a dedicated method (add_vector) for adding aVectorto aPoint. However, you also implement the operator-based approach with std::ops::Add. Consistency might improve clarity; consider using operator overloading exclusively or referencing the ops-based approach internally.
172-236: Extend boundary checks in segment logic.
The code checks for valid segments and properly handles vertical/horizontal logic. However, it might be beneficial to handle boundary cases (e.g., near-equal floating-point coordinates or extremely large values) or to provide tolerance for floating-point comparison.crates/triangulation/src/path_triangulation.rs (2)
27-37: Ensure consistent winding order across multiple passes.
The "fix_triangle_orientation" function corrects winding order by swapping indices if the signed area is negative. For improved robustness, explicitly document or test more complex polygons to ensure consistent winding across multiple consecutive operations.
144-155: Clarify special handling for paths with fewer than two points.
Returning a set of dummy triangles when path length < 2 might be surprising to users. Add a short docstring or a clear error to ensure it's understood that minimal geometry is provided as a fallback, or consider returning an empty triangulation as an alternative.python/bermuda/_bermuda.pyi (1)
3-5: Add docstring alignment with Rust.
The function signature includes parameters like path, closed, limit, and bevel. For discoverability, consider adding short docstring hints in the stub file, matching the docstring structure in the Rust implementation so that linters and IDEs can display usage info.Cargo.toml (2)
7-7: Remove commented codeThe commented out
namefield should either be removed or uncommented if needed.
15-16: Consider consistent version constraint styleThe version constraints use different styles:
- pyo3: Pinned to exact version "0.23.3"
- numpy: Allows minor updates "0.23"
Consider using consistent version constraints to make dependency management more predictable. Either pin both to exact versions or allow minor updates for both.
.pre-commit-config.yaml (1)
13-13: Consider consistent version pinning strategyThe hooks use different version pinning styles:
- validate-pyproject: v0.23
- typos: v1.28.3
- prettier: v3.1.0
- ruff-pre-commit: v0.8.3
Consider adopting a consistent version pinning strategy across all hooks to make updates more manageable.
Also applies to: 18-18, 42-42, 48-48
.github/workflows/wheels.yml (3)
3-16: Consider optimizing workflow triggersThe current configuration runs on all PR branches, which might lead to unnecessary builds. Consider limiting PR triggers to specific branches or adding path filters to run only when relevant files change.
pull_request: branches: - - "**" + - "main" + - "setup_repo" + paths: + - "**/*.py" + - "**/*.rs" + - "pyproject.toml" + - "Cargo.toml"
19-29: Add timeout settings to prevent hung jobsAdd a timeout-minutes setting to ensure the job doesn't run indefinitely if something goes wrong.
pre-commit: name: Run code checks runs-on: ubuntu-latest + timeout-minutes: 10 steps:
31-51: Enable pip caching for faster buildsWhile uv caching is enabled, explicitly caching pip dependencies can further improve build times.
- uses: actions/setup-python@v5 name: Install Python ${{ matrix.python_version }} with: python-version: ${{ matrix.python_version }} + cache: 'pip'tests/test_triangulate.py (1)
6-141: Add test cases for edge scenariosWhile the current test cases are good, consider adding the following edge cases:
- Empty path
- Single point path
- Path with duplicate points
- Path with very large coordinates
- Path with very small (near zero) coordinates
[ + ( + [], + False, + False, + 0, + [], + ), + ( + [[0, 0]], + False, + False, + 2, + [], + ), + ( + [[0, 0], [0, 0], [1, 1]], + False, + False, + 6, + [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5]], + ), # existing test cases... ],pyproject.toml (2)
5-28: Add recommended project metadataConsider adding the following fields to improve package metadata:
- license
- authors
- repository URL
- keywords
[project] name = "bermuda" dynamic = [ "version", ] description = "Spatial algorithms for napari project, compiled for performance" readme = "README.md" requires-python = ">=3.9" +license = "BSD-3-Clause" +authors = [ + {name = "Your Name", email = "[email protected]"} +] +urls = {Repository = "https://github.com/your-org/bermuda"} +keywords = ["spatial", "algorithms", "triangulation", "rust"]
158-160: Add Rust optimization settings for release buildsConsider adding Rust-specific environment variables to optimize the build:
[tool.cibuildwheel.linux] before-all = "curl https://sh.rustup.rs -sSf | sh -s -- -y" -environment = { PATH="$HOME/.cargo/bin:$PATH" } +environment = { + PATH="$HOME/.cargo/bin:$PATH", + RUSTFLAGS="-C target-cpu=native -C opt-level=3", + CARGO_PROFILE_RELEASE_LTO="true" +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/dependabot.yml(1 hunks).github/workflows/wheels.yml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)Cargo.toml(1 hunks)crates/bermuda/Cargo.toml(1 hunks)crates/bermuda/src/lib.rs(1 hunks)crates/triangulation/Cargo.toml(1 hunks)crates/triangulation/src/lib.rs(1 hunks)crates/triangulation/src/path_triangulation.rs(1 hunks)crates/triangulation/src/point.rs(1 hunks)pyproject.toml(1 hunks)python/bermuda/__init__.py(1 hunks)python/bermuda/_bermuda.pyi(1 hunks)tests/test_triangulate.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- .github/dependabot.yml
- python/bermuda/init.py
- crates/bermuda/Cargo.toml
- .gitignore
- crates/triangulation/Cargo.toml
🔇 Additional comments (3)
crates/triangulation/src/point.rs (1)
66-72: Consider floating-point precision for hashing.
Using f32.to_bits() for hashing is not necessarily problematic, but be aware that small floating-point variations can break hashing collisions. This is acceptable if the domain is strictly controlled; otherwise, consider an approach that tolerates slight floating-point differences.
✅ Verification successful
Let me search for more context about how Point is used in the codebase, particularly in hash-based collections.
Let me search for more context about the triangulation algorithm and any tests that might show the precision requirements.
Let me try one final search to understand the usage context of Point in the triangulation implementation.
Based on the context gathered, I can now provide a final response:
The current Point hashing implementation is appropriate for this use case
The Point struct is used in a triangulation algorithm where exact point equality is required, as evidenced by the implementation of Eq trait and its usage in geometric calculations. The current hashing approach using to_bits() is correct because:
- Points are used in exact geometric calculations (vector operations, projections)
- The implementation requires strict point equality (not approximate)
- No fuzzy matching or approximate point comparison is needed in the triangulation algorithm
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Explanation:
# We search for usage patterns of hashed Points to see if floating-point variations might cause undue collisions.
rg 'Hash for Point' -A 10
Length of output: 850
Script:
#!/bin/bash
# Search for Point usage in hash-based collections (HashSet, HashMap)
rg "Hash(Set|Map)<.*Point" -B 2 -A 2
# Also check the Point struct definition and any documentation
rg "struct Point" -B 5 -A 5
Length of output: 707
Script:
#!/bin/bash
# Search for test cases involving Point
rg "test.*Point" -A 10
# Look for any documentation or comments about precision requirements
rg -i "precision|epsilon|tolerance" crates/triangulation/
# Check if there's any README or documentation
cat crates/triangulation/README.md 2>/dev/null
Length of output: 121
Script:
#!/bin/bash
# Look for Point usage in the main triangulation logic
rg "Point" crates/triangulation/src/ -B 2 -A 2
# Check for any floating-point comparisons
rg "f32::epsilon|f32::EPSILON" crates/triangulation/src/
Length of output: 7910
.github/workflows/wheels.yml (1)
93-113: LGTM! Secure PyPI publishing configuration
The configuration uses trusted publishing and has appropriate conditions for releases.
pyproject.toml (1)
118-130: Review and address ignored lint rules
Some ignored rules have TODO comments indicating they need to be fixed:
- B028 (no-explicit-stacklevel)
- SIM117 (merge-with-statements) is waiting for Python 3.9 drop
Consider creating issues to track these technical debt items.
Would you like me to create GitHub issues to track fixing these ignored rules?
| let path_: Vec<Point> = path | ||
| .as_array() | ||
| .rows() | ||
| .into_iter() | ||
| .map(|row| Point { | ||
| x: row[0], | ||
| y: row[1], | ||
| }) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add dimension validation for the input array
The review comment is valid. The current implementation assumes the input array has exactly 2 columns but doesn't validate this. The PyReadonlyArray2 type ensures it's a 2D array, but accessing row[0] and row[1] without checking the shape could panic if the array has fewer than 2 columns. Additionally, the underlying Rust implementation checks for path length < 2 but this check happens after the conversion.
- Add shape validation before the conversion:
if path.shape()[1] != 2 {
return Err(PyValueError::new_err("Input array must have shape Nx2"));
}
if path.shape()[0] < 2 {
return Err(PyValueError::new_err("Input array must have at least 2 points"));
}🔗 Analysis chain
Validate array dimensions before processing.
When converting the Python array into a Rust Vec<Point>, it might be safer to check that the array has exactly two columns and that its shape is non-empty. Otherwise, an out-of-bounds access might occur.
Let me gather more information about the input validation by checking the Python bindings and the actual implementation.
Let me check the Python bindings in more detail to see how the array dimensions are handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Explanation:
# We search for references to `triangulate_path_edge` calls. We verify that all call sites provide valid Nx2 arrays.
# The presence of pre-validation or dimension checks in the surrounding code is also important.
rg -A 5 'triangulate_path_edge'
Length of output: 3272
Script:
#!/bin/bash
# Check the Python bindings for any input validation
ast-grep --pattern 'fn triangulate_path_edge<$_>($_) {
$$$
}'
# Also check the actual implementation in triangulation crate
rg -A 10 'pub fn triangulate_path_edge' crates/triangulation/src/
Length of output: 1111
Script:
#!/bin/bash
# Check the Python bindings implementation
cat crates/bermuda/src/lib.rs
Length of output: 2628
| fn add_triangles_for_join( | ||
| triangles: &mut PathTriangulation, | ||
| p1: point::Point, | ||
| p2: point::Point, | ||
| p3: point::Point, | ||
| prev_length: point::Coord, | ||
| cos_limit: point::Coord, | ||
| bevel: bool, | ||
| ) -> f32 { | ||
| let idx = triangles.offsets.len(); | ||
| let mut mitter = point::Vector::new(0.0, 0.0); | ||
| let length = point::vector_length(p2, p3); | ||
| let p1_p2_diff_norm = (p2 - p1) / prev_length; | ||
| let p2_p3_diff_norm = (p3 - p2) / length; | ||
|
|
||
| let cos_angle = | ||
| p1_p2_diff_norm.x * p2_p3_diff_norm.x + p1_p2_diff_norm.y * p2_p3_diff_norm.y; | ||
| let sin_angle = | ||
| p1_p2_diff_norm.x * p2_p3_diff_norm.y - p1_p2_diff_norm.y * p2_p3_diff_norm.x; | ||
|
|
||
| triangles.centers.push(p2); | ||
| triangles.centers.push(p2); | ||
|
|
||
| // Check sin_angle to compute mitter vector | ||
| if sin_angle == 0.0 { | ||
| mitter = point::Vector::new(p1_p2_diff_norm.y / 2.0, -p1_p2_diff_norm.x / 2.0); | ||
| } else { | ||
| let mut scale_factor = 1.0 / sin_angle; | ||
| if bevel || cos_angle < cos_limit { | ||
| // Compute bevel join and handle limits for inner vector length | ||
| let (sign, mag) = sign_abs(scale_factor); | ||
| scale_factor = sign * 0.5 * mag.min(prev_length.min(length)); | ||
| } | ||
| mitter = (p1_p2_diff_norm - p2_p3_diff_norm) * scale_factor * 0.5; | ||
| } | ||
|
|
||
| if bevel || cos_angle < cos_limit as f32 { | ||
| triangles.centers.push(p2); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx, idx + 1, idx + 2)); | ||
|
|
||
| if sin_angle < 0.0 { | ||
| triangles.offsets.push(mitter); | ||
| triangles.offsets.push(point::Vector::new( | ||
| -p1_p2_diff_norm.y * 0.5, | ||
| p1_p2_diff_norm.x * 0.5, | ||
| )); | ||
| triangles.offsets.push(point::Vector::new( | ||
| -p2_p3_diff_norm.y * 0.5, | ||
| p2_p3_diff_norm.x * 0.5, | ||
| )); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx, idx + 2, idx + 3)); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx + 2, idx + 3, idx + 4)); | ||
| } else { | ||
| triangles.offsets.push(point::Vector::new( | ||
| p1_p2_diff_norm.y * 0.5, | ||
| -p1_p2_diff_norm.x * 0.5, | ||
| )); | ||
| triangles.offsets.push(-mitter); | ||
| triangles.offsets.push(point::Vector::new( | ||
| p2_p3_diff_norm.y * 0.5, | ||
| -p2_p3_diff_norm.x * 0.5, | ||
| )); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx + 1, idx + 2, idx + 3)); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx + 1, idx + 3, idx + 4)); | ||
| } | ||
| } else { | ||
| triangles.offsets.push(mitter); | ||
| triangles.offsets.push(-mitter); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx, idx + 1, idx + 2)); | ||
| triangles | ||
| .triangles | ||
| .push(point::Triangle::new(idx + 1, idx + 2, idx + 3)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor large function.
The function "add_triangles_for_join" is quite large and handles multiple steps (computing angles, offsets, generating indexes, etc.). Splitting the logic into smaller helper functions could improve readability and testability.
.pre-commit-config.yaml
Outdated
| language: system | ||
| types: [rust] | ||
| pass_filenames: false | ||
| files: ^crates/(uv-cli|uv-settings)/ |
There was a problem hiding this comment.
💡 Codebase verification
Files pattern in pre-commit hook doesn't match project structure
The cargo-dev-generate-all hook's file pattern ^crates/(uv-cli|uv-settings)/ is incorrect. The repository actually contains different crates:
crates/bermudacrates/triangulation
This pattern should be updated to match the actual project structure, otherwise the hook won't run on the correct files.
🔗 Analysis chain
Verify the files pattern restriction
The cargo-dev-generate-all hook is restricted to crates/(uv-cli|uv-settings)/ but these directories aren't mentioned in the workspace setup. Ensure this pattern matches your project structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the specified directories exist and contain Rust files
fd -t d "^(uv-cli|uv-settings)$" crates/
Length of output: 42
Script:
#!/bin/bash
# Let's check the actual directory structure and Cargo.toml files
fd -t d "^crates$"
fd -t f "Cargo.toml" crates/
ls -la crates/ 2>/dev/null || echo "No crates directory found"
Length of output: 359
Script:
#!/bin/bash
# Let's also check the git history to see if these directories were renamed or moved
git log --oneline --all --full-history -- 'crates/uv-cli' 'crates/uv-settings'
Length of output: 0
Script:
#!/bin/bash
# Let's examine the workspace configuration
fd "Cargo.toml" -d 1
cat Cargo.toml 2>/dev/null || echo "No root Cargo.toml found"
Length of output: 474
There was a problem hiding this comment.
@Czaki, noted. Let me know if you need any further assistance or if there’s anything else to address!
Co-authored-by: Wouter-Michiel Vierdag <[email protected]>
This PR contains an implementation of edge triangulation in Rust. It looks slightly faster than C++ version (but I think that I know the source of at least part of the slowdown).
It is much faster than numba.
data.zip
Summary by CodeRabbit
New Features
triangulate_path_edgefunction for triangulating paths exposed to Python..gitignorefile for various programming environments.pyproject.tomlfile for defining build system and project metadata for the "bermuda" package.Bug Fixes
Tests
triangulate_path_edgefunction to validate triangulation logic.Documentation
__init__.pyto include the newtriangulate_path_edgefunction in the public API.