Skip to content

[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

Merged
merged 5 commits into from
Feb 1, 2021

Conversation

BaptBillard
Copy link
Contributor

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 was algorthm=nipals. Thus, by removing the 'svd' flag, only one value was available which implies that the algorithm 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 the srt file.

Copy link
Member

@NicolasHug NicolasHug left a 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

@BaptBillard
Copy link
Contributor Author

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!

Base automatically changed from master to main January 22, 2021 10:53
@BaptBillard
Copy link
Contributor Author

@NicolasHug it should be okay now!

Copy link
Member

@jeremiedbb jeremiedbb left a 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>
@rth
Copy link
Member

rth commented Feb 1, 2021

Merging with +2. Thanks!

@rth rth merged commit 07e1a6b into scikit-learn:main Feb 1, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
glemaitre pushed a commit that referenced this pull request Apr 28, 2021
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
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.

PLSRegression unexpected keyword argument 'algorithm'
5 participants