implement euclidean distance for all geometry types #1029
Conversation
There was a problem hiding this comment.
Hi @mcassels - thanks for the PR!
I haven't reviewed very thoroughly yet, but I had a question about the usage of clone - see my inline commentary.
| T: GeoFloat + FloatConst + RTreeNum + FloatConst, | ||
| { | ||
| fn euclidean_distance(&self, geom: &Geometry<T>) -> T { | ||
| let self_as_geom = Geometry::from(self.clone()); |
There was a problem hiding this comment.
This usage of clone is unfortunate, since it potentially doubles our memory usage. Did you explore alternatives that don't require a clone here? Maybe it'd be a ton of code?
There was a problem hiding this comment.
Thanks for the review! I've updated this to avoid the use of clone
There was a problem hiding this comment.
I'm quite personally interested in this :) Could the use of cloning here be avoided by creating the impls manually?
There was a problem hiding this comment.
@JosiahParry the most recent commit removes this use of clone :)
| T: GeoFloat + FloatConst + RTreeNum + FloatConst, | ||
| { | ||
| fn euclidean_distance(&self, other: &$for) -> T { | ||
| match self { |
There was a problem hiding this comment.
We end up doing this in a lot of places. You can use:
impl<T> EuclideanDistance<T, $for> for Geometry<T>
where
T: GeoFloat + FloatConst + RTreeNum
{
crate::geometry_delegate_impl! {
fn euclidean_distance(&self, other: &$for) -> T;
}
} | impl_euclidean_distance_for_iter_geometry!(MultiPoint<T>, [Point<T>, Line<T>, LineString<T>, MultiLineString<T>, Polygon<T>, MultiPolygon<T>, GeometryCollection<T>]); | ||
| impl_euclidean_distance_for_iter_geometry!(MultiLineString<T>, [Point<T>, Line<T>, LineString<T>, Polygon<T>, MultiPolygon<T>, GeometryCollection<T>]); | ||
| impl_euclidean_distance_for_iter_geometry!(MultiPolygon<T>, [Point<T>, Line<T>, LineString<T>, Polygon<T>, GeometryCollection<T>]); | ||
| impl_euclidean_distance_for_iter_geometry!(GeometryCollection<T>, [Point<T>, Line<T>, LineString<T>, Polygon<T>]); |
There was a problem hiding this comment.
Each of these macros is taking a smaller list of input for the second parameter. I assume that's because the implementations have already been covered by a previous macro.
The trouble is, it's hard to review this and be confident that we didn't miss one.
For example, while geometry.euclidean_distance(&multi_polygon) is implemented, we are missing geometry.euclidean_distance(&multi_point).
One idea I had is, even though it might be more typing, to not have the macro implementation be reflexive.
e.g. only implement impl<T> EuclideanDistance<T, $target> for $for, but not the inverse impl<T> EuclideanDistance<T, $for> for $target.
I think that might make it easier to scan the source code for missing combinations. e.g. something like
impl_euclidean_distance_for_iter_geometry!(MultiPoint<T>, [Point<T>, MultiPoint<T>, Line<T>, LineString<T>, MultiLineString<T>, Polygon<T>, MultiPolygon<T>, GeometryCollection<T>, Geometry<T>]);
impl_euclidean_distance_for_iter_geometry!(MultiLineString<T>, [Point<T>, MultiPoint<T>, Line<T>, LineString<T>, MultiLineString<T>, Polygon<T>, MultiPolygon<T>, GeometryCollection<T>, Geometry<T>]);
impl_euclidean_distance_for_iter_geometry!(MultiPolygon<T>, [Point<T>, MultiPoint<T>, Line<T>, LineString<T>, MultiLineString<T>, Polygon<T>, MultiPolygon<T>, GeometryCollection<T>, Geometry<T>]);
impl_euclidean_distance_for_iter_geometry!(GeometryCollection<T>, [Point<T>, MultiPoint<T>, Line<T>, LineString<T>, MultiLineString<T>, Polygon<T>, MultiPolygon<T>, GeometryCollection<T>, Geometry<T>]);
(Note I included Geometry in the second parameter list)
Possibly that second parameter could be baked into a macro itself so you don't have to type it all out, but I don't feel that strongly about that.
Similarly with the other macros.
There was a problem hiding this comment.
Thanks, I agree that it's hard to see if any combinations are missing. I've now updated this to make the macro implementations not reflexive, and also re-organized the macros a bit. I hope that it's more readable and clear now.
One note regarding this point:
geometry.euclidean_distance(&multi_polygon) is implemented, we are missing geometry.euclidean_distance(&multi_point)
In the new version, all the implementations of euclidean distance between Geometry<T> and specific geometry types are handled by 2 separate macros, so Geometry<T> is not included in the parameter lists for the multi-geometry macros. This seemed cleaner, since we need to have impl<T> EuclideanDistance<T, Geometry<T>> for all geometry types.
| fn euclidean_distance(&self, point: &Point<T>) -> T { | ||
| if self.intersects(point) { | ||
| return T::zero(); | ||
| fn euclidean_distance(&self, other: &Geometry<T>) -> T { |
There was a problem hiding this comment.
My condolences to your wrists. 😉
I'd expect this to be implementable as
impl<T> EuclideanDistance<T> for Geometry<T>
where T: GeoFloat + FloatConst
{
crate::geometry_delegate_impl! {
fn euclidean_distance(&self, other: &Geometry<T>) -> T;
}
}
Please revisit this method after addressing the missing implementations mentioned above.
|
Hi @mcassels, are you still interested in working on this? Let me know if you have any questions. If you don't think you'll have time, I can try to finish it up. |
|
@michaelkirk thanks for the review! I'm still planning to work on this, sorry for the delay -- been travelling with limited internet access. I'm hoping to get back to this within the next couple days |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
|
Amazing! |
CHANGES.mdif knowledge of this change could be valuable to users.This change implements #979