#1384 - Add four point case for convex hull

[SebastianGuadalupe]

Closes #1384.

[schillic]

There is a problem with the sorting of vertices (hence the error):

julia> using LazySets
julia> B = Ball1(zeros(2), 1.);

# this branch:
julia> P = convert(VPolygon, B)
VPolygon{Float64,Array{Float64,1}}(Array{Float64,1}[[0.0, -1.0], [-1.0, 0.0], [0.0, 1.0], [1.0, 0.0]])

# master:
julia> P = convert(VPolygon, B)
VPolygon{Float64,Array{Float64,1}}(Array{Float64,1}[[-1.0, 0.0], [0.0, -1.0], [1.0, 0.0], [0.0, 1.0]])

Please add a test case that detects this.

[schillic]

EDIT: This was fixed in #1441. Now you need to rebase (sorry).

---

LazySets.ConvexHull: Test Failed at /home/travis/build/JuliaReach/LazySets.jl/test/unit_ConvexHull.jl:105
  Expression: convex_hull!(points, algorithm="")
    Expected: ErrorException
  No exception thrown

This error is a bug in our tests:

LazySets.jl/test/unit_ConvexHull.jl

Lines 103 to 105 in a5545ae

	@test convex_hull!(points, algorithm="monotone_chain") == sorted
	@test convex_hull!(points, algorithm="monotone_chain_sorted") == sorted
	@test_throws ErrorException convex_hull!(points, algorithm="")

The tests should instead either use the non-! function or use copy(points), e.g.:

    @test ispermutation(convex_hull!(copy(points), algorithm="monotone_chain"), sorted)
    @test ispermutation(convex_hull!(copy(points), algorithm="monotone_chain_sorted"), sorted)
    @test_throws ErrorException convex_hull!(copy(points), algorithm="")

[schillic]

I wonder why your tests did not catch the first error. Can you add a test for that case?

[schillic]

Adding the output type forces Julia to convert the result, which means it does not actually return the original points if the input was not a Vector.

[schillic]

Suggested change

	function _collinear_case!(points::Vector{VN}, A::VN, B::VN, C::VN, D::VN)::Vector{VN} where {N<:Real, VN<:AbstractVector{N}}
	function _collinear_case!(points::Vector{VN}, A::VN, B::VN, C::VN, D::VN) where {N<:Real, VN<:AbstractVector{N}}

[mforets]

Note that the outer signature is already typed to be a Vector (in and out), what is kept arbitrary is the representation of each point:

function convex_hull!(points::Vector{VN};
                      algorithm=default_convex_hull_algorithm(points),
                      backend=nothing)::Vector{VN} where {N<:Real, VN<:AbstractVector{N}}
...

[schillic]

True, and in full generality, each element could have a different concrete type. The most general version would thus be:

function _collinear_case!(points::AbstractVector{<:AbstractVector{N}}, A::AbstractVector{N},
    B::AbstractVector{N}, C::AbstractVector{N}, D::AbstractVector{N}) where {N<:Real}

I think if you do not use this version, the method will crash if you use, e.g., SparseVectors.

[schillic]

Do you need the type annotations or can you just remove them?

[mforets]

After discussion on gitter, @SebastianGuadalupe would you please remove type annotations in _collinear_case!?

[schillic]

And then squash 👍

[mforets]

After discussion on gitter, @SebastianGuadalupe would you please remove type annotations in _collinear_case!?

_collinear_case!(points, A, B, C, D)

[mforets]

LGTM, great job!

---

For reference,

julia> B = Ball1(zeros(2), 1.0);

julia> points = vertices_list(B);

# master v1.13.0
julia> @btime convex_hull!($(copy(points)))
 403.095 ns (7 allocations: 432 bytes)

# this branch
julia> @btime convex_hull!($(copy(points)))
28.775 ns (0 allocations: 0 bytes)

[schillic]

LGTM (after addressing the open comment and squashing).

[mforets]

after addressing the open comment

i'll handle it, plus some other minor tweaks in the same file that i planned to do, in a follow-up PR.

squashing

done (from github).