deprecate EuclideanDistance #1232
Conversation
As part of aligning all our line measure traits (see #1181), all the Point x Point distance calculations had been moved into `line_measures::metric_spaces`. Euclidean space is the only space that implements distance calculations on *non* point geometries. This commit moves all those calculations onto the `line_measures::metric_spaces::Euclidean` so that we can fully deprecate the existing EuclideanDistance trait.
michaelkirk
changed the title
michaelkirk
force-pushed
the
mkirk/deprecate-euclidean-distance
branch
from
3cb6732
to
f09257e
Compare
michaelkirk
force-pushed
the
mkirk/deprecate-euclidean-distance
branch
from
f09257e
to
0e9c0a4
Compare
| impl<T> EuclideanDistance<T, Line<T>> for Coord<T> | ||
| where | ||
| T: GeoFloat, | ||
| { | ||
| /// Minimum distance from a `Coord` to a `Line` | ||
| fn euclidean_distance(&self, line: &Line<T>) -> T { | ||
| line.euclidean_distance(self) | ||
| Euclidean::distance(&Point(*self), line) |
There was a problem hiding this comment.
All the methods in this file now delegate to the new Euclidean as Distance implementation.
| if line_string.intersects(polygon) { | ||
| F::zero() | ||
| } else if !polygon.interiors().is_empty() | ||
| // FIXME: Explodes on empty line_string |
There was a problem hiding this comment.
re: FIXME - this panic on the next line isn't new behavior in this PR, but I noticed it while moving the code over.
Note that currently nearest_neighbor_distance returns Float::max_value if the input geometries are empty.
So, since we're already returning Float::max_value for some degenerate input, I'd argue it'd be preferable to always return Float::max_value for any degenerate input in this algorithm, rather than sometimes panic'ing and sometimes returning Float::max_value.
A larger breaking change could be to make distance return an Option.
❤️ empty geometries ❤️
| return F::zero(); | ||
| } | ||
|
|
||
| // REVIEW: This impl changed slightly. |
There was a problem hiding this comment.
Heads up that this impl changed slightly, though the behavior should be the same. I couldn't resist the simplification. 😇
All the other implementations were simply cut+paste+renamed.
The original impl is here: https://github.com/georust/geo/pull/1232/files#diff-23bef671023e471a3228d4ac73548395b9073026c10fb07f418ac3e81df8cdddL242
fn euclidean_distance(&self, other: &Polygon<T>) -> T {
if self.intersects(other) {
return T::zero();
}
// line-line distance between each exterior polygon segment and the line
let exterior_min = other
.exterior()
.lines()
.fold(<T as Bounded>::max_value(), |acc, point| {
acc.min(self.euclidean_distance(&point))
});
// line-line distance between each interior ring segment and the line
// if there are no rings this just evaluates to max_float
let interior_min = other
.interiors()
.iter()
.map(|ring| {
ring.lines().fold(<T as Bounded>::max_value(), |acc, line| {
acc.min(self.euclidean_distance(&line))
})
})
.fold(<T as Bounded>::max_value(), |acc, ring_min| {
acc.min(ring_min)
});
// return smaller of the two values
exterior_min.min(interior_min)
}| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
These are the tests copied from the legacy euclidean_distance trait, updated to use the new trait. I didn't review them very carefully, except to make sure that they still passed.
| @@ -93,9 +93,6 @@ pub fn partial_min<T: PartialOrd>(a: T, b: T) -> T { | |||
| } | |||
| } | |||
|
|
|||
| // Moved to their own module, but we re-export to avoid breaking the API. | |||
| pub use crate::coordinate_position::{coord_pos_relative_to_ring, CoordPos}; | |||
There was a problem hiding this comment.
The util module is not publicly accessible, so there's no need to worry about breaking any API's within it.
(I came across this because the only place using this call site was the internal implementation of the deprecated EuclideanDistance trait - which no longer uses it. The modern trait imports CoordPos from its "new" home.)
CHANGES.mdif knowledge of this change could be valuable to users.Part of #1181
In #1222, I moved only the
Point x Pointdistance calculations. EuclideanDistance is implemented for all other geometry types too. This PR moves the rest of those calculations to the newEuclideanline measure struct for consistency.It's a lot of code movement, but no change in behavior (unless I messed up 😬).