-
Notifications
You must be signed in to change notification settings - Fork 228
Optimise line locate point #1334
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
base: main
Are you sure you want to change the base?
Optimise line locate point #1334
Conversation
|
This seems great! No notes. 👏👏 👏
That makes sense to me. Though I'm also not super bothered by having a ComparableDistance trait which is currently only implemented for Euclidean. |
|
Just FYI - I've gotten bogged down with work this week. Still aiming to fix this up but will need a weekend to do it |
|
No rush whatsoever! |
|
Awesome work @AndrewLipscomb, is there anything I can do to help this PR along? Happy to have a raise a PR in your fork to resolve those CI failures. |
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.
This seems great and results in a nice speedup. We should merge it soon.
I know you've expressed a concern about the naming of the trait - I don't particularly share that concern. I'm fine either way.
If you have time to make those changes soon, I'll re-review, otherwise l'll fix up the docs and merge in a couple days.
| /// faster to compute this value rather than true distance if all you need is a comparable figure between two | ||
| /// geometries | ||
| /// | ||
| /// Note that not all implementations support all geometry combinations, but at least `Point` to `Point` |
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.
This comment seems spurious (maybe it was copied from the Distance trait which was implemented on more than just the Euclidean metric space).
|
I've made fixes for the CI errors in AndrewLipscomb#1 if that helps :) |
CHANGES.mdif knowledge of this change could be valuable to users.See #1320
Two things here - first is optimising the LineLocatePoint algorithm.
A new bench shows this was the original timing
After updates
Great success, very nice.
I've put some of this work out publicly under a public
ComparableDistancetrait. However I don't think that's necessarily the best idea.I started off just working off of Boost Geometry's
comparable_distanceidea - but I think givenComparableDistancetrait would look like for non-Euclidean spacesI think it would be better exposed as an explicit
DistanceSquaredtrait that onlyEuclideanimplements - if I am writing my own algo's I'd like to have it reliably convertible to a real distance.Keen for some feedback here - I still have some clean up here before I mark this as ready to go.