-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add functions to find optimal partitions using ILP solvers. #3
base: master
Are you sure you want to change the base?
Conversation
such as Karmarkar-Karp and Greedy. | ||
""" | ||
|
||
from numberpartitioning import karmarkar_karp, greedy, complete_greedy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be useful if the examples here were included as docstrings on the relevant functions or modules. This way, they would be easy to include in auto-generated documentation, and it would be possible to run them as tests locally and in CI.
E.g. something like the example sections in the methods in https://github.com/scipy/scipy/blob/master/scipy/sparse/csgraph/_flow.pyx works well.
|
||
import cvxpy | ||
|
||
DEFAULT_SOLVERS=[cvxpy.XPRESS, cvxpy.OSQP, cvxpy.SCS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this require that the relevant binaries are available on the user's machine? And at least one of them requires a license for large problem instances.
Allowing to use these solvers is fine, but it would then be nice if all relevant dependencies were available through pypi or conda, so it's possible to use the library in a self-contained fashion.
Those would then be specifies as optional dependencies for the Python package.
except cvxpy.SolverError as err: | ||
logger.info("Solver %s fails: %s", solver, err) | ||
if not is_solved: | ||
problem.solve(solver=solvers[-1]) # If the first n-1 fail, try the last one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it should be possible to treat the final solver alongside the other ones. Any reason it's handled specifically?
|
||
Returns | ||
------- | ||
A partition representing by a ``PartitioningResult``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this always runs to optimality right? Given the NP-hard nature of the problem, that might be a bit of an ask. Would it be a lot of work to use callbacks to generate solutions along the way, or maybe make it easy to allow it to return non-optimak results (through time constraints, allowed gap, etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses an existing ILP solver, so - as far as I know - it cannot return solutions along the way. On the positive side, ILP solvers use many tricks that make the solution very efficient in practice.
Also added a "demo" folder with some simple demo programs.