-
Notifications
You must be signed in to change notification settings - Fork 199
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
Add line segmentize trait #1055
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Looks like Clippy still isn't happy. Do you see those failures when you run it locally? |
This comment was marked as outdated.
This comment was marked as outdated.
I pulled the most recent changes from main and CI is failing unrelated to my changes.
|
A couple lines further up in the failed action output shows:
So, presumably this is some kind of transient network failure. I've restarted the jobs. 🤞 |
thanks @michaelkirk! Looks good now. |
BTW have you seen #1050? It's not quite the same thing, but it seems like maybe you could leverage the functionality within. Though, I'm not sure if you'd want to, I haven't thought about it very hard. |
#1050 is interesting and likely a bit more challenging than what i endeavored for here! I could give it a further look after this. |
Recent commits allow for |
I've updated the branch with main. Please let me know what more is needed for this PR. Thanks! |
Just following up here. Please let me know what more is needed on this PR. Thanks! |
@michaelkirk / @urschrei, please let me know what tasks are remaining for this PR. Thanks! R bindings to geo are based on my fork with this functionality so a downstream package can improve their peformance by 4 orders of magnitude. I'd love to be able to point to main and keep up with new PRs :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments and questions from me.
All comments are resolved. Praying to the CI overlords 🙏🏼 |
Guessing that was an inadvertent close. |
oops! Correct. Thank you @urschrei. CI caught an example issue. |
Changes should be resolved and CI is satisfied. @urschrei when you get a chance, let me know if I've missed anything! Thanks again for the feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll leave it open for another couple of days in case anyone else wants to take a look.
Sounds good! Enjoy you're weekend |
[tolling bell] |
CHANGES.md
if knowledge of this change could be valuable to users.This PR adds a new trait
LineStringSegmentize
which provides a methodline_segmentize(n: usize)
to splitLineString
s inton
equal length LineStrings which are returned as anOption<MultiLineString>
. This is inspired by the lwgeom R package which wraps liblwgeom from PostGIS.