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

Implement Polyhedra interface #3

Merged
merged 2 commits into from
Feb 11, 2021
Merged

Implement Polyhedra interface #3

merged 2 commits into from
Feb 11, 2021

Conversation

blegat
Copy link
Member

@blegat blegat commented Feb 4, 2021

  • Removes Manifest.toml from Git. It seems to me that the common practice is not to check it out.
  • Implement the Polyhedra.VRepresentation for POI and Polyhedra.HRepresentation for IEQ.
  • Create a Library <: Polyhedra.Library and Polyhedron <: Polyhedra.Polyhedron tht uses traf for the double description.

@blegat blegat marked this pull request as ready for review February 5, 2021 12:57
@blegat blegat force-pushed the bl/polyhedron branch 2 times, most recently from c77ddbf to 9df5cd9 Compare February 5, 2021 15:33
@bdoolittle
Copy link
Member

@blegat Just wondering what the reasoning behind adding these changes to XPORTA.jl is? Naively I'd assume that XPORTA.jl would be independent from Polyhedron.jl while Polyhedron.jl uses XPORTA.jl as a dependency.

@blegat
Copy link
Member Author

blegat commented Feb 11, 2021

If Polyhedra has a dependency on every library, it would make it hard to install it and would make the ecosystem distributed. Here, the philosophy is that libraries can be created independently. This is similar to the JuMP ecosystem where we have an interface MathOptInterface and each solver have a package implementing both the Julia interface and MathOptInterface, see https://jump.dev/JuMP.jl/dev/installation/#Supported-solvers.
The only exception is Mosek which has Mosek.jl for the Julia interface and MosekTools.jl for the MathOptInterface.
For XPORTA, the advantage to have the Polyhedra interface in the XPORTA.jl package is that we can make the POI and IEQ types subtypes of Polyhedra.Representation so that we can directly use these representations without needing to copy the data into a structure implementing the Polyhedra.Representation interface.

@bdoolittle
Copy link
Member

okay, that's fine. The tests are failing due to GLPK not being found in the testing environment. Should be an easy enough fix. I'll see if I can get it figured out this morning. I'll merge the pull request once the automated tests are passing

@blegat
Copy link
Member Author

blegat commented Feb 11, 2021

Oops, I had a fix that I forgot to push, should be ok now

@bdoolittle
Copy link
Member

We are now failing because there is a dependency on JuMP deep in the polyhedra tests.

@bdoolittle
Copy link
Member

@blegat I had to resolve a few more changes, but things should be good now. Thanks for the help!

I do want to make a counterpoint to the architectural decision here about how to handle solver dependencies. I'll preface this with "I don't know what the right answer is, but there are some drawbacks of what we've done."

First, calling in the tests from Polyhedron.jl is a slick way to test these changes, but it also hides code and dependencies in a different repository from the one being tested. Hence, test failures in Polyhedron.jl can propagate and break the tests of XPORTA.jl which is not ideal.

Second, it adds a lot of overhead to testing XPORTA.jl for functionality that XPORTA.jl doesn't need responsible for.

This might just be a tradeoff between the overhead of testing XPORTA.jl versus the overhead of using Polyhedron.jl.
Perhaps a future solution might be to have a lightweight PolyhedronInterfaceBase.jl package that serves as a backbone for the types used for the solvers without having to worry about the rest of what Polyhedron.jl does.

This is just some food for thought. For now, things work so no need to change them right away.

-Cheers

@blegat
Copy link
Member Author

blegat commented Feb 11, 2021

First, calling in the tests from Polyhedron.jl is a slick way to test these changes, but it also hides code and dependencies in a different repository from the one being tested. Hence, test failures in Polyhedron.jl can propagate and break the tests of XPORTA.jl which is not ideal.

Indeed. From another point of view, it might add coverage to the rest of XPORTA.jl. In CDDLib and LRSLib, I have already found bugs in the Julia wrappers thanks to the polyhedron tests.

Perhaps a future solution might be to have a lightweight PolyhedronInterfaceBase.jl package that serves as a backbone for the types used for the solvers without having to worry about the rest of what Polyhedron.jl does.

That's been my dream too for some time and it's similar to the split between MathOptInterface and JuMP but it's trickier for Polyhedra and I couldn't yet figure out a nice way.

@bdoolittle bdoolittle merged commit 9677e0e into master Feb 11, 2021
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