-
-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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
Random svd parameter change #19459
Conversation
#DataUmbrella sprint |
@cinbez In the PR description above, can you add "Closes PR 19370", after the "Fixes #18584"? cc: @Mariam-ke |
There was a problem hiding this 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:
This is the reason why the tests are failing.
Thanks @ogrisel! Sorry about that. Added it in now. Hope I got it right! |
@cinbez, almost there! The tests are failing because some of them now throw the
for the test related to the function you modified and
for common tests (in case your modifications affect them... this is not always the case). |
@cinbez How is this PR going? Please let us know if we can answer any questions. cc: @Mariam-ke |
Hi there
I’ve had a tragedy in my family. Mom passed away last week after being ill for some weeks and dad is in hospital. So sorry.
Regards,
Cindy.
On 11 Mar 2021, at 16:17, Reshama Shaikh ***@***.***> wrote:
@cinbez<https://github.com/cinbez> How is this PR going? Please let us know if we can answer any questions.
cc: @Mariam-ke<https://github.com/Mariam-ke>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#19459 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFQLTT54YLCDT5J3BMA4IR3TDC7EVANCNFSM4XUFY4AQ>.
|
@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. |
Hi @reshamas, |
@cliffordEmmanuel Yes, of course. Please let us know if you have any questions. |
Sure, thanks very much |
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. |
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.