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

#416 - Conversion from Zonotope to HPolytope #958

Merged
merged 13 commits into from
Jan 17, 2019
Merged

#416 - Conversion from Zonotope to HPolytope #958

merged 13 commits into from
Jan 17, 2019

Conversation

schillic
Copy link
Member

Closes #416.

src/convert.jl Outdated Show resolved Hide resolved
src/convert.jl Outdated Show resolved Hide resolved
@mforets
Copy link
Member

mforets commented Jan 12, 2019

This efficient conversion is a great feature to have, i look forward to reviewing it.

@schillic
Copy link
Member Author

I think it is far from efficient, but the problem is just hard.

@schillic
Copy link
Member Author

I realized that I added the convert method. What I wanted to add was the constraints_list method 😢

@schillic schillic force-pushed the schillic/416 branch 2 times, most recently from a4753e4 to 3cf7e59 Compare January 16, 2019 07:38
@schillic
Copy link
Member Author

I just renamed the variable name for StrictlyIncreasingIndices from ip to sii (I think I first had a different name for the type and then forgot to adapt the variable name).

src/Zonotope.jl Outdated Show resolved Hide resolved
src/helper_functions.jl Outdated Show resolved Hide resolved
src/helper_functions.jl Outdated Show resolved Hide resolved
src/helper_functions.jl Outdated Show resolved Hide resolved
mforets and others added 4 commits January 16, 2019 14:38
Co-Authored-By: schillic <schillic@informatik.uni-freiburg.de>
Co-Authored-By: schillic <schillic@informatik.uni-freiburg.de>
Co-Authored-By: schillic <schillic@informatik.uni-freiburg.de>
@mforets
Copy link
Member

mforets commented Jan 16, 2019

I realized that I added the convert method. What I wanted to add was the constraints_list method

i think that the convert(HPolytope, ::Zonotope) method should be added as well, because it is a pity that it falls back to the dual representation given that constraints_list can make use of this PR.

EDIT:

Actually, convert methods for HPolytope and HPolyhedron are very different:

# uses dual rep
function convert(::Type{HPolytope}, P::AbstractPolytope)
    return convert(HPolytope, convert(VPolytope, P))
end

# may or may not use dual rep depending on the implementation of constraints_list
function convert(::Type{HPolyhedron}, P::AbstractPolytope)
    return HPolyhedron(constraints_list(P))
end

Let me note that this would also make sense:

function convert(::Type{HPolytope}, P::AbstractPolytope)
    return HPolytope(constraints_list(P))
end

I don't know for sure which implementation we should pick. Maybe it is better to have both implementations available through a kwarg. (I tend to think that the implem that calls the constraints_list should be the default one).

Since this is a bit out of the scope of a Zonotpe -> HPolytope conversion, i suggest that we tackle this is a separate issue. I've created #1017.

src/Zonotope.jl Outdated Show resolved Hide resolved
src/Zonotope.jl Outdated Show resolved Hide resolved
src/Zonotope.jl Outdated Show resolved Hide resolved
Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

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

I finished reviewing it all, well done! 👏
It won't be hard to re-use this code with parallelotopes.

As suggested below, while we are at this let's fix the case m=n before merging. I also left three ideas to make it faster.

test/unit_Zonotope.jl Outdated Show resolved Hide resolved
@schillic
Copy link
Member Author

i think that the convert(HPolytope, ::Zonotope) method should be added as well, because it is a pity that it falls back to the dual representation given that constraints_list can make use of this PR.

No need for that. In #932 we want to use constraints_list by default.

@schillic schillic merged commit 50526fa into master Jan 17, 2019
@schillic schillic deleted the schillic/416 branch January 17, 2019 12:32
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