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

WIP: First version of Polyhedral with QPDAS solver #76

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

mfalt
Copy link
Collaborator

@mfalt mfalt commented Mar 27, 2019

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:

Setup OSQP
  0.015556 seconds (95 allocations: 2.365 MiB)
Setup QPDAS
  0.000734 seconds (128 allocations: 1.539 MiB)
First prox OSQP:
  0.698441 seconds (16 allocations: 25.938 KiB)
First prox QPDAS:
  0.000935 seconds (1.19 k allocations: 336.773 KiB)
100 prox! OSQP:
 68.826197 seconds (1.60 k allocations: 3.297 MiB)
100 prox! QPDAS:
  0.420764 seconds (591.76 k allocations: 38.232 MiB, 0.64% gc time)

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.

@mfalt mfalt requested a review from lostella March 27, 2019 19:56
u[upper_and_lower] ]

IndPolyhedralQPDAS{R}(Aeq, beq, Cieq, dieq)
end
Copy link
Collaborator Author

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.

Copy link
Member

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.

@mfalt
Copy link
Collaborator Author

mfalt commented Mar 27, 2019

Didn't mean to push the benchmark, will remove those.
This PR solves part of #52 (again)

Copy link
Member

@lostella lostella left a 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\\}.
Copy link
Member

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)

Copy link
Collaborator Author

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
Copy link
Member

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

Copy link
Collaborator Author

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
Copy link
Member

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.

@mfalt
Copy link
Collaborator Author

mfalt commented Mar 28, 2019

Could you also update IndPolyhedral as part of this, and add the option for using this variant?

This should already be in the PR, or did you mean something else?

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.

Will do

QPDAS is already on its way to be registered?

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)

@lostella
Copy link
Member

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.

@mfalt
Copy link
Collaborator Author

mfalt commented Mar 29, 2019

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.

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?

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.

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.

@mfalt
Copy link
Collaborator Author

mfalt commented Apr 4, 2019

The benchmarks for random problems with bounds either as lb <= Ax <= ub, (denoted u+l) and Ax <= ub (u) are as follows on my computer, for n=100 to 10000 variables:
osqp_qpdas_benchmark_random

where the ratios of the timings:
osqp_qpdas_benchmark_ratio

Note that QPDAS is faster in almost all cases, except when the number of variables is 5000 or 10000, and there are 2500 and 5000 upper and lower constraints respectively. However, osqp takes 2.2 and 11 hours to do init + 11proxes, and qpdas 0.5 and 8 hours in these cases, so I am not sure it is relevant for this package.

I will try to generate more tests for ill-conditioned problems when I have time.

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