Implementating Concavehull#1328
Conversation
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1328 +/- ##
==========================================
+ Coverage 85.79% 86.17% +0.37%
==========================================
Files 200 201 +1
Lines 6436 6497 +61
==========================================
+ Hits 5522 5599 +77
+ Misses 914 898 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
|
@souma4 what is the status of this one? Is it ready for review? |
| """ | ||
| struct Concave <: HullMethod end | ||
|
|
||
| function hull(points, ::Concave; k=3) |
There was a problem hiding this comment.
Notice that the design we follow is one in which every parameter of the method is stored in the method object itself. The k parameter should be a field of the Concave struct. Also, try to be more specific with the method name, perhaps KnnJarvisMarch?
Can't we adjust our current JarvisMarch to include a k parameter instead of repeating a similar implementation?
There was a problem hiding this comment.
As far as your first suggestion, that's easy to swap in.
For the second, Jarvis March works by performing a global ordering of points by "right hand turns." What do you think of making a more generic JarvisHull that can be used to dispatch the k-th jarvis march algorithm, but KnnJarvisMarch is a type that defaults to k=3, and JarvisMarch is just a method dispatch for hull where points are used to set k?
Something to the effect of
abstract type JarvisHull <: HullMethod end # I can see removing this and Making Knn JarvisMarch a subtype of HullMethod. That would avoid introducing a new abstract type. Just might make reduce extensibility.
struct JarvisMarch <: JarvisHull end
struct KnnJarvisMarch <: JarvisHull
k::Int
KnnJarvisMarch() = new(3)
KnnJarvisMarch(k::Int) = new(k)
end
function hull(points, method::J) where J <: JarvisHull #if we remove the abstract type, then just replace J with the concrete type
# k-nearest implementation that I propose
# ...
end
hull(points, ::JarvisMarch) = hull(points, KnnJarvisMarch(length(points))
Let me know what you think
There was a problem hiding this comment.
I would keep it as simple as possible. From what I can tell we only need to generalize the implementation of JarvisMarch to handle k. By default we can set k=nothing and update it inside the hull call to match the current behavior. If the generalization to handle k implies worst performance when k=npoints, then we can consider alternative designs.
|
@juliohm I still want to simplify the internal logic. The check for self intersection is a bit messy right now so I don't think it's ready. And with your other feedback, it might be worth slotting this Knn approach into |
|
@juliohm This should be good for review now. I refactored the main jarvis algorithm to dispatch on different |
|
Thank you @souma4. I will try to find some time to review the PR over the week. |
| # perform primary Jarvis march loop | ||
| ℐ = _jarvisloop(method, points, p, n, i, O, A) | ||
| # if no hull is found, return convex hull as fallback | ||
| isnothing(ℐ) && return convexhull(p) |
There was a problem hiding this comment.
We can't call convexhull, which is a higher-level abstraction inside hull, which is the low-level implementation. I also don't understand why is it possible that the indices are nothing. Whenever we call hull with a set of points, the result of the Jarvis loop should be valid, right?
| end | ||
|
|
||
| # concave hull | ||
| function _jarvisloop(method::JarvisMarch{I}, points, p, n, i, O, A) where {I<:Integer} |
There was a problem hiding this comment.
From the paper I recall that the only difference between JarvisMarch{Nothing} and JarvisMarch{K} the the set of candidate indices. In the former, we consider all remaining candidates, whereas in the latter we consider the K-nearest candidates.
Couldn't we simply preserve the original implementation, with minimal changes, by just introducing a helper function to specialize the set of candidates?
jarviscandidates(::JarvisMarch{Nothing}, ...) = # all remaining indices
jarviscandidates(::JarvisMarch{<:Integer}, ...) = # K-nearest indicesThe less we modify the original implementation, the less likely we introduce bugs.
If these functions need an extra state (e.g., KNN searcher), then this state could be instantiated out of the loop, and passed to the function:
searcher = nothing
searcher = KNearestSearch(...)
jarviscandidates(searcher::Nothing, ...)
jarviscandidates(searcher::KNearest, ...)Alternatively, you could select a function before the Jarvis loop based on K to avoid extra complications:
# define candidates function
candidates = if k isa Integer
# instantiate searcher once
searcher = KNearestSearch(...)
# return function that retrieves k-nearest candidates
i -> search(points[i], searcher)
else # k is nothing
# return function that retrieves remaining candidates
i -> setdiff(1:n, [i])
end
# now candidates can be used regardless of K
js = candidates(i)There was a problem hiding this comment.
Possibly. The biggest issue would be the self intersection check and reinserting the starting point to the KNearestSearch mask, but there is probably a solution. I'll work on that.
| # initial state for retries | ||
| i₀, O₀, A₀ = i, O, A | ||
|
|
||
| # try increasing k until valid hull found |
There was a problem hiding this comment.
I am concerned that you are implementing more than just Jarvis March with fixed K. This meta algorithm for increasing k is necessary or is it another layer of abstraction to enforce some kind of expected result?
If for some k and some specific points the algorithm is not valid (e.g., infinite loop), we should just return an error. Any additional logic to attempt multiple k could be made available in a separate method like JarvisMarchManyKAttempts or something.
There was a problem hiding this comment.
This is so that people don't need to tweak input k if their k is too low to find a valid result. we could easily do a wrap around the function for many k attempts. I'll roll with fixed k
…rather than separate concave method
…finding candidates and then picking the next
|
Sorry for the commit wall. I screwed up a rebase to the newest version of Meshes. Nice thing is this no longer contains commits from the integral patches. This is ready for review and I've left some comments to help in the review. |
|
Tests are failing, so it is not ready for review yet. |
|
huh... I really thought it would pass all tests, but it's some weird issue where infinite loops are occuring when the inputs are random. I'll figure it out. |
|
Okay, now we're good for a review |
There was a problem hiding this comment.
Thank you @souma4 for the updated PR. It is looking much cleaner now.
Try to define/document the 2 or 3 main operations that change in the presence or absence of k. With these operations well-defined, implement the corresponding methods.
My current understanding is that we only need to define the jarvissearcher operation, which creates a searcher or a dummy (nothing) object, the jarviscandidates operation, that takes the searcher and returns indices, and some sort of jarvisupdate operation.
| AdaptiveJarvisMarch(k) | ||
|
|
||
| Compute the concave hull of a set of points or geometries using an adaptive | ||
| version of the Jarvis's march algorithm. The algorithm will iteratively increase `k` until all points are in the hull | ||
|
|
||
| see also `JarvisMarch` for creating a `convexhull` and `concavehull` for default | ||
| concave hulls starting with `k = 3`. | ||
|
|
||
| """ | ||
| struct AdaptiveJarvisMarch{K<:JarvisMarch{<:Integer}} <: HullMethod | ||
| march::K | ||
| end | ||
| AdaptiveJarvisMarch() = AdaptiveJarvisMarch(JarvisMarch(3)) | ||
| AdaptiveJarvisMarch(k::I) where {I<:Integer} = AdaptiveJarvisMarch(JarvisMarch(k)) | ||
|
|
||
| function hull(points, method::AdaptiveJarvisMarch) | ||
| k = method.march.k | ||
| pointsꜝ = unique(points) | ||
| for kᵢ in k:length(pointsꜝ) | ||
| chul = hull(points, JarvisMarch(kᵢ)) | ||
| # validate we found a hull that contains all points, if so return it, otherwise increase k and try again | ||
| isnothing(chul) && continue | ||
| all(points .∈ Ref(chul)) && return chul | ||
| end | ||
| # otherwise, fallback to convex hull | ||
| hull(points, JarvisMarch()) | ||
| end |
There was a problem hiding this comment.
This algorithm is too simple to deserve a struct. Users will likely need different strategies for picking k and interrupting the loop. Please remove AdaptiveJarvisMarch from the PR. We can think about it later if really necessary.
| # if no candidates exist (k too large), fallback to convex hull | ||
| isnothing(𝒞) && return hull(points, JarvisMarch()) |
There was a problem hiding this comment.
This condition should happen in the beginning, right after the corner cases with n==1, n==2, n==3. You can simply check if k >= n and call the convex hull case.
| # corner cases | ||
| n == 1 && return p[1] | ||
| n == 2 && return Segment(p[1], p[2]) | ||
| n == 3 && return PolyArea(p) |
There was a problem hiding this comment.
Isn't this case already covered by the general algorithm? Is this extra line added for performance?
|
|
||
| # candidates for next point | ||
| 𝒞 = [1:(i - 1); (i + 1):n] | ||
| candidates = jarviscandidates(searcher, 𝒞, n, p, i, i, start) |
There was a problem hiding this comment.
It is much easier to understand the proposed changes if the variable names are not changed. Before this PR, we called 𝒞 the pool of candidate indices. Please preserve the name.
|
|
||
| _gconcavehull(geoms) = _pconcavehull(p for g in geoms for p in boundarypoints(g)) | ||
|
|
||
| _pconcavehull(points) = hull(points, AdaptiveJarvisMarch()) # default k = 3 for concave hulls |
There was a problem hiding this comment.
I think this internal _pconcavehull function could hold the heuristic you called AdaptiveJarvisMarch. The implementation should live here though.
| # can AdaptiveJarvisMarch(Integer) be constructed with an integer k and work | ||
| @test hull(cart.([(0, 0), (1, 0), (1, 1), (0, 1), (0.5, -1)]), AdaptiveJarvisMarch(3)) isa PolyArea | ||
|
|
||
| for method in [GrahamScan(), JarvisMarch(), AdaptiveJarvisMarch()] |
There was a problem hiding this comment.
Please remove the AdaptiveJarvisMarch from this testset and any related change. We need a dedicated test set for concave hulls.
|
@souma4 when do you think you will have some time to continue the work on this PR? :) |
|
This week I'll have the updated version pushed. |
…d some fn names to be more descriptive. Made variable names more consistent to original implementation
… JarvisMarch{Int} within existing tests, but ignore it for cases where invalid hulls are expected.
|
I think this is good for a review. |
| see `concavehull` for a version that iteratively increases `k` until all | ||
| points are in the hull, which is useful when an effective `k` is not known prior. | ||
| This gurantees correctness. |
There was a problem hiding this comment.
The concavehull is a higher-level function, and shouldn't be mentioned in the docstring of the implementation. Please remove this paragraph.
| If `k` is provided, the algorithm will attempt to compute a concave hull using k | ||
| nearest neighbors. However, the algorithm is not guaranteed to succeed for any particular `k`. |
There was a problem hiding this comment.
What happens when the algorithm does not succeed? That would be useful information to add to the docstring.
| struct JarvisMarch{K} <: HullMethod | ||
| k::K | ||
| end | ||
| JarvisMarch() = JarvisMarch{Nothing}(nothing) | ||
| JarvisMarch(k::I) where {I<:Integer} = JarvisMarch{I}(k) |
There was a problem hiding this comment.
| struct JarvisMarch{K} <: HullMethod | |
| k::K | |
| end | |
| JarvisMarch() = JarvisMarch{Nothing}(nothing) | |
| JarvisMarch(k::I) where {I<:Integer} = JarvisMarch{I}(k) | |
| struct JarvisMarch{K} <: HullMethod | |
| k::K | |
| end | |
| JarvisMarch() = JarvisMarch{Nothing}(nothing) | |
| JarvisMarch(k::I) where {I<:Integer} = JarvisMarch{I}(k) |
| n == 1 && return p[1] | ||
| n == 2 && return Segment(p[1], p[2]) | ||
| kₒ = method.k | ||
| isnothing(kₒ) || kₒ ≥ n && return hull(points, JarvisMarch()) |
There was a problem hiding this comment.
We need proper error handling when k > n. If the user specifies k manually, he/she should know that it can't be larger than n.
| jarvissearcher(k, n, p) = nothing, nothing, 1:n | ||
|
|
||
| function jarvissearcher(kₒ::T, n, p) where {T<:Integer} | ||
| k = min(max(kₒ, T(3)), n) |
There was a problem hiding this comment.
I feel that you constantly mix side effects from different auxiliary functions with side effects that are part of the main function. Why the jarvissearcher auxiliary function needs to correct the input k? Isn't that the job of the main function? If you are asking for a searcher with invalid k, and correcting the k inside the auxiliary function, it is really hard to predict what is going to happen later on in the main function.
Try to improve encapsulation. In this case, you should probably handle the corner cases with k=3 and k > n in the main function before calling jarvissearcher.
|
|
||
| Concave hull of `object`. | ||
| """ | ||
| # function concavehull end |
| function _pconcavehull(points) | ||
| pointsꜝ = unique(points) | ||
| n = length(pointsꜝ) - 1 | ||
| for k in 3:n | ||
| chul = hull(pointsꜝ, JarvisMarch(k)) | ||
| # validate we found a hull that contains all points, if so return it, otherwise increase k and try again | ||
| isnothing(chul) && continue | ||
| all(pointsꜝ .∈ Ref(chul)) && return chul | ||
| end | ||
| # otherwise, fallback to convex hull | ||
| hull(pointsꜝ, JarvisMarch()) | ||
| end |
There was a problem hiding this comment.
Please describe the heuristic that was implemented in the docstring. You're increasing k from 3 to n until all points are inside of the resulting polygonal area. Is there another heuristic to consider in practical applications? For example, select k such that the area is minimized? Select k such that all vertices of the resulting polygon are at least d meters apart? Should we give users the freedom to pick their own heuristic? If yes, I suggest that we add this concavehull utility in a separate PR.
2 participants
Implementing a k nearest neighbors approach for Concavehulls #497. The paper referenced in the issue is primarily for combining offset points and is quite a bit more involved than this paper. This looks very similar to Jarvis March, just with an added check to prevent self intersections. I refactored some of the convex hull tests to make sure we are properly testing hulls in general, but not holding concave hulls to the same standard as convex hulls.
I am curious, though, The tests starting at L43 and L75 give different polygons between concave and convex hulls. The convex hull hugs tightly to the points, but the concave hull smooths them. To me it seems like a valid solution (plus, concave hulls don't have a singular correct solution), but it seems sort of weird that the area of the convex hull is less than the area of the concave hull. Let me know what you think.
edit: Ah, I didn't catch it's just defaulting to convex hull because it fails to get some points inside the polygon. More work to be done. Will mark for review when it gets solved