Add Concave Hull algorithm #480
Conversation
|
Thanks for the addition, we'd be delighted to have it - let us know when you're ready for a review, or if you have any other questions in the mean time! |
JavedNissar
force-pushed
the
concave-hull
branch
2 times, most recently
from
172187a to
f079984
Compare
|
@urschrei I think I'm close. Unfortunately, I'm struggling with setting up a search function for the R tree. I have the following: impl<T, W> SelectionFunction<T> for SelectWithinDistanceOfGeomFunction<W>
where
T: RTreeObject + EuclideanDistance<W, Line<W>>,
W: Float + RTreeNum {
fn should_unpack_parent(&self, envelope: &T::Envelope) -> bool {
envelope.euclidean_distance(&self.geometry) < self.max_dist
}
fn should_unpack_leaf(&self, leaf: &T) -> bool {
leaf.euclidean_distance(&self.geometry) < self.max_dist
}
}This results in the following compile error: As you can imagine, EuclideanDistance isn't implemented on the envelope but it is implemented on the generic T. Is it possible to convert the envelope into T? If not, do you have an alternative formulation of the types and the implementation that would be better than this one? Any help would be appreciated. |
|
@restitutororbis Just a brief comment as I'm pushed for time: not that I know of, but I'm not much of an authority on types – one possibility may be to use the |
JavedNissar
force-pushed
the
concave-hull
branch
from
f079984 to
126d595
Compare
JavedNissar
force-pushed
the
concave-hull
branch
2 times, most recently
from
40c57a3 to
3c7b3ba
Compare
|
@restitutororbis - were you able to work through the envelope type errors? LMK if not, and I can take a look to see if I have any ideas. |
|
@michaelkirk Unfortunately not, I instead took an alternative route where I use the distance from the center of the line which doesn't exactly match up with Concaveman but it's similar enough. If you want though, feel free to try to push forward on the custom search function. |
|
Sure - which commit would best exemplify the currently faced issue? |
|
@michaelkirk I'm confused. If you're talking about the custom search function that got overwritten when I adopted my new approach. Looking back on it, I probably should have just kept committing bit it's a bit late for that. Regardless, I can try to re-implement my approach over the weekend but that seems like a fool's errand to me since that didn't compile. If you're talking about the current action points. All that's left is tests. The current implementation should work so it's just a matter of testing various usecases. I got some ideas for tests that I'll hopefully push up tonight. |
|
I think I misunderstood. I was offering to help look into a compiler error, and wanted to know which sha to build from to see said error. If you're no longer blocked by anything, I don't need to step on your toes. =) |
|
@michaelkirk Oh, no worries. I think that was a misunderstanding more on my part than yours anyways. I'm not blocked on anything atm. |
JavedNissar
force-pushed
the
concave-hull
branch
4 times, most recently
from
da4184c to
ae6480e
Compare
JavedNissar
force-pushed
the
concave-hull
branch
from
ae6480e to
dde91d6
Compare
|
@urschrei Do you happen to have the time to review this any time soon? No worries if not. |
|
If by soon you mean "this week", then yes! |
There was a problem hiding this comment.
Thanks for working on this @restitutororbis!
Some other folks might want to take a look, but I've left a first-round of feedback (some of it labeled as optional).
| @@ -0,0 +1,150 @@ | |||
| vec![ | |||
| [5.227792433014884,59.468030338041025], | |||
There was a problem hiding this comment.
Did this fixture come from somewhere else or is this more so intended as a regression check?
I wonder if we should add a json parser to our devDependencies to make fixture wrangling easier/less error prone...
There was a problem hiding this comment.
This is a regression check. I ran the algorithm against the Norway data and exported the output to determine if the algorithm has changed in the future.
geo/examples/concavehull-usage.rs
Outdated
| @@ -0,0 +1,85 @@ | |||
| use geo::algorithm::concavehull::ConcaveHull; | |||
There was a problem hiding this comment.
thanks for including an example!
geo/src/algorithm/concavehull.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub trait ConcaveHull<T> { |
There was a problem hiding this comment.
Could you please add a doc comment, including an example invocation, to this trait declaration.
Also, minor code organization nit, typically we order things like:
- trait declaration
- trait impls
- private helper functions
- test module
I think the idea is that often people's preferred documentation is the source code itself, so we should order it in a way that puts the more relevant usage info towards the type top.
So, could you move the private methods defined above this trait declaration to be after the trait's impls (just before mod test?
geo/src/algorithm/concavehull.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn concave_hull<T>(points: Vec<Point<T>>, concavity: T) -> Vec<Point<T>> |
There was a problem hiding this comment.
Could you include a comment with a link to the version/line of the source inspiration for the implementation.
(I think maybe it's https://github.com/mapbox/concaveman/blob/54838e1/index.js#L11)?
This is helpful if we later find bugs or have questions about why things are a certain way.
There was a problem hiding this comment.
seems like we could get rid of some more clones if this method would borrow points, I don't see a reason to move it.
geo/src/algorithm/concavehull.rs
Outdated
| } | ||
| while !line_queue.is_empty() { | ||
| let maybe_line = line_queue.pop_front(); | ||
| if let Some(line) = maybe_line { |
There was a problem hiding this comment.
Alternatively:
while let Some(line) = line_queue.pop_front() {
let edge_length = line.euclidean_length();
....
But take it or leave it.
geo/src/algorithm/concavehull.rs
Outdated
| let maybe_line = line_queue.pop_front(); | ||
| if let Some(line) = maybe_line { | ||
| let edge_length = line.euclidean_length(); | ||
| let dist = edge_length.clone() / concavity; |
geo/src/algorithm/concavehull.rs
Outdated
| let edge_length = line.euclidean_length(); | ||
| let dist = edge_length.clone() / concavity; | ||
| let possible_closest_point = | ||
| find_point_closest_to_line(interior_points_tree.clone(), line, dist); |
There was a problem hiding this comment.
seems like we should borrow &interior_points_tree and remove the clone
geo/src/algorithm/concavehull.rs
Outdated
| T: Float + RTreeNum, | ||
| { | ||
| let w = line.euclidean_length() + max_dist.clone() + max_dist.clone(); | ||
| let h = max_dist.clone() + max_dist.clone(); |
There was a problem hiding this comment.
unnecessary clones in the preceding two lines
geo/src/algorithm/concavehull.rs
Outdated
| .collect(); | ||
| let mut interior_points_tree: RTree<Point<T>> = RTree::bulk_load(interior_points.clone()); | ||
|
|
||
| let mut concave_list: Vec<Point<T>> = Vec::default(); |
There was a problem hiding this comment.
idiomatically, just vec![], like the line below.
geo/src/algorithm/concavehull.rs
Outdated
| let mut candidates = interior_points_tree | ||
| .locate_within_distance(line.centroid(), search_dist) | ||
| .peekable(); | ||
| let peek = candidates.peek(); |
There was a problem hiding this comment.
would the rest of this method be cleaner implemented using candidates.min_by(...?
Though maybe it'd be less clean that way once you have to explicitly handle NaN... so take it or leave it.
|
@michaelkirk Thanks for the review! I'll address your feedback as soon as I can. |
JavedNissar
force-pushed
the
concave-hull
branch
from
dde91d6 to
ffa07b3
Compare
geo/src/algorithm/concavehull.rs
Outdated
| where | ||
| T: Float + RTreeNum, | ||
| { | ||
| let h = max_dist.add(max_dist.clone()); |
There was a problem hiding this comment.
@michaelkirk Is there a way to avoid cloning here? When I take it out, I get the following error:
error[E0308]: mismatched types
--> geo/src/algorithm/concavehull.rs:125:26
|
117 | fn find_point_closest_to_line<T>(
| - this type parameter
...
125 | let h = max_dist.add(max_dist);
| ^^^^^^^^
| |
| expected type parameter `T`, found `&T`
| help: consider dereferencing the borrow: `*max_dist`
|
= note: expected type parameter `T`
found reference `&T`
= help: type parameters must be constrained to match other types
= note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters
geo/src/algorithm/concavehull.rs
Outdated
| where | ||
| T: Float + RTreeNum, | ||
| { | ||
| let multipoint: MultiPoint<T> = points.clone().into(); |
There was a problem hiding this comment.
@michaelkirk Similarly, is there a way to avoid cloning here? Once again, the error is below:
error[E0277]: the trait bound `geo_types::Point<T>: std::convert::From<&std::vec::Vec<geo_types::Point<T>>>` is not satisfied
--> geo/src/algorithm/concavehull.rs:151:44
|
151 | let multipoint: MultiPoint<T> = points.into();
| ^^^^ the trait `std::convert::From<&std::vec::Vec<geo_types::Point<T>>>` is not implemented for `geo_types::Point<T>`
|
= help: the following implementations were found:
<geo_types::Point<T> as std::convert::From<(T, T)>>
<geo_types::Point<T> as std::convert::From<[T; 2]>>
<geo_types::Point<T> as std::convert::From<geo_types::Coordinate<T>>>
= note: required because of the requirements on the impl of `std::convert::Into<geo_types::Point<T>>` for `&std::vec::Vec<geo_types::Point<T>>`
= note: required because of the requirements on the impl of `std::convert::From<&std::vec::Vec<geo_types::Point<T>>>` for `geo_types::MultiPoint<T>`
= note: required because of the requirements on the impl of `std::convert::Into<geo_types::MultiPoint<T>>` for `&std::vec::Vec<geo_types::Point<T>>`
There was a problem hiding this comment.
I don't see an easy way to get rid of this one.
geo/src/algorithm/concavehull.rs
Outdated
| fn find_point_closest_to_line<T>( | ||
| interior_points_tree: &RTree<Point<T>>, | ||
| line: Line<T>, | ||
| max_dist: &T, |
There was a problem hiding this comment.
| max_dist: &T, | |
| max_dist: T, |
There was a problem hiding this comment.
I've never used GH code "suggestion" feature... let's see if it works, I tried to suggest the way that I would write this bit. T is Float, so that means it's also Copy. I'd probably lean into that here and not borrow T at all.
geo/src/algorithm/concavehull.rs
Outdated
| where | ||
| T: Float + RTreeNum, | ||
| { | ||
| let h = max_dist.add(max_dist.clone()); |
There was a problem hiding this comment.
| let h = max_dist.add(max_dist.clone()); | |
| let h = max_dist + max_dist; |
geo/src/algorithm/concavehull.rs
Outdated
| where | ||
| T: Float + RTreeNum, | ||
| { | ||
| let multipoint: MultiPoint<T> = points.clone().into(); |
There was a problem hiding this comment.
I don't see an easy way to get rid of this one.
geo/src/algorithm/concavehull.rs
Outdated
| @@ -0,0 +1,418 @@ | |||
| use crate::algorithm::convexhull::ConvexHull; | |||
There was a problem hiding this comment.
one more nit: could you rename this file to concave_hull.rs to be in line with the rest of the crate?
geo/src/algorithm/concavehull.rs
Outdated
| use std::ops::Add; | ||
|
|
||
| pub trait ConcaveHull<T> { | ||
| /// Returns the concave hull of a Polygon. |
There was a problem hiding this comment.
Thank you for adding some doc comments.
Since you've implemented this for more than just Polygons, maybe clearer to say something like:
Returns the concave hull of a Geometry.
Also bonus points if you can avoid using the term "concave hull" in your description. Maybe something like...
Returns a polygon which covers the geometry. Unlike convex hulls, which also cover their geometry, a concave hull does so while trying to further minimize its area by "shrink wrapping" around the geometry it covers.
Heh - do people know what "shrink wrapping" means? Anyway, good docs are tough, and somewhat subjective, so take it or leave it.
There was a problem hiding this comment.
What do you think about this?
/// Returns a polygon which covers a geometry. Unlike convex hulls, which also cover
/// their geometry, a concave hull does so while trying to further minimize its area by
/// constructing edges such that the exterior of the polygon incorporates points that would
/// be interior points in a convex hull.
JavedNissar
force-pushed
the
concave-hull
branch
2 times, most recently
from
c61c4b8 to
fadc7a9
Compare
Hi @gak! Thanks for reaching out, these flaws are definitely not your fault, I've been in the midst of a refactor and I pushed too soon. Try giving the latest revision a shot, it should pass your CI. Let me know if it doesn't. |
|
Thanks heaps for the fix, just had to rename the import and it worked again. I realise a branch in a PR is a WIP and totally don't expect it to be always working. It was just a heads up. 👍 |
There was a problem hiding this comment.
Had only one minor suggestion. Otherwise, lgtm.
Thanks for your contribution @restitutororbis . I'll give it a few days, to see if anyone else has suggestions, and otherwise merge the PR
geo/src/algorithm/concave_hull.rs
Outdated
| } | ||
| }) | ||
| .collect(); | ||
| let mut interior_points_tree: RTree<Coordinate<T>> = RTree::bulk_load(interior_coords.clone()); |
There was a problem hiding this comment.
interior_coords does not seem to be used after this; the clone can be removed.
geo-types/src/coordinate.rs
Outdated
| _ => unreachable!(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you please run cargo fmt and push up any changes?
geo/src/algorithm/concave_hull.rs
Outdated
| { | ||
| type Scalar = T; | ||
| fn concave_hull(&self, concavity: Self::Scalar) -> Polygon<Self::Scalar> { | ||
| let mut points: Vec<_> = self.exterior().points_iter().map(|point|{ |
There was a problem hiding this comment.
| let mut points: Vec<_> = self.exterior().points_iter().map(|point|{ | |
| let mut points: Vec<_> = self.exterior().0.clone(); |
There was a problem hiding this comment.
(and delete the following 2 lines)
geo/src/algorithm/concave_hull.rs
Outdated
| let mut aggregated: Vec<Coordinate<Self::Scalar>> = self | ||
| .0 | ||
| .iter() | ||
| .flat_map(|elem| elem.exterior().0.iter().map(|c| *c)) |
There was a problem hiding this comment.
| .flat_map(|elem| elem.exterior().0.iter().map(|c| *c)) | |
| .flat_map(|elem| elem.exterior().0.clone() |
geo/src/algorithm/concave_hull.rs
Outdated
| { | ||
| type Scalar = T; | ||
| fn concave_hull(&self, concavity: Self::Scalar) -> Polygon<Self::Scalar> { | ||
| Polygon::new(concave_hull(&mut self.clone().0, concavity), vec![]) |
There was a problem hiding this comment.
| Polygon::new(concave_hull(&mut self.clone().0, concavity), vec![]) | |
| Polygon::new(concave_hull(&mut self.0.clone(), concavity), vec![]) |
Pretty nitpicky - but since we only need the coords, we can clone just those, rather than cloning the entire Geometry.
geo/src/algorithm/concave_hull.rs
Outdated
| let mut aggregated: Vec<Coordinate<T>> = self | ||
| .0 | ||
| .iter() | ||
| .flat_map(|elem| elem.clone().0) |
There was a problem hiding this comment.
| .flat_map(|elem| elem.clone().0) | |
| .flat_map(|elem| elem.0.clone()) |
|
There is one useful optimization that is possible in the beginning of this algo. The implementation of quick hull actually guarantees that unless there are at most three coords in the input, the coords slice is mutated so that the first n entries are the hull points, and the rest are the interior (were n is size of conv hull). Since there's no concave-ness to add if we have < 4 points, we could handle that case and just return the conv hull. Then, the first part of figuring which are interior points becomes trivial (and we don't need the rtree for it). The guarantee I've mentioned above of the qhull implementation is not documented, so I'm okay with not doing that optimization in this PR. Just noting this so we may get to it at a later point. |
I've implemented the early return on less than 4 coordinates but I didn't have any luck implementing the optimization with regard to using the earlier portion of |
|
@restitutororbis Sorry, I didn't consider some of the recursive calls; the observation doesn't hold in the current impl. Please ignore the suggestion. |
|
No worries!
Sent with [ProtonMail](https://protonmail.com) Secure Email.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
…On Monday, September 28, 2020 4:26 AM, rmanoka ***@***.***> wrote:
***@***.***(https://github.com/RestitutorOrbis) Sorry, I didn't consider some of the recursive calls; the observation doesn't hold in the current impl. Please ignore the suggestion.
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#480 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAKU47Y4FDYWKNBMDEXNHO3SIBCCBANCNFSM4PCY5QAA).
|
There was a problem hiding this comment.
This LGTM - I'll wait a couple days before merging in case anyone else wants a final review.
Thanks for your contribution and your patience with the review process @restitutororbis!
|
bors r+ |
480: Add Concave Hull algorithm r=michaelkirk a=RestitutorOrbis As part of a-b-street/abstreet#198, I've been working on adding an implementation of the Concave Hull algorithm primarily inspired by https://github.com/mapbox/concaveman. This implementation doesn't completely copy concaveman but it does ape the notion of using R trees in order to avoid a linear search through the points although my use of R trees relies on a circular envelope rather than the rectangular envelope that concaveman seems to use. Co-authored-by: Javed Nissar <javed.nissar@mail.utoronto.ca>
|
Build failed: |
|
Do you know why this new test might be failing @restitutororbis? from https://github.com/georust/geo/runs/1190445056 |
JavedNissar
force-pushed
the
concave-hull
branch
from
035118d to
7b7271e
Compare
JavedNissar
force-pushed
the
concave-hull
branch
from
7b7271e to
02143f9
Compare
@michaelkirk It seems when I updated the impl, the Norway output changed as well. From looking at the SVG file, it doesn't seem visually distinct so I'm not entirely sure how concerned I should be about that. Do you think it would make sense to remove the Norway test? I was hoping it would be an effective regression test but it seems so brittle that any slight change to the algorithm perturbs it. For reference, from running If you think it is fine as is, then the updated Norway test works, so you should be able to merge it in without issues. |
geo/src/algorithm/concave_hull.rs
Outdated
| /// and also uses ideas from the following paper: | ||
| /// www.iis.sinica.edu.tw/page/jise/2012/201205_10.pdf | ||
| /// | ||
| /// # Example |
There was a problem hiding this comment.
| /// # Example | |
| /// # Examples | |
| /// |
| let mut candidates = interior_coords_tree | ||
| .locate_within_distance(centroid_coord, search_dist) | ||
| .peekable(); | ||
| let peek = candidates.peek(); |
There was a problem hiding this comment.
if you add a ? here, you can avoid doing the big match below
| let peek = candidates.peek(); | |
| let peek = candidates.peek()?; |
There was a problem hiding this comment.
When I try to do this, I get borrow checker errors that I'll need to work through so I'm going to elect to not proceed with this one.
geo/src/algorithm/concave_hull.rs
Outdated
| .filter_map(|coord| { | ||
| if !hull_tree.contains(coord) { | ||
| Some(*coord) | ||
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
i think this is the same?
| .filter_map(|coord| { | |
| if !hull_tree.contains(coord) { | |
| Some(*coord) | |
| } else { | |
| None | |
| } | |
| }) | |
| .filter(|coord| !hull_tree.contains(coord)) |
There was a problem hiding this comment.
Thanks for catching that! I've made a change to account for this.
|
@restitutororbis Thanks for this effort, and your patience! Let's leave the norway test in for now. bors r+ |
|
Build succeeded: |
|
Woohoo, thanks @restitutororbis! |
As part of a-b-street/abstreet#198, I've been working on adding an implementation of the Concave Hull algorithm primarily inspired by https://github.com/mapbox/concaveman. This implementation doesn't completely copy concaveman but it does ape the notion of using R trees in order to avoid a linear search through the points although my use of R trees relies on a circular envelope rather than the rectangular envelope that concaveman seems to use.