Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

ENH Deprecated the default random_state=0 in randomized_svd #19670

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

Merged
merged 11 commits into from
Mar 15, 2021

Conversation

cliffordEmmanuel
Copy link
Contributor

@cliffordEmmanuel cliffordEmmanuel commented Mar 13, 2021

Reference Issues/PRs

Fixes #18584
Closes #19459
Closes #19370

What does this implement/fix? Explain your changes.

Fixes the default value of random state in the randomized_svd function and provides a deprecation warning should the value not be provided. 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.

Any other comments?

Thanks to @cinbez for her initial work on this

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @cliffordEmmanuel !

sklearn/utils/extmath.py Show resolved Hide resolved
sklearn/utils/extmath.py Outdated Show resolved Hide resolved
doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved
@cliffordEmmanuel
Copy link
Contributor Author

Thank you @thomasjpfan
I really appreciate your feedback. I'll incorporate those changes right away

cliffordEmmanuel and others added 2 commits March 13, 2021 15:49
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@cliffordEmmanuel
Copy link
Contributor Author

Hi, @thomasjpfan can you please advise on the Changelog Check. It seems to fail every time.
Thanks!

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment, otherwise LGTM!

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved
@thomasjpfan thomasjpfan changed the title Random svd param change ENH Deprecated the default random_state=0 in randomized_svd Mar 13, 2021
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Check Changelog / check (pull_request) Failing after 18s

Looks like a false positive.

@rth rth merged commit 77e998d into scikit-learn:main Mar 15, 2021
@cmarmo
Copy link
Contributor

cmarmo commented Mar 15, 2021

Looks like a false positive.

The change log file refers to the previous pull request #19459 not the current one #19670. I don't believe this is an issue... :) I'm commenting here to track the information on the test failure.

@cliffordEmmanuel
Copy link
Contributor Author

cliffordEmmanuel commented Mar 15, 2021

Thank you all, this has been my first attempt at this and I'm really grateful for the experience.
Moving on to find another.... :)

@ogrisel
Copy link
Member

ogrisel commented Mar 15, 2021

Congrats @cliffordEmmanuel!

@reshamas
Copy link
Member

#DataUmbrella

@cinbez
Copy link
Contributor

cinbez commented Mar 15, 2021 via email

marrodion pushed a commit to marrodion/scikit-learn that referenced this pull request Mar 17, 2021
…earn#19670)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: cinbez <cindyb@lightstone.co.za>
@cliffordEmmanuel cliffordEmmanuel deleted the random_svd_param_change branch March 22, 2021 05:46
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set utils.extmath.randomized_svd parameter random_state default value to None
7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.