Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove clone in geometry trait implementation #1121

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

kylebarron
Copy link
Collaborator

@kylebarron kylebarron commented Nov 28, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

I struggled with this initially because I thought I had to implement Iter in terms of Self::ItemType, which resulted in a double &&Self::ItemType.

In geoarrow-rs this resulted in a massive speedup (see geoarrow/geoarrow-rs#278 (comment)) for converting geo objects to geoarrow (which is entirely implemented on top of these traits):

convert Vec<geo::Polygon> to PolygonArray
                        time:   [31.564 µs 31.608 µs 31.661 µs]
                        change: [-52.414% -52.271% -52.131%] (p = 0.00 < 0.05)
                        Performance has improved.

In the short term, in geoarrow-rs this also means all algorithms that result in geometries will get faster (though this probably isn't a huge part of the overall time) because currently all algorithms are implemented by converting each geometry to geo, applying the operation, and converting the output vec back to geoarrow.

Copy link
Contributor

@RobWalt RobWalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 👍🏼

@kylebarron kylebarron merged commit 6b44e27 into georust:traits Nov 28, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants