DEPR Change default value of random_state in SVD #19370

cliffordEmmanuel · 2021-02-06T09:01:58Z

Reference Issues/PRs

References #18584

What does this implement/fix? Explain your changes.

Fixes the default value of random state in randomized_svd and provides a deprecation warning should the value not be provided.

Any other comments?

@cinbez provided assistance

reshamas · 2021-02-06T09:04:02Z

#DataUmbrella sprint

adrinjalali

The error in the CI is related to your change. You can see the logs and see where it fails and fix that in the same PR.

sklearn/utils/extmath.py

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:.

ogrisel · 2021-02-06T09:56:18Z

I think we should review tests that are calling randomized_svd without passing an explicit value for random_state to update them (and pass random_state=0 or random_state=42 explicitly).

I believe there is a few of such cases in test_extmath.py:

git grep "randomized_svd(" | grep test_

adrinjalali · 2021-02-06T10:59:46Z

sklearn/utils/extmath.py

+        During the deprecation cycle this will set to zero and afterwards
+        it will be set to None.


Suggested change

During the deprecation cycle this will set to zero and afterwards

it will be set to None.

The previous behavior (`random_state=0`) is deprecated, and

from v1.2 the default value will be `random_state=None`. Set

the value of `random_state` explicitly to suppress the deprecation

warning.

WDYT @glemaitre @ogrisel

adrinjalali · 2021-02-06T11:06:46Z

You have linter issues (some lines are too long now)

ogrisel

Let's be even more explicit in the deprecation message:

ogrisel · 2021-02-06T11:11:05Z

sklearn/utils/extmath.py

+            "it will be set to 0 starting from version 1.2 "
+            "Set 'random_state' to an integer value to silence "
+            "this warning, "
+            "or to 0 to keep the behavior of versions <1.0 ",


I think we should rather say:

"If random_state is not supplied, the current default is to use 0 as a fixed seed. This will change to None in version 1.2 leading to non-deterministic results that better reflect the nature of the randomized_svd solver. If you want to silence this warning, set random_state to an integer seed or to None explicitly depending if you want your code to be deterministic or not."

Sure, thanks very much

lorentzenchr · 2021-02-06T15:44:22Z

The "ci/circleci: lint" tells you that there are a few line with more than 79 characters in the file sklearn/utils/tests/test_extmath.py: lines 253, 261, 311, 315, 405.

As stated in the developer guide, you might want to run

git diff upstream/main -u -- "*.py" | flake8 --diff

in order to detect PEP8 violations before pushing.

If you do this more often, it can help to use a pre-commit hook (also described in the developer guide) with

pip install pre-commit
pre-commit install

inside your scikit-learn directory.

sklearn/utils/extmath.py

ogrisel

LGTM once the remaining comments above are taken care of!

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

cliffordEmmanuel · 2021-02-07T05:50:49Z

LGTM once the remaining comments above are taken care of!

Hi please pardon my newbie question but what does LGTM mean?

cliffordEmmanuel · 2021-02-07T05:53:51Z

The "ci/circleci: lint" tells you that there are a few line with more than 79 characters in the file sklearn/utils/tests/test_extmath.py: lines 253, 261, 311, 315, 405.

As stated in the developer guide, you might want to run
git diff upstream/main -u -- "*.py" | flake8 --diff
in order to detect PEP8 violations before pushing.

If you do this more often, it can help to use a pre-commit hook (also described in the developer guide) with
pip install pre-commit
pre-commit install
inside your scikit-learn directory.

I really appreciate this suggestion, could you kindly show me how to navigate to error lines, I was trying to use the details but it quickly became overwhelming and I couldn't find the error lines.
Thanks

lorentzenchr · 2021-02-07T08:56:45Z

LGTM = looks good to me

lorentzenchr · 2021-02-07T09:05:22Z

I really appreciate this suggestion, could you kindly show me how to navigate to error lines, I was trying to use the details but it quickly became overwhelming and I couldn't find the error lines.

How to navigate to error lines, but where? Do you mean in the CI (continuous integration)? I scroll until I find something. Often, searching for "error" or "failure" works.

lorentzenchr · 2021-02-07T09:08:15Z

doc/whats_new/v1.0.rst

+<<<<<<< HEAD
+=======
+
+>>>>>>> c8456e71c15c36ed47e094263d7d0e0c57bd8065


Suggested change

<<<<<<< HEAD

=======

>>>>>>> c8456e71c15c36ed47e094263d7d0e0c57bd8065

I guess, those are the remnant from resolving a merge conflict.

lorentzenchr · 2021-02-07T09:09:20Z

doc/whats_new/v1.0.rst

+<<<<<<< HEAD
+=======
+
+>>>>>>> c8456e71c15c36ed47e094263d7d0e0c57bd8065


Suggested change

<<<<<<< HEAD

=======

>>>>>>> c8456e71c15c36ed47e094263d7d0e0c57bd8065

lorentzenchr · 2021-02-07T09:09:48Z

doc/whats_new/v1.0.rst

+<<<<<<< HEAD
+=======
+
+>>>>>>> c8456e71c15c36ed47e094263d7d0e0c57bd8065


Suggested change

<<<<<<< HEAD

=======

>>>>>>> c8456e71c15c36ed47e094263d7d0e0c57bd8065

reshamas · 2021-02-15T17:54:47Z

Hello @cliffordEmmanuel @cinbez
How is this PR going? Do you have any questions on how to move this PR forward?

cinbez · 2021-02-16T07:45:07Z

HI @reshamas - I have tried to get hold of @cliffordEmmanuel but with no luck. I have opened my own pull request solving this same issue here. Waiting for it to be approved.

cliffordEmmanuel added 3 commits February 6, 2021 08:12

changed the default parameter to None for random_svd

3631f0f

added deprecation warning for random_state

4d058ec

fixed whitespace issues

db6017e

github-actions bot added the module:utils label Feb 6, 2021

adrinjalali reviewed Feb 6, 2021

View reviewed changes

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

glemaitre changed the title ~~Random svd parameter change~~ DEPR Change default value of random_state in SVD Feb 6, 2021

glemaitre reviewed Feb 6, 2021

View reviewed changes

cliffordEmmanuel added 2 commits February 6, 2021 10:33

supplied the random_state default value for all tests

2512046

updated the docstring to reflect the warning changes

5da5aad

adrinjalali reviewed Feb 6, 2021

View reviewed changes

added contribution in whats_new doc

5b439db

updated docstring

7580404

ogrisel reviewed Feb 6, 2021

View reviewed changes

cliffordEmmanuel and others added 4 commits February 6, 2021 11:12

fixed trailing whitespace

edb80f8

updated warning message

cf720ac

fixed trailing whitespaces

8dfeb53

Merge branch 'main' into random_svd_parameter_change

f611719

ogrisel reviewed Feb 6, 2021

View reviewed changes

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

ogrisel reviewed Feb 6, 2021

View reviewed changes

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

ogrisel approved these changes Feb 6, 2021

View reviewed changes

cliffordEmmanuel and others added 5 commits February 7, 2021 05:06

Update sklearn/utils/extmath.py

c8456e7

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

updated docstring

22ce37b

updated docstring

5bbd2fd

resolved conflicts

9e4b02b

fixed linting issues

11dcd43

lorentzenchr reviewed Feb 7, 2021

View reviewed changes

cinbez mentioned this pull request Feb 18, 2021

Random svd parameter change #19459

Closed

rth mentioned this pull request Mar 15, 2021

ENH Deprecated the default random_state=0 in randomized_svd #19670

Merged

rth closed this in #19670 Mar 15, 2021

cliffordEmmanuel deleted the random_svd_parameter_change branch March 22, 2021 05:47

		During the deprecation cycle this will set to zero and afterwards
		it will be set to None.

-        During the deprecation cycle this will set to zero and afterwards
-        it will be set to None.
+        The previous behavior (`random_state=0`) is deprecated, and
+        from v1.2 the default value will be `random_state=None`. Set
+        the value of `random_state` explicitly to suppress the deprecation
+        warning.

Uh oh!

DEPR Change default value of random_state in SVD #19370

DEPR Change default value of random_state in SVD #19370

Uh oh!

Conversation

cliffordEmmanuel commented Feb 6, 2021

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

reshamas commented Feb 6, 2021

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 6, 2021

Uh oh!

adrinjalali Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

ogrisel Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Feb 6, 2021

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

cliffordEmmanuel Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

lorentzenchr commented Feb 6, 2021

Uh oh!

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

cliffordEmmanuel commented Feb 7, 2021

Uh oh!

cliffordEmmanuel commented Feb 7, 2021

Uh oh!

lorentzenchr commented Feb 7, 2021

Uh oh!

lorentzenchr commented Feb 7, 2021

Uh oh!

lorentzenchr Feb 7, 2021

Choose a reason for hiding this comment

Uh oh!

lorentzenchr Feb 7, 2021

Choose a reason for hiding this comment

Uh oh!

lorentzenchr Feb 7, 2021

Choose a reason for hiding this comment

Uh oh!

reshamas commented Feb 15, 2021

Uh oh!

cinbez commented Feb 16, 2021

Uh oh!

Uh oh!