-
Notifications
You must be signed in to change notification settings - Fork 24
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
WIP: First version of Polyhedral with QPDAS solver #76
base: master
Are you sure you want to change the base?
Conversation
u[upper_and_lower] ] | ||
|
||
IndPolyhedralQPDAS{R}(Aeq, beq, Cieq, dieq) | ||
end |
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 function is quite nasty, but I don't see another way to support the old syntax otherwise.
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.
It’s okay, I don’t see many other way of doing it.
I’m open to changing the overall syntax for the construction of IndPolyhedral
objects. But maybe we can open a dedicated issue for that and focus here in meeting the current construction signature.
Didn't mean to push the benchmark, will remove those. |
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.
Thank you! Benchmark results are impressive, good job with the solver!
Could you also update IndPolyhedral
as part of this, and add the option for using this variant? Also, the docstring for IndPolyhedral
could be updated with the solver argument and a brief description of the available options for it. I believe I didn’t do this before since OSQP was the only option there.
QPDAS is already on its way to be registered?
IndPolyhedralQPDAS(A, C, b, d) | ||
|
||
```math | ||
S = \\{x : \\langle A, x \\rangle = b\\ ∧ \\langle C, x \\rangle \\le b\\}. |
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 would write Ax and Cx without angular parentheses (we use those everywhere else for inner products)
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.
You're right, I just copied or from somewhere and forgot to change that.
function (f::IndPolyhedralQPDAS{R})(x::AbstractVector{R}) where R | ||
Ax = f.A * x | ||
Cx = f.C * x | ||
return all(Ax .<= f.b .& Cx .<= f.d) ? R(0) : Inf |
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.
Is it Ax .== f.b
here? Or rather isapprox.(Ax, f.b)
.
If this was a bug and no test failed, maybe a test is 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.
Bug, will fix and add test.
u[upper_and_lower] ] | ||
|
||
IndPolyhedralQPDAS{R}(Aeq, beq, Cieq, dieq) | ||
end |
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.
It’s okay, I don’t see many other way of doing it.
I’m open to changing the overall syntax for the construction of IndPolyhedral
objects. But maybe we can open a dedicated issue for that and focus here in meeting the current construction signature.
This should already be in the PR, or did you mean something else?
Will do
I opened it to the public yesterday, wanted to test it against osqp here first. Will probably request it to be registered this weekend. I will also post some more benchmarks, it seems like it is consistently 2x to 3000x faster than osqp for a wide variety of projection problems. So we should probably considering to make it the new dafault, maybe through some deprecation? (Trying to not be too partial here) |
Yes sorry, it’s already in this PR, I was probably checking in master by mistake. I’m open to changing the default, let’s maybe look into a more extensive comparison for this type of projections, including ill-conditioned problems where some solvers may have a hard time. By the way, are you aware of any other open source QP solvers with Julia bindings (or pure Julia ones, even better)? Because when I was looking into this, I could only find OSQP, which was also very young back then. |
I completely agree, I'm currently running tests for large problems (its currently at 10_000 variables, 2000 equality, 2000inequality, osqp takes 2hours, qpdas 20min (doing 10 consecutive proxes on similar points). In this case I think the dual is quite ill-conditioned, do you have some good tips for generating ill-conditioned problems?
Not really, at least not active-set solvers. qpOASES has similar performance as QPDAS, but it is in C++, and I was unable to write a working wrapper since it keeps variables in memory internally. |
QPDAS is still not registered so the tests will fail (but pass locally).
I ran a simple benchmark with vectors of length 1000, 50 equality, and 50 inequality constraints and no constraints on the vector itself. The benchmark results, running https://gist.github.com/mfalt/465969600b27a4ec285ab67c2530d374
are:
Note that performance is worse when adding upper and lower constraints on the vector itself, I haven't tested how it compares to osqp in this case yet.