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: Implement MathOptInterface #11

Closed
wants to merge 53 commits into from

Conversation

tkoolen
Copy link
Collaborator

@tkoolen tkoolen commented Feb 9, 2018

(Based off of #9)

I think I at least have the basics down now. For example, the following works:

using OSQP, JuMP

solver = OSQP.MathOptInterfaceOSQP.OSQPInstance()
m = Model(solver = solver)
@variable(m, x)
@variable(m, y)
@constraint(m, 1 <= x <= 4)
@constraint(m, 2 <= y <= Inf)
@objective(m, Min, x^2 + y^2)
status = solve(m)
@show JuMP.resultvalue(x)
@show JuMP.resultvalue(y)
@show JuMP.objectivevalue(m)

For this you need:

Status:

elseif osqpstatus == :Dual_infeasible
MOI.InfeasibleOrUnbounded
elseif osqpstatus == :Primal_infeasible
MOI.InfeasibleNoResult
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSQP returns proofs of infeasibility, so this should be MOI.Success.

elseif osqpstatus == :Interrupted
MOI.Interrupted
elseif osqpstatus == :Dual_infeasible
MOI.InfeasibleOrUnbounded
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also MOI.Success. The dual status correctly reports MOI.InfeasibilityCertificate.

elseif osqpstatus == :Solved_inaccurate
MOI.AlmostSuccess
elseif osqpstatus == :Primal_infeasible_inaccurate
MOI.InfeasibleNoResult
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AlmostSuccess

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 12, 2018

Thanks for the feedback. I've fixed the TerminationStatuses and adapted to the MOI name changes.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 13, 2018

Woohoo, all of the contlinear and contquadratic tests are now passing (except for linear7, which has vector constraints, and qcqp/socp tests)! See commit messages for today's updates.

@tkoolen tkoolen force-pushed the tk/mathoptinterface branch 3 times, most recently from 460d867 to 4d0e4ce Compare February 14, 2018 15:23
@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #11 into master will increase coverage by 6.45%.
The diff coverage is 93.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
+ Coverage   78.59%   85.05%   +6.45%     
==========================================
  Files           4        5       +1     
  Lines         500      850     +350     
==========================================
+ Hits          393      723     +330     
- Misses        107      127      +20
Impacted Files Coverage Δ
src/OSQP.jl 83.33% <ø> (ø) ⬆️
src/MathOptInterfaceOSQP.jl 93.71% <93.71%> (ø)
src/interface.jl 69.91% <0%> (+0.81%) ⬆️

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 0ae99c6...8626912. Read the comment docs.

@tkoolen tkoolen force-pushed the tk/mathoptinterface branch 7 times, most recently from 6cb7111 to f4331dc Compare February 16, 2018 04:19
@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 16, 2018

I'm getting pretty close now. Problem modification (both objective and constraints) seems to be working, at least for the types of modification covered by MOIT. Let me know if you have any comments.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 18, 2018

Regarding code coverage: seeing as this is meant to be used through MOIU.CachingOptimizer, should I just get rid of the gets and cangets for:

  • MOI.ObjectiveSense
  • MOI.NumberOfVariables
  • MOI.ListOfVariableIndices

as well as isvalid for VariableIndex?

@mlubin
Copy link
Collaborator

mlubin commented Feb 18, 2018

It wasn't the intention of the API to have solvers that are only usable with CachingOptimizer. For example, let's say you read QP models from a text file and then just want to copy! them into a solver and solve. CachingOptimizer introduces an extra level of complexity that isn't needed in that case. The fact that this use case isn't covered in MOI.Test is a bug.

On the other hand, it is the intention for package authors to only implement what they want to support, so 🤷‍♂️.

CC @blegat

@blegat
Copy link
Collaborator

blegat commented Feb 19, 2018

You can use copytest (see e.g. https://github.com/JuliaOpt/MathOptInterface.jl/blob/f411c1ae6d48b61bffed438602c19d6823e1b8f6/test/copy.jl#L8) to test these. You shouldn't need CachingOptimizer for this test

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 19, 2018

Ah, great. Thanks.

@tkoolen tkoolen force-pushed the tk/mathoptinterface branch 2 times, most recently from de5a599 to 1f8ff8d Compare February 19, 2018 23:54
@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 20, 2018

I'm seeing some strange behavior related to problem modification and/or warm starting.

First, I would have expected this test to pass: https://github.com/oxfordcontrol/OSQP.jl/pull/11/files#diff-db1be9f95f6ecf272979beabd83257ddR228. I'm solving the same problem twice and explicitly setting the warm start before each solve, but the results aren't exactly the same. Is there some additional state in OSQP? Could still be a bug in my code as well.

Second, I'm getting random test failures in a test that uses fixed data, namely:

Expression: MOI.get(optimizer, MOI.TerminationStatus()) == MOI.get(cleanoptimizer, MOI.TerminationStatus())
   Evaluated: IterationLimit::MathOptInterface.TerminationStatusCode = 5 == Success::MathOptInterface.TerminationStatusCode = 0
Stacktrace:
 [1] test_optimizer_modification(::##29#34, ::OSQP.MathOptInterfaceOSQP.OSQPModel{Float64}, ::OSQP.MathOptInterfaceOSQP.OSQPOptimizer, ::MathOptInterface.Utilities.IndexMap, ::OSQP.MathOptInterfaceOSQP.OSQPOptimizer, ::MathOptInterface.Test.TestConfig) at /home/twan/code/RigidBodyDynamics/v0.6/OSQP/test/MathOptInterfaceOSQP.jl:151
 [2] macro expansion at /home/twan/code/RigidBodyDynamics/v0.6/OSQP/test/MathOptInterfaceOSQP.jl:244 [inlined]
 [3] macro expansion at ./test.jl:860 [inlined]
 [4] anonymous at ./<missing>:?

Sometimes when I start Julia, the test passes, sometimes I get that error. @bstellato, any ideas?

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 20, 2018

We'll see what Travis says, but locally, not setting check_termination seems to fix the randomness. Maybe I'm completely misunderstanding what that's supposed to do. I was expecting it to be a true/false thing, but now I see e.g. https://github.com/oxfordcontrol/osqp/blob/54345b86a9740a302cd812c43c14a2a5a25569cd/src/osqp.c#L201.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 20, 2018

Test coverage is pretty good now. I think this is pretty much done now.

FYI: I need to work on other stuff the next couple of days.

@bstellato
Copy link
Collaborator

The check_termination parameter is an integer not just bool. It corresponds to the number of iterations between each termination check http://osqp.readthedocs.io/en/latest/interfaces/solver_settings.html If check_termination=0, we never check it.

The solver is not exactly deterministic. This is because we perform the updates of the step-size rho when the osqp_solve time starts to be comparable to osqp_setup time (e.g., a predefined fraction of the setup_time. See the adaptive_rho_fraction parameter). The reason behind is that if we are doing so many first-order iterations that we are getting close to the setup time, we might as well update the step-size rho (performing a new factorization) and continue. After lots of testing on many problems this approach turned out to be the most reliable one. However, since it depends on operating system timings, OSQP might not behave in the exact same way for multiple calls.
The difference should be minimal though. More details are in the paper or the brief paragraph in the documentation.

If you want OSQP to be deterministic, you can set the adaptive_rho_interval parameter. When it is different than 0 it reflects the number of iterations between each rho update without resorting to any time measurement.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Feb 21, 2018

Thanks. Just setting adaptive_rho_interval to a nonzero value (I used 25) didn't make the solver behave deterministically, but setting adaptive_rho to false did. Is there something else that's based on time even if adaptive_rho_interval is nonzero?

@bstellato
Copy link
Collaborator

I am surprised adaptive_rho_interval != 0 makes the solver non deterministic. It should not happen. Maybe there is a bug. Could you please provide a MWE where it happens?

Also, adaptive_rho is really important for having good performance. I think that independently from the time-measured updates, if we set small enough tolerances, the solution should pass the tests. OSQP is a first-order method and at the moment (without implementing acceleration schemes) we cannot expect to have crazy low tolerances (e.g., 1e-16) without significantly increasing the number of iterations.

Another comment: I am not sure you use WarmStartCache every time you update the solver but if we solve several QPs in sequence, OSQP automatically warm starts x and y from the previous solution. Thus, there is no need to call the warm_start! method. However, we have to do it if the user provides an x and/or y.

@tkoolen tkoolen closed this May 1, 2018
bstellato pushed a commit that referenced this pull request May 17, 2018
Add support for vector constraints.
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.

5 participants