tkoolen commented 6 years ago

I know MathOptInterface is coming soon, but I want to try this with JuMP right now.

No tests yet, but most things seem to be working. Demo code:

using OSQP, JuMP

solver = OSQPSolver()
MathProgBase.setparameters!(solver; Silent = true)
m = Model(solver = solver)
@variable(m, x)
@variable(m, y)
@constraint(m, 1 <= x <= 4)
@constraint(m, y >= 3)
@objective(m, Min, x^2 + y^2)
status = solve(m)
@show getvalue(x)
@show getvalue(y)
@show getobjectivevalue(m)

For ease of implementation, I decided to store copies of P, q, etc. in OSQPModel and call OSQP.setup! in MathProgBase.optimize!. None of the updating infrastructure is currently used. Still, it's a start.

mlubin commented 6 years ago

Looks reasonable. You should run the relevant functions in MathProgBase/test/quadprog.jl as part of the unit tests.

But I'd add that the important MOI infrastructure for QPs is now in place in JuMP, and we could use some early testers.

bstellato commented 6 years ago

Thank you for this. I guess the next steps are to:

Complete the remaining functions that are compatible with OSQP
Implement the problem data updates properly (this gives OSQP the real speedups by the way). I think this can be done by
- keeping track of the problem data as an internal copy of P, q, A, l, u
- calling the osqp function updates whenever possible, e.g., when setobj!(m, c) is called we call OSQP.update!(model, q=c)
- having a boolean flag to define whether a new setup is required when optimize! is called. This is useful when funcitons like addvar! are called.
Add the quadprog tests as in Gurobi.jl

tkoolen commented 6 years ago

You should run the relevant functions in MathProgBase/test/quadprog.jl as part of the unit tests.

Since OSQP doesn't support quadratic constraints, neither quadprogtest nor qpdualtest is really applicable. There's also the additional issue of lack of support for variable bounds (do you agree with my handling of that by the way, or should they automatically be turned into constraints?). I opted to just copy over the relevant parts of quadprogtest for now. The dual-related functionality should still be tested somehow though.

codecov-io commented 6 years ago

Codecov Report

Merging #9 into master will increase coverage by 8.22%. The diff coverage is 89.35%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
+ Coverage   71.69%   79.91%   +8.22%     
==========================================
  Files           3        4       +1     
  Lines         272      488     +216     
==========================================
+ Hits          195      390     +195     
- Misses         77       98      +21

Impacted Files	Coverage Δ
src/OSQP.jl	`83.33% <ø> (ø)`	:arrow_up:
src/interface.jl	`69.54% <ø> (+0.82%)`	:arrow_up:
src/mpbinterface.jl	`89.35% <89.35%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eb1ea2d...5094552. Read the comment docs.

mlubin commented 6 years ago

There's also the additional issue of lack of support for variable bounds (do you agree with my handling of that by the way, or should they automatically be turned into constraints?).

Depends on whether you want to call OSQP on JuMP models that have variables with bounds.

bstellato commented 6 years ago

Since OSQP doesn't support quadratic constraints, neither quadprogtest nor qpdualtest is really applicable. There's also the additional issue of lack of support for variable bounds (do you agree with my handling of that by the way, or should they automatically be turned into constraints?). I opted to just copy over the relevant parts of quadprogtest for now. The dual-related functionality should still be tested somehow though.

I think people use variable bounds a lot. We could keep track of them in the problem data as

    lb <= Ax <= ub
    l <= x <= u

Then, when we call optimize! we stack them together. I can add it later today or tomorrow.

tkoolen commented 6 years ago

Re: https://github.com/oxfordcontrol/OSQP.jl/pull/9#issuecomment-363144570: I started doing that initially, but I got a little worried that the interpretation of the dual variables would become confusing/nonstandard.

We also need better handling of sense. For now, I've restricted sense to be :Min in my latest commit in order to at least avoid blatantly wrong behavior. I'm not sure what the JuMP/MathProgBase convention is for handling duals when sense == :Max, so guidance would be appreciated. Also, the fact that loadproblem! contains the sense but not the 'P' matrix would have made subsequent calls to setquadobj! do the wrong thing for sense == :Max.

mlubin commented 6 years ago

For now, I've restricted sense to be :Min in my latest commit in order to at least avoid blatantly wrong behavior. I'm not sure what the JuMP/MathProgBase convention is for handling duals when sense == :Max, so guidance would be appreciated.

JuMP has lots of tests for duals on LPs with both min/max sense. Not sure if there are specific tests for QPs. You should really feel free to only implement the parts that you need, because all of this is going to be tossed out after JuMP 0.18. Rejecting Max problems is reasonable.

tkoolen commented 6 years ago

Improved test coverage. Also discovered and fixed a minor problem with OSQP.warm_start! along the way.

tkoolen commented 6 years ago

Don't merge yet; I suspect there's something wrong with the quadratic objective methods.

bstellato commented 6 years ago

Thanks for adding these tests. Tomorrow I will merge my changes as well to complete the interface.

tkoolen commented 6 years ago

Fixed the objective bug and added yet more tests that displayed the problem. I'm successfully using OSQP through JuMP for my intended application now.

bstellato commented 6 years ago

Damn, I pushed before finishing some stuff!!! Sorry. I had to rush out to finish some other things. I wanted to rebase everything before pushing it. I will adjust those things tomorrow ok? (Have lots of IKEA stuff to put together...).

Thanks anyway for the performance tips. I am not totally familiar with Julia performance guidelines yet. I haven't used it that much until very recently.

bstellato commented 6 years ago

I have fixed the interface. It should now support incremental model building, infeasibility certificates, Max or Min problems and problem data updates. There was a problem with the dual variables sign before. I have removed a couple of unnecessary allocations but I am quite sure we could do something better memory-wise. However, we might just leave it as it is and focus on performance improvements for the MathOptInterface. Let me know what you think.

tkoolen commented 6 years ago

Looks good to me!

osqp / OSQP.jl

Add MathProgBase solver interface. #9

Codecov Report