chore: Port georust/geo changes to sedona-geo-generic-alg #261
Conversation
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
…enerated TODO_GEO_COMMITS.txt
…nerated TODO_GEO_COMMITS.txt
…skip optimized intersects; mark PR #1377 SKIPPED
Renamed rectangle corner variables for clarity: - lt/rb (left-top/right-bottom) → lb/rt (left-bottom/right-top) - This makes variable names match their actual positions - lb = min (left-bottom), rt = max (right-top) Ported from georust/geo commits: - a83b4d50: Merge pull request #1368 - bbacf118: rename rectangle corners Tests: All 295 tests pass
…Rect v. Triangle
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR ports optimization changes from georust/geo to sedona-geo-generic-alg, primarily focusing on improving the performance of intersection operations. The changes include algorithm optimizations, improved implementations for specific geometry type intersections, and updates to use more idiomatic constructors for empty geometries.
- Optimized polygon-polygon intersection logic to avoid redundant checks
- Added specialized intersection implementations for Triangle and Rect with Polygon
- Replaced verbose empty geometry constructors with concise
.empty()calls
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/sedona-geo-traits-ext/src/multi_polygon.rs | Expanded trait implementations by removing macro usage and explicitly defining polygon access methods |
| rust/sedona-geo-generic-alg/src/algorithm/map_coords.rs | Updated to use LineString::empty() instead of LineString::new(vec![]) |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/triangle.rs | Added optimized Triangle-Polygon and Triangle-Rect intersection implementations with tests |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/rect.rs | Added optimized Rect-Polygon intersection implementation and refactored Line intersection logic |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/polygon.rs | Refactored polygon-polygon intersection with optimized algorithm and removed delegated implementations |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/point.rs | Updated georust/geo source commit reference |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/mod.rs | Updated source reference and added missing test coverage for Coord intersections |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/line_string.rs | Added specialized LineString intersection implementations for Polygon, MultiPolygon, Rect, and Triangle |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/line.rs | Updated georust/geo source commit reference |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/coordinate.rs | Updated georust/geo source commit reference |
| rust/sedona-geo-generic-alg/src/algorithm/intersects/collections.rs | Updated georust/geo source commit reference |
| rust/sedona-geo-generic-alg/src/algorithm/dimensions.rs | Updated documentation examples to use .empty() constructor |
| rust/sedona-geo-generic-alg/src/algorithm/coordinate_position.rs | Updated test to use Polygon::empty() constructor |
| rust/sedona-geo-generic-alg/src/algorithm/centroid.rs | Updated tests to use .empty() constructors for MultiLineString and MultiPolygon |
| rust/sedona-geo-generic-alg/benches/intersection.rs | Added new benchmark cases for various intersection scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/sedona-geo-generic-alg/src/algorithm/intersects/triangle.rs
Outdated
Show resolved
Hide resolved
rust/sedona-geo-generic-alg/src/algorithm/intersects/triangle.rs
Outdated
Show resolved
Hide resolved
rust/sedona-geo-generic-alg/src/algorithm/intersects/triangle.rs
Outdated
Show resolved
Hide resolved
rust/sedona-geo-generic-alg/src/algorithm/intersects/triangle.rs
Outdated
Show resolved
Hide resolved
rust/sedona-geo-generic-alg/src/algorithm/intersects/triangle.rs
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Ported changes are mostly for optimizing intersects:
emptyconstructor to other crates. georust/geo#1364We observed performance improvements after applying these patches. Now the performance of sedona-geo-generic-alg's intersects function is on par with the upstream georust/geo:
Distance computation uses intersects internally, so the performance of distance computation should be improved theoretically. However, I have not observed performance improvements by running the distance bench, probably because it only involves simple shapes.