search

tulip-control / polytope

Geometric operations on polytopes of any dimension
https://pypi.org/project/polytope
Other
73 stars 19 forks source link

Extract as new module `solvers`, and support MOSEK #50

Closed johnyf closed 6 years ago

johnyf commented 6 years ago

Addresses:

  1. A bug described at: https://github.com/tulip-control/polytope/issues/17#issuecomment-335316011.
  2. Separation of concerns, by hiding implementation details of interfaces in their own module, called solvers.
  3. Merges the branch mosek.
slivingston commented 6 years ago

I shall finish reviewing later today.

This PR has been open for more than one month, but there has been no negative comment about changes to the API. Backwards compatibility only seems to be affected for special uses of the package, and I guess that the number of affected (broken API) implementations is small or zero.

slivingston commented 6 years ago

Currently I do not have access to MOSEK, but the support of it is via cvxopt, and the use of cvxopt continues to be correct. It might be interesting to compare performance between the cvxopt wrapper of MOSEK and the MOSEK Fusion API.

necozay commented 6 years ago

My experience from another project: using MOSEK API directly is much faster than cvxopt (but we were solving fewer and larger optimization problems for the other project).

slivingston commented 6 years ago

Because there is now a module named solvers in the polytope package, it can be confusing to also use from cvxopt import solvers in solvers.py.

Because it only concerns internal naming style within solvers.py, this comment is not yet critical. I recommend to eventually change to import cvxopt.solvers, instead of solvers. As it is, the name resolution for solvers in cvxopt vs. in polytope is unambiguous, but if someone is skimming the code, it might not be clear to them.

johnyf commented 6 years ago

Another possibility is from cvxopt import solvers as cvx_solvers: slightly shorter (in this case small difference, would matter more to obtain shorter lines for a deeper import, e.g., networkx.drawing.nx_pydot).

johnyf commented 6 years ago

The import issue observed above has been addressed in 765aded0bca2313682395a25ccfadfed09f1a74e.