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

fix: scipy time limit raises SolverError #2080

Merged
merged 3 commits into from Mar 31, 2023

Conversation

allenlawrence94
Copy link
Contributor

@allenlawrence94 allenlawrence94 commented Mar 27, 2023

Description

When using SciPy, if the solver reaches it's set time limit, a SolverError is raised. This change makes it so these instances are instead reported as OPTIMAL_INACCURATE. I think this behavior is more consistent with other solvers - this is at least how GLPK works currently.

I haven't added a unit test for this. Let me know if this seems necessary.

Type of change

  • New feature (backwards compatible)
  • New feature (breaking API changes)
  • Bug fix
  • Other (Documentation, CI, ...)

Contribution checklist

  • Add our license to new files.
  • Check that your code adheres to our coding style.
  • Write unittests.
  • Run the unittests and check that they’re passing.
  • Run the benchmarks to make sure your change doesn’t introduce a regression.

@github-actions
Copy link

github-actions bot commented Mar 27, 2023

Benchmarks that have stayed the same:

   before           after         ratio
 [a9e2a0e8]       [887c0895]
      52.0±0s          54.4±0s     1.05  sdp_segfault_1132_benchmark.SDPSegfault1132Benchmark.time_compile_problem
      34.5±0s          35.0±0s     1.02  cvar_benchmark.CVaRBenchmark.time_compile_problem
      4.73±0s          4.80±0s     1.02  simple_LP_benchmarks.SimpleFullyParametrizedLPBenchmark.time_compile_problem
      10.3±0s          10.4±0s     1.01  semidefinite_programming.SemidefiniteProgramming.time_compile_problem
      27.8±0s          28.1±0s     1.01  simple_LP_benchmarks.SimpleLPBenchmark.time_compile_problem
      13.2±0s          13.3±0s     1.00  svm_l1_regularization.SVMWithL1Regularization.time_compile_problem
      2.10±0s          2.10±0s     1.00  simple_LP_benchmarks.SimpleScalarParametrizedLPBenchmark.time_compile_problem
      11.0±0s          10.8±0s     0.98  huber_regression.HuberRegression.time_compile_problem
      10.3±0s          9.70±0s     0.94  optimal_advertising.OptimalAdvertising.time_compile_problem

@SteveDiamond
Copy link
Collaborator

Thanks for making this PR! You should make sure a solution is actually present when the iteration limit/time limit is reached. We made a similar change to the SCIP interface, which caused a different bug (see #2050). This is a bit hard to unit test but if you could try I'd appreciate it (in test_conic_solvers.py).

@allenlawrence94
Copy link
Contributor Author

allenlawrence94 commented Mar 27, 2023

Will do! Apologies for not searching issues first Just realized that SCIP != SciPy

@allenlawrence94
Copy link
Contributor Author

@SteveDiamond You were right to be concerned! It seems that when a time limit is reached, scipy will only sometimes return a solution. I was only able to get "Time limit reached" with a solution present when solving a MI problem with presolve=True (which is the default).

The unit test I added may be flaky since it uses a time limit, but, unfortunately, SciPy/HiGHS has no iteration limit for MI problems. If you're concerned about this, we could drop the "run without enough time to find optimum" part.

@SteveDiamond
Copy link
Collaborator

Since the "run without enough time to find optimum" test passes on all CI platforms I think it's probably ok. @rileyjmurray @phschiele what do you think?

@SteveDiamond SteveDiamond mentioned this pull request Mar 29, 2023
9 tasks
@SteveDiamond SteveDiamond merged commit 2ed1c48 into cvxpy:master Mar 31, 2023
50 checks passed
@phschiele phschiele mentioned this pull request Jun 22, 2023
Paulnkk pushed a commit to Paulnkk/cvxpy that referenced this pull request Jul 15, 2023
@Paulnkk Paulnkk mentioned this pull request Jul 15, 2023
Paulnkk pushed a commit to Paulnkk/cvxpy that referenced this pull request Aug 1, 2023
* fix: scipy time limit raises SolverError

* fix: only allow optimal_inaccurate from scipy if solution is present

* chore: lint fix
@Paulnkk Paulnkk mentioned this pull request Aug 1, 2023
SteveDiamond added a commit that referenced this pull request Aug 7, 2023
* Introduce ruff linter (#2049)

* Fix scip timelimit no solution (#2084)

* Add condition timelimit and no solution

* Add timelimit example

* Add unit test timelimit and no solution

* attempt

* Update timelimit

* try to fix windows

* remove failing test

---------

Co-authored-by: Steven Diamond <steven@gridmatic.com>

* Adds devcontainer support (#2088)

* Adds minimal devcontainer setup

* Adds docs

* Fix conv and add convolve (#2047)

* make conv more like np.convolve

* more tests

* make backwards compatible

* add convolve, deprecate conv

* added convolve, deprecated conv

* update docs

* Cherry-pick #2080

* Fix scip timelimit no solution (#2084)

* Add condition timelimit and no solution

* Add timelimit example

* Add unit test timelimit and no solution

* attempt

* Update timelimit

* try to fix windows

* remove failing test

---------

Co-authored-by: Steven Diamond <steven@gridmatic.com>

* Fixes to PowCone3D and updates to test_conic_solvers.py (#2131)

Update solver_test_helpers.py to include a cone program with SDP and power cone constraints.

Correct a bug in PowCone3D.residual, which should have been declared as a property but was not.

---------

Co-authored-by: Steven Diamond <steven@gridmatic.com>

* Revert "Fixes to PowCone3D and updates to test_conic_solvers.py (#2131)"

This reverts commit e68445e.

* Fix rare bugs in curvature calculation: non-psd diagonal matrices and starting vectors in eigsh (#2086)

* explicitly generate the starting vector in our calls to eigsh; generate the vector based on a fixed random stream.

* Add special casing for diagonal matrices

---------

Co-authored-by: Riley Murray <rileyjmurray@users.noreply.github.com>

* Fix bug with calling Mosek simplex solver (#2126)

* got new mosek simplex test passing

* revert clarabel intf change

* CBC interface: add ability to configure the underlying CLP solver (#1821)

* update CBC our interface (which passes through cylp) so that the user can configure the underlying CLP solver.

* write tests to verify that changing CLP tolerance settings will change CLP's behavior predictably.

* write tests to verify that changing logLevel affects the amount of material logged to console. (This was hard!)

* remove deprecated old test code

* replace TestCBC's use of unittest.skipUnless with appropriately modified pytest.mark.skipif.

---------

Co-authored-by: Riley Murray <rileyjmurray@users.noreply.github.com>

* remove github banner (#2160)

* Update scipy_wrapper (#2159)

* update spwrapper

* Update cvxpy/interface/scipy_wrapper.py

Co-authored-by: Isaac Virshup <ivirshup@gmail.com>

---------

Co-authored-by: Philipp Schiele <44360364+phschiele@users.noreply.github.com>
Co-authored-by: Isaac Virshup <ivirshup@gmail.com>

* Fixes to PowCone3D and updates to test_conic_solvers.py (#2131)

Update solver_test_helpers.py to include a cone program with SDP and power cone constraints.

Correct a bug in PowCone3D.residual, which should have been declared as a property but was not.

---------

Co-authored-by: Steven Diamond <steven@gridmatic.com>

* Explicit import of  power cone 3d

* Only port bug fix for mosek

* Only port bug fix for mosek

* Only port bug fix for mosek

---------

Co-authored-by: Philipp Schiele <44360364+phschiele@users.noreply.github.com>
Co-authored-by: Steven Diamond <steven@gridmatic.com>
Co-authored-by: Steven Diamond <diamond@cs.stanford.edu>
Co-authored-by: Allen Lawrence <allenlawrence94@gmail.com>
Co-authored-by: Riskfolio <dany.cajas.n@uni.pe>
Co-authored-by: Riley Murray <rileyjmurray@users.noreply.github.com>
Co-authored-by: bkurtz <bkurtz@users.noreply.github.com>
Co-authored-by: Isaac Virshup <ivirshup@gmail.com>
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