-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[New solver] DAQP solver initial interface #2312
Conversation
@SteveDiamond @phschiele @PTNobel @rileyjmurray any comments? Also @bstellato should be happy (OSQP is both faster and more robust). Thanks and happy holidays. |
Hey Enzo, I'm out of the country and won't be able to review this till after I'm back. |
For more context here's another little benchmark, on linearly constrained least squares (so there's not proximal step involved). OSQP clearly in the lead. https://gist.github.com/enzbus/426bd635d3dd7df559855c934495c899 |
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.
This looks great to me! Thanks, Enzo! Let's have a another review before we merge, but seems like everything is there.
Thanks! Fixed that, and DAQP's author approves this initial implementation (see darnstrom/daqp#31 ). Thanks and happy new year! |
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.
Overall, this seems like a good interface; thank you Enzo!
I am a bit nervous about adding a "sparse -> dense" step in the solver interface. It feels like using this solver is a big performance footgun, and at the least it should be called out in the docs. Thoughts?
@@ -42,9 +42,10 @@ | |||
from cvxpy.reductions.solvers.conic_solvers.xpress_conif import XPRESS as XPRESS_con | |||
from cvxpy.reductions.solvers.qp_solvers.copt_qpif import COPT as COPT_qp |
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.
Shouldn't this and the line below be moved down below the comment # QP interfaces
too?
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.
I don't remember but I think the import reorder was done by ruff
, is there a logic I was missing?
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.
I think the QP solver interfaces were supposed to be below the comment about # QP solvers
and the conic programs above...
DAQP.VAR_ID: | ||
intf.DEFAULT_INTF.const_to_matrix(np.array(xstar)) | ||
} | ||
# need to skip unused variable bounds constraints |
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.
What are these unused variable bounds constraints? Why are they being passed to the solver?
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.
To my understanding variable bounds are mandatory, this point also the author was particular about and asked whether Cvxpy can't handle those natively (I told him there are people looking into those but it's not there yet).
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.
Ah okay that makes sense.
Agreed, I've done only limited execution time benchmarking but it didn't look to me that the sparse-dense conversion was too costly, worth investigating though. I don't see an alternative, though, other than perhaps factoring it out to be re-used by other interfaces, I think only one other at this point, or writing a new canonicalizer with dense output which is much more work I guess. Edit: let me double check on your comments and add that to the docs. |
For what it's worth, there is a dense canonicalization backend now if you use canon_backend=NUMPY_CANON_BACKEND I don't think it's necessary or desirable to couple the canonicalization backend to the solver, but it might be worth trying this interface with that backend or documenting that the dense backend is recommended. Also, I will post some thoughts about propagating variable bounds in the discord |
Ok! I should have thought of that, will do some more interesting benchmarks then. Also, what's the policy for |
SolverStats.solve_time is always from the solver. Sometimes solvers also break out setup time, in which case you should record it in SolverStats.setup_time. If the solver doesn't break out setup time, no need to record anything. |
@SteveDiamond correct me if I'm wrong, but the NumPy backend will convert to sparse before passing it into the solver interface right? We should probably have a way to skip that conversion if the solver interface wants a dense object. |
That could be right, I'm not sure though. @phschiele @Transurgeon how does the NumPy backend work in this regard? |
That's right, the dense backend has the same (sparse) output format as the other backends. We'd need to have a flag for the output and also update a few transformations that happen after the backend (or at least, verify that all overloaded operators work for sparse and dense matrices). |
Do you guys want to look into amending the numpy canonicalizer to produce a dense problem matrix? It would be cool but out of scope for this solver interface I guess, can always be added later. I haven't had time to benchmark it better yet but I was under the impression the sparse-dense conversion wasn't too costly. As an aside, the solver author mentioned he was interested in |
We can look into that later. This is good to go. |
I did some work on propagating variable upper and lower bounds to the solver: If the general design seems good, it will be easy to integrate with the DAQP interface. |
Hello @Transurgeon, looks like #2309 overwrote the changes to |
Thank you @Transurgeon @SteveDiamond ! PS I encountered the same issue recently (splitting a git-tracked file into many ones breaks automatic merge) and with some digging I found the solution, if you use this script it keeps the correct git history and allows for automatic merge. |
Description
This is a first implementation of an interface to the DAQP active set (dense) QP solver, an open source solver published in 2023. This was requested in issue #2310.
Notes
This is a straightforward implementation of the Python interface described in the documentation. The main catch is that DAQP with its default parameters can't solve QPs whose cost matrix has zero eigenvalues (that includes LPs). It can do so if we change the
eps_prox
option; I implemented the following logic:eps_prox
= 0 which gives best performance (both numerical and execution time)eps_prox=1e-5
; this was obtained by benchmarking with a typical use-case -> https://gist.github.com/enzbus/3d0236c7ed93cff5f0ec3f8587bcd67eType of change
Contribution checklist