Implementating Concavehull

[souma4]

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

[github-actions]

Benchmark Results (Julia vlts)

Time benchmarks

	master	7e1ec3e...	master / 7e1ec3e...
clipping/SutherlandHodgman	3.72 ± 0.18 μs	3.74 ± 0.24 μs	0.994 ± 0.08
discretization/simplexify	0.672 ± 0.037 ms	0.677 ± 0.09 ms	0.992 ± 0.14
intersection/ray-triangle	0.05 ± 0 μs	0.05 ± 0.001 μs	1 ± 0.02
sideof/ring/large	6.53 ± 0.01 μs	6.53 ± 0.01 μs	1 ± 0.0022
sideof/ring/small	0.05 ± 0.01 μs	0.06 ± 0.01 μs	0.833 ± 0.22
topology/half-edge	2.74 ± 0.046 ms	2.73 ± 0.048 ms	1.01 ± 0.025
winding/mesh	16 ± 0.32 ms	16.1 ± 0.3 ms	0.993 ± 0.027
time_to_load	1.23 ± 0.0032 s	1.15 ± 0.0035 s	1.08 ± 0.0043

Memory benchmarks

	master	7e1ec3e...	master / 7e1ec3e...
clipping/SutherlandHodgman	0.053 k allocs: 4.97 kB	0.053 k allocs: 4.97 kB	1
discretization/simplexify	18.1 k allocs: 1.92 MB	18.1 k allocs: 1.92 MB	1
intersection/ray-triangle	0 allocs: 0 B	0 allocs: 0 B
sideof/ring/large	0 allocs: 0 B	0 allocs: 0 B
sideof/ring/small	0 allocs: 0 B	0 allocs: 0 B
topology/half-edge	18.1 k allocs: 2.92 MB	18.1 k allocs: 2.92 MB	1
winding/mesh	23.2 k allocs: 3.08 MB	23.2 k allocs: 3.08 MB	1
time_to_load	0.153 k allocs: 14.5 kB	0.153 k allocs: 14.5 kB	1

[codecov]

Codecov Report

❌ Patch coverage is 92.40506% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.87%. Comparing base (ffbde54) to head (7e1ec3e).
⚠️ Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
src/hulls.jl	53.84%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1328      +/-   ##
==========================================
+ Coverage   85.44%   85.87%   +0.43%     
==========================================
  Files         198      200       +2     
  Lines        6402     6513     +111     
==========================================
+ Hits         5470     5593     +123     
+ Misses        932      920      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Benchmark Results (Julia v1)

Time benchmarks

	master	7e1ec3e...	master / 7e1ec3e...
clipping/SutherlandHodgman	2.72 ± 0.33 μs	2.69 ± 0.29 μs	1.01 ± 0.16
discretization/simplexify	0.405 ± 0.023 ms	0.401 ± 0.024 ms	1.01 ± 0.083
intersection/ray-triangle	0.05 ± 0.001 μs	0.05 ± 0.01 μs	1 ± 0.2
sideof/ring/large	6.84 ± 0.02 μs	6.78 ± 0.01 μs	1.01 ± 0.0033
sideof/ring/small	0.06 ± 0.001 μs	0.06 ± 0.01 μs	1 ± 0.17
topology/half-edge	2.83 ± 0.26 ms	2.8 ± 0.2 ms	1.01 ± 0.12
winding/mesh	15.5 ± 0.3 ms	15.5 ± 0.11 ms	1 ± 0.021
time_to_load	1.1 ± 0.019 s	1.03 ± 0.0061 s	1.07 ± 0.02

Memory benchmarks

	master	7e1ec3e...	master / 7e1ec3e...
clipping/SutherlandHodgman	0.064 k allocs: 5.55 kB	0.064 k allocs: 5.55 kB	1
discretization/simplexify	0.0362 M allocs: 1.93 MB	0.0362 M allocs: 1.93 MB	1
intersection/ray-triangle	0 allocs: 0 B	0 allocs: 0 B
sideof/ring/large	0 allocs: 0 B	0 allocs: 0 B
sideof/ring/small	0 allocs: 0 B	0 allocs: 0 B
topology/half-edge	25.9 k allocs: 3.17 MB	25.9 k allocs: 3.17 MB	1
winding/mesh	0.0413 M allocs: 3.08 MB	0.0413 M allocs: 3.08 MB	1
time_to_load	0.145 k allocs: 11 kB	0.145 k allocs: 11 kB	1

[juliohm]

@souma4 what is the status of this one? Is it ready for review?

[juliohm]

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?

[souma4]

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

[juliohm]

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.

[souma4]

@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 jarvis.jl and deleting the concave.jl file.

[souma4]

@juliohm This should be good for review now. I refactored the main jarvis algorithm to dispatch on different JarvisMarch{k} methods. if on Nothing it's same as original implementation (which JarvisMarch() creates). If on an integer, then it dispatches the knearest approach. If a valid hull cannot be made at a given k it iterates through k's until a valid one is found. According to the profiler, most time is spent checking if a proposed "next segment" intersects the current hull segments. Without that, it's very easy to get inappropriate early termination.

[juliohm]

Thank you @souma4. I will try to find some time to review the PR over the week.