Update scipy_wrapper

[SteveDiamond]

Description

Updates the scipy_wrapper as suggested in scipy/scipy#18749
Issue link (if applicable): #2158

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]

Benchmarks that have stayed the same:

   before           after         ratio
 [8f3e2b95]       [e9d68aa0]
      21.7±0s          21.9±0s     1.01  simple_LP_benchmarks.SimpleLPBenchmark.time_compile_problem
      10.2±0s          10.3±0s     1.01  svm_l1_regularization.SVMWithL1Regularization.time_compile_problem
      28.0±0s          28.1±0s     1.00  cvar_benchmark.CVaRBenchmark.time_compile_problem
      8.54±0s          8.54±0s     1.00  semidefinite_programming.SemidefiniteProgramming.time_compile_problem
      3.86±0s          3.84±0s     1.00  simple_LP_benchmarks.SimpleFullyParametrizedLPBenchmark.time_compile_problem
      7.77±0s          7.72±0s     0.99  optimal_advertising.OptimalAdvertising.time_compile_problem
      8.81±0s          8.72±0s     0.99  huber_regression.HuberRegression.time_compile_problem
      42.1±0s          41.5±0s     0.99  sdp_segfault_1132_benchmark.SDPSegfault1132Benchmark.time_compile_problem
      1.72±0s          1.68±0s     0.98  simple_LP_benchmarks.SimpleScalarParametrizedLPBenchmark.time_compile_problem

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[phschiele]

LGTM. Let me summarize the understanding that I've gathered as of now (this might be obvious to some readers).

1. Returning NotImplemented from a binary operation (instead of raising NotImplementedError) is a signal that a certain operation is not implemented, but a fallback may be available (e.g., if a.__eq__(b) is not implemented, b.__eq__(a) might still be available). Only if all recovery attempts fail, an exception is being raised.
2. scipy_wrapper ensures that all binary operations fall back to the cvxpy implementation when other is a cvxpy expression.
3. This might not always be necessary, though, as SciPy may already provide this signal in their implementation, e.g., here.

[PTNobel]

1. Returning NotImplemented from a binary operation (instead of raising NotImplementedError) is a signal that a certain operation is not implemented, but a fallback may be available (e.g., if a.eq(b) is not implemented, b.eq(a) might still be available). Only if all recovery attempts fail, an exception is being raised.

Correct.

3. This might not always be necessary, though, as SciPy may already provide this signal in their implementation, e.g., here.

Do we know how recent that code was? I wonder if we even need this hotfix nowadays...It was added in 2013 per git blame, and it seems this originated as: f8b8e7e#diff-5b5c7742b5ff35de17876c5a718a7f6f1aeb774037c2e73d3c39bb7e937607c3
which doesn't seem relevant to this file...

In any case, I wonder if there is still a need. Especially on the __div__, __add__, __sub__, __mul__ arguments.

Testing with

class C:
    pass
    
getattr(sp.dok_matrix(np.zeros((5, 5))), func)(C())

only errored for __ge__, __le__, __lt__, __gt__ and returned NotImplemented for the rest.

[phschiele]

1. Returning NotImplemented from a binary operation (instead of raising NotImplementedError) is a signal that a certain operation is not implemented, but a fallback may be available (e.g., if a.eq(b) is not implemented, b.eq(a) might still be available). Only if all recovery attempts fail, an exception is being raised.

Correct.

3. This might not always be necessary, though, as SciPy may already provide this signal in their implementation, e.g., here.

Do we know how recent that code was? I wonder if we even need this hotfix nowadays...It was added in 2013 per git blame, and it seems this originated as: f8b8e7e#diff-5b5c7742b5ff35de17876c5a718a7f6f1aeb774037c2e73d3c39bb7e937607c3 which doesn't seem relevant to this file...

In any case, I wonder if there is still a need. Especially on the __div__, __add__, __sub__, __mul__ arguments.

Testing with

class C:
    pass
    
getattr(sp.dok_matrix(np.zeros((5, 5))), func)(C())

only errored for __ge__, __le__, __lt__, __gt__ and returned NotImplemented for the rest.

Good point. Indeed, I have checked that, based on our tests, we do not need any patch with recent SciPy versions. However, because I neither know that our tests cover all cases, nor checked with older SciPy versions, I was hesitant to suggest removing the patching entirely.

[PTNobel]

However, because I neither know that our tests cover all cases, nor checked with older SciPy versions, I was hesitant to suggest removing the patching entirely.

Could we deprecate the patch in 1.4 and see if it is still needed? We could raise a warning and asks people to contact us if they hit it.

[rileyjmurray]

@PTNobel I'd support deprecating in 1.4.