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

Update to SCS 3.0 #50

Merged
merged 25 commits into from
Jan 13, 2022
Merged

Update to SCS 3.0 #50

merged 25 commits into from
Jan 13, 2022

Conversation

stephane-caron
Copy link
Member

Discussion at bodono/scs-python#36.

qpsolvers/solvers/scs_.py Outdated Show resolved Hide resolved
@bodono
Copy link

bodono commented Jan 12, 2022

By the way, you can support the lb and ub terms with SCS using the box cone if you want.

@stephane-caron
Copy link
Member Author

As in the original question there bodono/scs-python#36 (comment), I reduced the default feasibility tolerances (same as in the Python example: eps_abs=1e-9 and eps_rel=1e-9) to align SCS's precision with those of the other solvers. Now it solves all problems in the test suite within tolerance.

@stephane-caron
Copy link
Member Author

stephane-caron commented Jan 12, 2022

By the way, you can support the lb and ub terms with SCS using the box cone if you want.

I had forgotten 😊 Yes, let's try this as well. But since there is some extra logic (adding the t variable and the corresponding constraint, you gave the details here) let me do it in a separate PR. This way we can also measure the performance compared to the current implementation.

@bodono
Copy link

bodono commented Jan 12, 2022

Well eps_abs=1e-9 is quite tight for first-order solvers like SCS / OSQP etc. Something like 1e-4 is more appropriate, which is what I see is the default setting for OSQP (in fact OSQP doesn't check that the duality gap is small so might return solutions even looser than 1e-4).

data["b"] = h
cone["l"] = h.shape[0] # positive orthant
else: # no constraint
return sparse.linalg.lsqr(P, -q)[0]
Copy link

@bodono bodono Jan 12, 2022

Choose a reason for hiding this comment

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

The only issue is if q has a component in the null space of P, in which case the problem is unbounded below, since we can set Px = 0 and q'x to be as negative as we want. We can check this using something like:

x_star = sparse.linalg.lsqr(P, -q)[0]
if np.linalg.norm(P @ x_star + q) > 1e-9: 
  # return something to say this problem is unbounded
else:
  return x_star

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! Oh my, actually most functions assume that P is definite 🙈 I've created #51 to make sure this gets covered in the future.

Added this check in 469da9b 👌

@stephane-caron
Copy link
Member Author

stephane-caron commented Jan 12, 2022

Well eps_abs=1e-9 is quite tight for first-order solvers like SCS / OSQP etc. Something like 1e-4 is more appropriate, which is what I see is the default setting for OSQP (in fact OSQP doesn't check that the duality gap is small so might return solutions even looser than 1e-4).

OSQP's feasibility tolerances are indeed at 1e-4, yet its solutions are usually much tigher than 1e-4 in practice, closer to 1e-8 (perhaps its "polishing" helps). Over the problems in the unit test suite, OSQP and the other solvers return solutions that are within 1e-8 of each other (with a few manual exceptions made for CVXOPT, ECOS and SCS which are sometimes 1e-6 or 1e-5 away from the others).

When we use 1e-4 tolerances with SCS, 5 / 12 tests fail. The only test where SCS's solution is significantly away from the others is this one:

  ======================================================================
  FAIL: test_bounds_scs (tests.test_solve_qp.TestSolveQP)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/home/runner/work/qpsolvers/qpsolvers/tests/test_solve_qp.py", line 217, in test
      self.assertLess(norm(x - known_solution), sol_tolerance)
  AssertionError: 0.00013826729950568376 not less than 1e-06   # OSQP is less than 1e-08

(Precisely a test where the box cone would help, so we'll come back to that.) For the other ones, the returned solution is slightly beyond the current tolerance but stays in the same ballpark.

This approach for CI testing of solvers has its drawbacks 😅 and it is rather designed to catch regressions. Here it would make sense to me to increase the tests' sol_tolerance for SCS, and keep SCS's default tolerance settings.

@bodono
Copy link

bodono commented Jan 12, 2022

Ok I see, yes likely the polishing is providing the high tolerances when it succeeds. In that case it would make sense to tighten SCS tolerances to something like 1e-6 or whatever makes the tests pass.

@stephane-caron
Copy link
Member Author

stephane-caron commented Jan 12, 2022

Sounds good. I tried the values below until all tests pass:

eps_* test test_bounds test_one_ineq test_sparse_bounds test_sparse
1e-4 ❌ 1.1e-6 > 1e-6 ❌ 1.3e-4 > 1e-6 ❌ 1.37e-5 > 1e-5 ❌ 3.6e-7 > 1e-8 ❌ 5.8e-8 > 1e-8
1e-5 ❌ 1.1e-6 > 1e-6 ✔️ ❌ 1.37e-5 > 1e-5 ❌ 3.6e-7 > 1e-8 ❌ 5.8e-8 > 1e-8
1e-6 ❌ 1.1e-6 > 1e-6 ✔️ ✔️ ❌ 3.6e-7 > 1e-8 ❌ 5.8e-8 > 1e-8
1e-7 ✔️ ✔️ ✔️ ✔️ ❌ 5.8e-8 > 1e-8
1e-8 ✔️ ✔️ ✔️ ✔️ ❌ 5.8e-8 > 1e-8
1e-9 ✔️ ✔️ ✔️ ✔️ ✔️

It seems we get most of the effect for eps_* = 1e-7.

The last test, test_sparse, differs from the others in that (1) it compares to an exact solution and (2) Gurobi performs so badly on it that I set the tolerance to 1e-8 only because CVXPY (which invokes OSQP) solved it so tight. Since this is a comparison to only one solver, I feel the best is to:

  • Relax the solution tolerance to 1e-7 for this test
  • Set eps_abs=1e-7 and eps_rel=1e-7 by default for SCS

Follows from #50 (comment)

Actually, also increase the inequality tolerance to 1e-7 for this test.
@bodono
Copy link

bodono commented Jan 12, 2022

Ok, looks good!

@stephane-caron
Copy link
Member Author

Thank you very much for your help on this PR 😃

@stephane-caron stephane-caron merged commit d1a5701 into master Jan 13, 2022
@stephane-caron stephane-caron deleted the scs-3.0-for-real branch July 25, 2022 15:56
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.

None yet

2 participants