Skip to content

Conversation

@AndrewLipscomb
Copy link
Contributor

@AndrewLipscomb AndrewLipscomb commented Mar 22, 2025

  • [*] 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.

See #1320

Two things here - first is optimising the LineLocatePoint algorithm.

A new bench shows this was the original timing

LineString line_locate_point
                        time:   [259.34 µs 259.57 µs 259.83 µs]
                        change: [-1.6333% -1.0309% -0.5978%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) high mild
  1 (1.00%) high severe

After updates

LineString line_locate_point
                        time:   [73.201 µs 73.534 µs 74.106 µs]
                        change: [-71.777% -71.684% -71.525%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

Great success, very nice.

I've put some of this work out publicly under a public ComparableDistance trait. However I don't think that's necessarily the best idea.

I started off just working off of Boost Geometry's comparable_distance idea - but I think given

  • This will be at least strictly Euclidean to start, which implies all "comparable distances" will just be the non-sqrt'ed versions which is kind of handy to know
  • I have no idea what a ComparableDistance trait would look like for non-Euclidean spaces

I think it would be better exposed as an explicit DistanceSquared trait that only Euclidean implements - 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.

@michaelkirk
Copy link
Member

This seems great! No notes. 👏👏 👏

I think it would be better exposed as an explicit DistanceSquared trait that only Euclidean implements - if I am writing my own algo's I'd like to have it reliably convertible to a real distance.

That makes sense to me. Though I'm also not super bothered by having a ComparableDistance trait which is currently only implemented for Euclidean.

@AndrewLipscomb
Copy link
Contributor Author

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

@urschrei
Copy link
Member

No rush whatsoever!

@bennjii
Copy link

bennjii commented Apr 27, 2025

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.

Copy link
Member

@michaelkirk michaelkirk left a 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`
Copy link
Member

@michaelkirk michaelkirk Jun 10, 2025

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).

@bennjii
Copy link

bennjii commented Jun 11, 2025

I've made fixes for the CI errors in AndrewLipscomb#1 if that helps :)

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.

4 participants