-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
[MRG+2] fix warning and behavior in randomized_svd wrt power iterations #7311
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
Conversation
ping @giorgiop was that the intended behavior? I changed the default value of Also, now, if a user gives a particular integer, we will actually do what they said! |
LGTM. The code is indeed much nicer than with the n_iter_specified. Do we have an idea, a track record of any particular reason that we ended up with this variable (n_iter_specified)? Cc @giorgiop |
@amueller Before merge, you have a docstring that you need to fix: it gives a failing doctest. |
dd1e77d
to
970ace9
Compare
is set to 7, unless the user specifies a higher number. This improves | ||
precision with few components. | ||
problems. When 'auto', it is set to 4, unless `n_components` is small | ||
(< .1 * min(X.shape)) `n_iter` in which case is set to 7. |
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.
maybe i'm being slow, but i'm not sure what this sentence means.
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.
Maybe the following would be clearer:
When `n_iter='auto'`, it is effectively set to 4 to when `n_components` is larger than
`0.1 * min(X.shape)` and set to 7 otherwise.
LGTM as well besides failing doctest / the sentence I can't comprehend. |
Besides the failing doctest, LGTM as well. |
@amueller The failings come from the |
I rebased, fixed the doctests and merged. |
thanks @ogrisel |
Trying to fix the warning mess left by #5299 and #6749 (issue #6746).