Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#5 - Implement hrep from vrep for VPolygon #376

Merged
merged 15 commits into from
Jul 26, 2018
Merged

#5 - Implement hrep from vrep for VPolygon #376

merged 15 commits into from
Jul 26, 2018

Conversation

schillic
Copy link
Member

@schillic schillic commented Jul 8, 2018

Closes #5.

  • docs
  • tests
  • manual handling of 2-vertices case (the current solution is unbounded)

@schillic schillic requested a review from mforets July 8, 2018 20:34
@schillic
Copy link
Member Author

schillic commented Jul 8, 2018

using LazySets
points_all = [[0.9,0.2], [0.4,0.6], [0.2,0.1], [0.1,0.3], [0.3,0.28]]
for i in [0, 1, 2, 5]
    points = i == 0 ? Vector{Vector{Float64}}() : points_all[1:i]
    V = VPolygon(points, apply_convex_hull=i > 0)
    H1 = tohrep(V)
    H2 = HPolygon{Float64}() # check that constraints are sorted correctly
    for c in H1.constraints
        addconstraint!(H2, c)
    end
    println(V)
    println(H1)
    println(H1.constraints == H2.constraints)
    println()
end

Output:

LazySets.VPolygon{Float64}(Array{Float64,1}[])
LazySets.HPolygon{Float64}(LazySets.HalfSpace{Float64}[])
true

LazySets.VPolygon{Float64}(Array{Float64,1}[[0.9, 0.2]])
LazySets.HPolygon{Float64}(LazySets.HalfSpace{Float64}[LazySets.HalfSpace{Float64}([1.0, 1.0], 1.1), LazySets.HalfSpace{Float64}([-1.0, 0.0], -0.9), LazySets.HalfSpace{Float64}([0.0, -1.0], -0.2)])
true

LazySets.VPolygon{Float64}(Array{Float64,1}[[0.9, 0.2], [0.4, 0.6]])
LazySets.HPolygon{Float64}(LazySets.HalfSpace{Float64}[LazySets.HalfSpace{Float64}([0.4, 0.5], 0.46), LazySets.HalfSpace{Float64}([-0.5, 0.4], 0.04), LazySets.HalfSpace{Float64}([-0.4, -0.5], -0.46), LazySets.HalfSpace{Float64}([0.5, -0.4], 0.37)])
true

LazySets.VPolygon{Float64}(Array{Float64,1}[[0.1, 0.3], [0.2, 0.1], [0.9, 0.2], [0.4, 0.6]])
LazySets.HPolygon{Float64}(LazySets.HalfSpace{Float64}[LazySets.HalfSpace{Float64}([0.4, 0.5], 0.46), LazySets.HalfSpace{Float64}([-0.3, 0.3], 0.06), LazySets.HalfSpace{Float64}([-0.2, -0.1], -0.05), LazySets.HalfSpace{Float64}([0.1, -0.7], -0.05)])
true

@mforets
Copy link
Member

mforets commented Jul 10, 2018

Before I add the tests, what do you think?

i didn't check the details yet, but i would suppose that we want to exploit that fact that a VPolygon has its vertices ordered. Hence we can go through the vertices in, say, counter clockwise-order and for each pair of vertices add the corresponding constraint.

@schillic
Copy link
Member Author

i would suppose that we want to exploit that fact that a VPolygon has its vertices ordered

Yes, I do that. Even better, I start with the vertex pair that corresponds to the first constraint in the resulting HPolygon. This avoids additional sorting.

@schillic schillic changed the title [WIP #5] Implement hrep from vrep for VPolygon #5 - Implement hrep from vrep for VPolygon Jul 11, 2018
@schillic
Copy link
Member Author

Thanks for the documentation.

we can equally choose q instead of p

No, that is why it is called halfspace_left 😉

src/HalfSpace.jl Outdated
"""
function halfspace_left(p::AbstractVector{N},
q::AbstractVector{N})::HalfSpace{N} where {N<:Real}
@assert p != q "the points must not be equal"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this notion of "left", i think, only makes sense in 2D, but HalfSpace is general so i think we should make this check here and throw an error otherwise. i can add it and see if you agree.

Copy link
Member Author

@schillic schillic Jul 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. EDIT: Forget it, this was silly.
  2. Do you think we should also add a version from a LineSegment (which is just the one-liner which unwraps the two vertices and calls this function here)?
  3. Should we move the function to AbstractHPolygon instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should also add a version from a LineSegment (which is just the one-liner which unwraps the two vertices and calls this function here)?

sounds good, i'll do in the next push.

also and while we are at this, add halfspace_right.

Should we move the function to AbstractHPolygon instead?

i think that in HalfSpace.jl is fine.

@mforets
Copy link
Member

mforets commented Jul 20, 2018

No, that is why it is called halfspace_left

My formulation was not the best, my point being that for choosing b (not a) one can pick either p or q, since dot(p, a) = dot(q, a) for the given a.

@mforets
Copy link
Member

mforets commented Jul 20, 2018

That said, i think that it looks better without that part.

line_dir = -line_dir
addconstraint!(H, LinearConstraint(line_dir, dot(L.p, line_dir)))
return H
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done!

@mforets
Copy link
Member

mforets commented Jul 20, 2018

I wonder how does this tohrep function compare to the (for me simpler):

(EDIT)

constraints_list = vcat([halfspace_left(P.vertices_list[i], P.vertices_list[i+1]) for i in 1:length(P.vertices_list)-1],
                        [halfspace_left(P.vertices_list[end], P.vertices_list[1])])
return HPolygon(constraints_list)

### Examples

The left half-space of the "east" and "west" directions in two-dimensions are the
upper and lower half-spaces:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good!

@schillic
Copy link
Member Author

I wonder how does this tohrep function compare to the (for me simpler):

constraints_list = vcat([halfspace_left(P.vertices_list[i], P.vertices_list[i+1]) for i in 1:length(P.vertices_list)-1],
                       [halfspace_left(P.vertices_list[end], P.vertices_list[1])])
return HPolygon(constraints_list)

Good thinking, but you need to start at the index that was found before (i in my code). Basically it should do what you did but with three lists and different start index. Maybe like this (not tested):

vl = P.vertices_list
constraints_list = vcat(
    [halfspace_left(vl[j], vl[j+1]) for j in i:length(vl)-1],
    [halfspace_left(vl[end], vl[1])],
    [halfspace_left(vl[j], vl[j+1]) for j in 1:i-1])
return HPolygon(constraints_list)

My usual way of thinking is to write the code myself, but I prefer your version. Do you want to add it?

@mforets
Copy link
Member

mforets commented Jul 21, 2018

My usual way of thinking is to write the code myself, but I prefer your version. Do you want to add it?

ok. it took me a while to understand what was wrong with my version 😄

@schillic
Copy link
Member Author

I just realized that this PR conflicts with #386. We should merge only one of them (the order does not matter) and then rebase and replace the missing vertices_list.

Copy link
Member Author

@schillic schillic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

function halfspace_right(p::AbstractVector{N},
q::AbstractVector{N})::HalfSpace{N} where {N<:Real}
return halfspace_right(q, p)
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a line


halfspace_left(L::LineSegment) = halfspace_left(L.p, L.q)

halfspace_right(L::LineSegment) = halfspace_right(L.p, L.q)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a line

@schillic
Copy link
Member Author

schillic commented Jul 22, 2018

The build fail with Julia v0.6 is the same as in #386. It could also be a recent bug in IntervalArithmetic.jl that only occurs with v6.0?
EDIT: Could be this... but then why does it work with v0.6.1? Below they also announce that v0.6 support will vanish with the next release, so maybe they already broke it... 😟

@mforets
Copy link
Member

mforets commented Jul 22, 2018

The build fail with Julia v0.6 is the same as in #386. It could also be a recent bug in IntervalArithmetic.jl that only occurs with v6.0?

Hmm in my machine the tests pass (v0.6.1) and

julia> Pkg.installed("IntervalArithmetic")
v"0.13.0+"

@mforets
Copy link
Member

mforets commented Jul 22, 2018

Hmm we can always pick the version that we use in REQUIRE, such as v0.13.

Sooner or later we should also switch to 0.7 .. 1.0

@schillic
Copy link
Member Author

Here (v0.6.2) they pass as well (I also updated the package). Note that the Travis builds for v0.6.1 and v0.6.1 pass; only v0.6 fails. The thing is that as soon as we merge, the green badge on the front page will become red 😞

@schillic
Copy link
Member Author

schillic commented Jul 25, 2018

Note to myself: I want to do this before merging.
EDIT: done

@schillic schillic merged commit 46b8d58 into master Jul 26, 2018
@schillic schillic deleted the schillic/5 branch July 26, 2018 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants