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

Use constraints_list when converting from polytope to HPolytope #932

Closed
schillic opened this issue Nov 30, 2018 · 3 comments
Closed

Use constraints_list when converting from polytope to HPolytope #932

schillic opened this issue Nov 30, 2018 · 3 comments
Assignees
Labels
performance 🐎 More efficient code refactoring 🔧 Internal code changes, typically no impact on API

Comments

@schillic
Copy link
Member

Currently we have this implementation:

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

It would be better to not use the V-representation. Every AbstractPolytope provides the constraints_list method now.

There is one problem with Zonotope, however, where the constraints_list method falls back to convert. So we first need #416.

@schillic schillic added performance 🐎 More efficient code refactoring 🔧 Internal code changes, typically no impact on API labels Nov 30, 2018
@mforets
Copy link
Member

mforets commented Feb 13, 2019

Since #416 is now done, shall we reconsider this issue? The fallback implementation to convert from an abstract polytope to an HPolytope is still the one quoted above (see this function).

I think that

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

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

can be unified into

function convert(T::Union{Type{HPolytope}, Type{HPolyhedron}}, P::AbstractPolytope;
                 use_constraint_representation=true)
    if use_constraint_representation
        return T(constraints_list(P))
    else
        return convert(T, convert(VPolytope, P))
    end
end

@schillic
Copy link
Member Author

No need for the flag. To create these two types, you always need a list of constraints. So this implementation only makes sense for types that do not support constraints_list, but they cannot be AbstractPolytopes then.

@mforets
Copy link
Member

mforets commented Feb 14, 2019

True, thanks for the correction. Either the inner call convert(VPolytope, P) or the outer one convert(T, ...) needs to call a constraints_list under the hood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance 🐎 More efficient code refactoring 🔧 Internal code changes, typically no impact on API
Projects
None yet
Development

No branches or pull requests

2 participants