Fast Singular Value Thresholding

[paquiteau]

This PR adds a Singular Value Threshold Operator. This does exactly the same that LowRankMatrix but faster (roughly 10 times, it depends on the threshold value, notebook and data available upon request).

Basically instead of computing all the singular value, and perform the thresholding, we only compute the one above the threshold, which save a lot of computation.

I left the legacy version for now, but this should act as a drop in replacement.

[sfarrens]

@paquiteau This is fantastic, great work! 🎉

Below are my thoughts on this PR. Let me know what you think. 🙂

1. I am not sure if we should add redundancy to the code by having to proximity operators that do the same thing, especially as they have different names it might not be obvious to the users that LowRankMatrix and SingularValueThreshold do the same thing. What I would propose instead is that we add your improvements to LowRankMatrix as something like _op_method_new and change the existing method to _op_method_old, then you can simply add a parameter to __init__ to choose which method to use with a default value (e.g. op_method='old'). We can set the default value to 'old' to avoid any potential problems downstream, but add a deprecation warning to _op_method_old noting that in a follow up release we will switch the default to the new method.
2. I would also propose to add your improved thresholding method as function to modopt.signal.svd and then import this function to modopt.opt.proximity where it can be called by _op_method_new.
3. I have added few other comments to the code for more specific points.

[paquiteau]

I made the requested changes.
I extracted the computation in its own function, and added test to hopefully increase coverage.

[paquiteau]

well I don't understand the only error of the test suite:

/home/runner/work/ModOpt/ModOpt/modopt/signal/svd.py:1:1: DAR000: Unexpected exception in darglint: Multiple types should be combined into a Union

[sfarrens]

well I don't understand the only error of the test suite:

/home/runner/work/ModOpt/ModOpt/modopt/signal/svd.py:1:1: DAR000: Unexpected exception in darglint: Multiple types should be combined into a Union

Umm, that's weird. It seems like some other people are getting this too, but I have not found a solution. I suggest you update line 61 of setup.cfg with the following:

 modopt/signal/svd.py: WPS345, DAR000
 #Todo: Check security of using system executable call
 #Todo: Investigate source of darglint error

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (develop@736e067). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop     #209   +/-   ##
==========================================
  Coverage           ?   93.13%           
==========================================
  Files              ?       36           
  Lines              ?     2475           
  Branches           ?        0           
==========================================
  Hits               ?     2305           
  Misses             ?      170           
  Partials           ?        0           

Flag	Coverage Δ
unittests	93.13% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

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 736e067...ca5524a. Read the comment docs.

[sfarrens]

@paquiteau Everything looks good now. There is just some very minor clean up left to do and then we can merge this PR. 🎉