[go: nahoru, domu]

Skip to content
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

Random svd parameter change #19459

Closed
wants to merge 4 commits into from

Conversation

cinbez
Copy link
Contributor
@cinbez cinbez commented Feb 15, 2021

Reference Issues/PRs

Fixes #18584
Closes PR #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 @cliffordEmmanuel for his assistance.

@cinbez
Copy link
Contributor Author
cinbez commented Feb 15, 2021

#DataUmbrella sprint

@reshamas
Copy link
Member
reshamas commented Feb 16, 2021

@cinbez
Thanks for the update. I sent @cliffordEmmanuel as well.

In the PR description above, can you add "Closes PR 19370", after the "Fixes #18584"?

cc: @Mariam-ke

Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Thanks @cinbez for the follow-up PR. However the snippet of code that actually raises the deprecation warning has been lost in the process:

https://github.com/scikit-learn/scikit-learn/pull/19370/files#diff-86c94a3ca33490c6190f488f5d40b01bf0fd29be36da0b4497ef0da1fda4148aR342-R352

This is the reason why the tests are failing.

@cinbez
Copy link
Contributor Author
cinbez commented Feb 19, 2021

Thanks @ogrisel! Sorry about that. Added it in now. Hope I got it right!

@cinbez cinbez requested a review from ogrisel February 23, 2021 06:36
@cmarmo
Copy link
Member
cmarmo commented Feb 24, 2021

@cinbez, almost there! The tests are failing because some of them now throw the FutureWarning you just added.
While the log of the checks is available here, you might also want to reproduce the check locally with the following commands

$ pytest  sklearn/utils/tests/test_extmath.py -Werror::FutureWarning

for the test related to the function you modified and

$ pytest  sklearn/tests -Werror::FutureWarning

for common tests (in case your modifications affect them... this is not always the case).

@reshamas
Copy link
Member

@cinbez How is this PR going? Please let us know if we can answer any questions.

cc: @Mariam-ke

@cinbez
Copy link
Contributor Author
cinbez commented Mar 11, 2021 via email

@reshamas
Copy link
Member

@cinbez Very sorry to hear the news. My deepest condolences to you and your family.

We'll pass this PR on to someone else. Thanks for being in communication.

@cliffordEmmanuel
Copy link
Contributor
cliffordEmmanuel commented Mar 11, 2021

Hi @reshamas,
Please is it possible for me to complete this PR?

@reshamas
Copy link
Member

@cliffordEmmanuel Yes, of course. Please let us know if you have any questions.

@cliffordEmmanuel
Copy link
Contributor

Sure, thanks very much

@cliffordEmmanuel
Copy link
Contributor

Hi @cmarmo it seems because of the conflict with the whats_new/v1.0.rst when I do a git pull upstream main it's says automatic merge failed.

please any idea on how to rectify this?

@cliffordEmmanuel
Copy link
Contributor
cliffordEmmanuel commented Mar 13, 2021

Hi @cmarmo it seems because of the conflict with the whats_new/v1.0.rst when I do a git pull upstream main it's says automatic merge failed.

please any idea on how to rectify this?

Never mind I rectified it now.

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
5 participants