ENH: Add lapack driver argument to randomized svd function #20617

skailasa · 2021-07-27T13:38:56Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This PR adds the parameter to specify the Lapack driver for the internal SVD taken over the matrix B, which is the projection of the matrix being compressed in a low-dimensional subspace (see Halko et al).

This is useful in numerical applications where the defualt 'gesdd' does not offer enough precision.

Any other comments?

This function should also support double precision. However, this should be the subject of a further PR.

glemaitre

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

You need to add an additional test to check that we can indeed pass the new parameter as expected.

sklearn/utils/extmath.py

ogrisel

Thanks for the PR. Could you please add a small unit test for this option?

sklearn/utils/extmath.py

skailasa · 2021-07-28T08:37:37Z

I added the test however left the quotation marks as single rather than double around variable names as suggested above in order to match the rest of this function/module.

glemaitre · 2021-07-28T09:34:46Z

quotation marks as single rather than double around variable names

I would expect black to fail then since the standard is double-quoted.
FYI: https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=30899&view=logs&jobId=32e2e1bb-a28f-5b18-6cfc-3f01273f5609&j=32e2e1bb-a28f-5b18-6cfc-3f01273f5609&t=fc67071d-c3d4-58b8-d38e-cafc0d3c731a

You just probably want to apply black on the two files and that's it.

doc/whats_new/v1.0.rst

sklearn/utils/extmath.py

sklearn/utils/tests/test_extmath.py

glemaitre · 2021-09-06T14:37:45Z

@skailasa would you be able to complete your PR?

skailasa · 2021-09-07T10:51:18Z

@skailasa would you be able to complete your PR?

Hi, yes sorry about dropping the ball on this. Been held up elsewhere/been on my holidays. WIll complete this today!

ogrisel

LGTM.

sklearn/utils/tests/test_extmath.py

jjerphan

This is indeed nice enhancement.

jjerphan · 2021-10-05T15:34:35Z

sklearn/utils/extmath.py

+        Whether to use the more efficient divide-and-conquer approach
+        (`"gesdd"`) or more general rectangular approach (`"gesvd"`) to compute
+        the SVD of the matrix B, which is the projection of M into a low
+        dimensional subspace, as described by Halko et. al.


as described by Halko et. al.

Can you please add a reference to the mention article?

I added a reference to the function's docstring, as it's mentioned multiple times during the parameter documentation.

Actually, I just checked and a reference is already in the function.

That's right. In that case, can you adapt the doc-string to add a Sphinx cross-reference similarly to this?

scikit-learn/sklearn/cluster/_optics.py

Lines 203 to 211 in 16419d1

References

----------

.. [1] Ankerst, Mihael, Markus M. Breunig, Hans-Peter Kriegel,

and Jörg Sander. "OPTICS: ordering points to identify the clustering

structure." ACM SIGMOD Record 28, no. 2 (1999): 49-60.

.. [2] Schubert, Erich, Michael Gertz.

"Improving the Cluster Structure Extracted from OPTICS Plots." Proc. of

the Conference "Lernen, Wissen, Daten, Analysen" (LWDA) (2018): 318-329.

jjerphan · 2021-10-05T15:34:35Z

sklearn/utils/tests/test_extmath.py

+def test_randomized_svd_lapack_driver():
+    # Check that different SVD drivers provide consistent results
+
+    # Matrix being compressed
+    n = 123
+    m = 456
+    k = 10
+    rng = np.random.RandomState(0)
+    X = rng.rand(n, m)
+
+    # Number of components
+    k = 10


How about parametrizing this test on a few combinations of n, m, k and on the random seed?

For instance:

Suggested change

def test_randomized_svd_lapack_driver():

# Check that different SVD drivers provide consistent results

# Matrix being compressed

n = 123

m = 456

k = 10

rng = np.random.RandomState(0)

X = rng.rand(n, m)

# Number of components

k = 10

@pytest.mark.parametrize("n", [50, 100, 300])

@pytest.mark.parametrize("m", [50, 100, 300])

@pytest.mark.parametrize("k", [10, 20, 50])

@pytest.mark.parametrize("seed", range(5))

def test_randomized_svd_lapack_driver(n, m, k, seed):

# Check that different SVD drivers provide consistent results

# Matrix being compressed

rng = np.random.RandomState(seed)

X = rng.rand(n, m)

I adapted my suggestion which was shadowing the parametrized value.

(It looks like you have not committed all the changes or pushed them.)

jjerphan

LGTM, after removing the last shadowing.

jjerphan · 2021-10-06T09:00:47Z

sklearn/utils/tests/test_extmath.py

+    X = rng.rand(n, m)
+
+    # Number of components
+    k = 10


I think this originates from the mistake I made when suggesting and this has to be removed.

Suggested change

k = 10

thomasjpfan

Thank you for the PR @skailasa !

thomasjpfan · 2021-10-08T21:57:54Z

sklearn/utils/extmath.py

+        dimensional subspace, as described in [1]_.
+


Suggested change

dimensional subspace, as described in [1]_.

dimensional subspace, as described in [1]_.

.. versionadded:: 1.1

thomasjpfan · 2021-10-08T21:57:54Z

doc/whats_new/v1.0.rst

@@ -913,6 +913,10 @@ Changelog
  these functions were not documented and part from the public API.
  :pr:`20521` by :user:`Olivier Grisel <ogrisel>`.

+- |Enhancement| :func:`utils.extmath.randomized_svd` now accepts an argument


This needs to move into 1.1

thomasjpfan · 2021-10-08T21:57:54Z

sklearn/utils/extmath.py

@@ -332,6 +333,12 @@ def randomized_svd(
            the value of `random_state` explicitly to suppress the deprecation
            warning.

+    lapack_svd_driver : {"gesdd", "gesvd"}, default="gesdd"


Nit: Do you think svd_lapack_driver is better here? It keeps the term "lapack_driver" together, which makes it a bit easier to search for when looking for "lapack_driver" in the scipy docs.

cmarmo · 2022-05-10T22:22:30Z

@skailasa you already have two approvals. Do you mind fixing conflicts and the last comment? Then this can probably be merged. Thanks!

glemaitre

LGTM. I just applied the suggestions of @thomasjpfan.
I will be merging when the CI turns green.

glemaitre · 2022-05-17T10:26:44Z

Thanks @skailasa

…rn#20617) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Add lapack driver

c3e4260

github-actions bot added the module:utils label Jul 27, 2021

skailasa changed the title ~~Add lapack driver~~ ENH: Add lapack driver argument to randomized svd function Jul 27, 2021

glemaitre reviewed Jul 27, 2021

View changes

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

ogrisel reviewed Jul 28, 2021

View changes

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

Respond to PR comments

b176aaf

Fix test

41d41ae

glemaitre reviewed Jul 28, 2021

View changes

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved

glemaitre reviewed Jul 28, 2021

View changes

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

glemaitre reviewed Jul 28, 2021

View changes

sklearn/utils/tests/test_extmath.py Outdated Show resolved Hide resolved

skailasa added 6 commits Sep 7, 2021

Respond to comments

855c55b

Merge branch 'main' into main

f1ad411

Remove unused imports

d11317e

Fix typo in test

ddfdf2d

Remove warning in tests

f9ec008

Fix tests

34950e5

ogrisel approved these changes Sep 27, 2021

View changes

sklearn/utils/tests/test_extmath.py Outdated Show resolved Hide resolved

Add random seed

f587360

jjerphan requested changes Oct 5, 2021

View changes

skailasa added 2 commits Oct 6, 2021

Respond to PR comments

f1a1b45

Add sphinx xref

3729d5c

jjerphan approved these changes Oct 6, 2021

View changes

Remove last shadow

9bd6deb

thomasjpfan reviewed Oct 8, 2021

View changes

glemaitre self-assigned this May 17, 2022

glemaitre added 3 commits May 17, 2022

Merge remote-tracking branch 'origin/main' into pr/skailasa/20617-1

cec0dd2

DOC move entry what's new

078abbc

REV adding last nitpicks

1fa2575

glemaitre approved these changes May 17, 2022

View changes

glemaitre merged commit 0c618b2 into scikit-learn:main May 17, 2022
28 checks passed

lesteve pushed a commit to lesteve/scikit-learn that referenced this pull request May 19, 2022

ENH Add lapack driver argument to randomized svd function (scikit-lea…

16adc8f

…rn#20617) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

mathijs02 pushed a commit to mathijs02/scikit-learn that referenced this pull request Dec 27, 2022

ENH Add lapack driver argument to randomized svd function (scikit-lea…

1e4da32

…rn#20617) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

ENH: Add lapack driver argument to randomized svd function #20617

ENH: Add lapack driver argument to randomized svd function #20617

skailasa commented Jul 27, 2021

glemaitre left a comment

ogrisel left a comment

skailasa commented Jul 28, 2021 •

edited

glemaitre commented Jul 28, 2021

glemaitre commented Sep 6, 2021

skailasa commented Sep 7, 2021

ogrisel left a comment

jjerphan left a comment

jjerphan Oct 5, 2021

skailasa Oct 6, 2021

skailasa Oct 6, 2021

jjerphan Oct 6, 2021

jjerphan Oct 5, 2021 •

edited

skailasa Oct 6, 2021

jjerphan Oct 6, 2021 •

edited

jjerphan left a comment

jjerphan Oct 6, 2021

thomasjpfan left a comment

thomasjpfan Oct 8, 2021

thomasjpfan Oct 8, 2021

thomasjpfan Oct 8, 2021

cmarmo commented May 10, 2022

glemaitre left a comment

glemaitre commented May 17, 2022

	References
	----------
	.. [1] Ankerst, Mihael, Markus M. Breunig, Hans-Peter Kriegel,
	and Jörg Sander. "OPTICS: ordering points to identify the clustering
	structure." ACM SIGMOD Record 28, no. 2 (1999): 49-60.

	.. [2] Schubert, Erich, Michael Gertz.
	"Improving the Cluster Structure Extracted from OPTICS Plots." Proc. of
	the Conference "Lernen, Wissen, Daten, Analysen" (LWDA) (2018): 318-329.

ENH: Add lapack driver argument to randomized svd function #20617

ENH: Add lapack driver argument to randomized svd function #20617

Conversation

skailasa commented Jul 27, 2021

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

glemaitre left a comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

skailasa commented Jul 28, 2021 • edited

glemaitre commented Jul 28, 2021

glemaitre commented Sep 6, 2021

skailasa commented Sep 7, 2021

ogrisel left a comment

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjerphan Oct 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjerphan Oct 6, 2021 • edited

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmarmo commented May 10, 2022

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented May 17, 2022

skailasa commented Jul 28, 2021 •

edited

jjerphan Oct 5, 2021 •

edited

jjerphan Oct 6, 2021 •

edited