-
Notifications
You must be signed in to change notification settings - Fork 205
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
line interpolate point across MetricSpaces #1321
base: main
Are you sure you want to change the base?
Conversation
ba43f1a
to
af415da
Compare
This is a big PR, but it's overwhelmingly docs and tests, I swear! |
6737a63
to
29eb15a
Compare
---- algorithm::linestring_segment::test::haversine_segment_length stdout ---- thread 'algorithm::linestring_segment::test::haversine_segment_length' panicked at geo/src/algorithm/linestring_segment.rs:333:13: assert_relative_eq!(Haversine.length(&segment), expected_segment_length, epsilon = 1e-9) left = 300003.0789340123 right = 299799.8099830805
…tart, not (Euclidean) line_interpolate_point
f4b888c
to
a18e16d
Compare
a18e16d
to
d421459
Compare
/// assert!(lines.next().is_none()); | ||
/// ``` | ||
pub fn rev_lines(&'_ self) -> impl ExactSizeIterator<Item = Line<T>> + '_ { | ||
self.0.windows(2).rev().map(|w| { |
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.
At first I assumed I wanted to do line_string.lines().rev()
but that's not quite what I needed.
For example, given a line string with points a, b, c
:
lines():
1. a->b
2. b->c
lines().rev():
1. b->c
2. a->b
But what I want is:
rev_lines():
1. c->b
2. b->a
I've just pushed up a bike-shedding commit. There are two equivalent new APIs: Euclidean.point_at_ratio_from_start(&line_string, 0.5)
line_string.point_at_ratio_from_start(&Euclidean, 0.5) Previously, the deprecation pointed you to the first version. I changed that deprecation to now recommend the latter. I don't think it matters much, but the second version is closer to the existing API, so I think it'll be slightly easier to migrate to. |
segment.line_interpolate_point((length - remainder) / length)?; | ||
|
||
let endpoint = $metric_space | ||
.point_at_ratio_from_start(&segment, (length - remainder) / length); |
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 is a bugfix in the case of Haversine
CHANGES.md
if knowledge of this change could be valuable to users.LineInterpolatePoint
is currently Euclidean only. As part of #1216, I wanted to make this functionality work across metric spaces.As a bonus, I also wanted to add some (IMO) long missing niceties to the api.
You can use all those methods across metric spaces:
The names are admittedly long. It's my own preference to have verbose method names over geos-like "mode" parameters.
Bonus
While working on this, and filling out the deprecations, I found and fixed a bug in
LineStringSegmentizeHaversine
which was using the legacyLineInterpolatePoint
, which was Euclidean only. It's easy to make this kind of mistake, the way we've historically not been very explicit about which metric space an algorithm is for.