-
-
Notifications
You must be signed in to change notification settings - Fork 26k
[MRG] Remove algorithm flag for PLS regression #19204
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
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 for the PR @BaptBillard
It seems that you have removed the parameter for _PLS
which is the base class. We don't want that: the algorithm
parameter is still relevant for PLSCannonical
.
All we need is to remove the reference to algorithm
in PLSRegression
's docstring
Oh ok, sorry for my misunderstanding! I will change that as soon as possible! |
Remove 'algorithm' flag in PLSRegression docstring.
@NicolasHug it should be okay now! |
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.
LGTM. Just a small nitpick
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Merging with +2. Thanks! |
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Reference Issues
Fixes #19197.
What does this implement/fix?
Problem using the flag
algorithm=svd
in PLSRegression method because it shouldn't be possible to do so. The only other possible value wasalgorthm=nipals
. Thus, by removing the 'svd' flag, only one value was available which implies that thealgorithm
flag wasn't useful anymore.Any other comments?
The
algorithm
flag has been removed from the code with all the associated checks (no function will check its value, the nipals algorithm will be used all the time). The documentation has been updated in the code and thesrt
file.