Skip to content

FIX ravel prediction of PLSRegression when fitted on 1d y #26602

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 8 commits into from
Jul 24, 2023

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Jun 18, 2023

Reference Issues/PRs

Fixes: #26549.

What does this implement/fix? Explain your changes.

This implements #26549 (comment) for PLSRegression. Please let me know if the alternative (#26549 (comment)) is more desired, or if I should implement this for other multi-output regressors as well in this PR.

@Charlie-XIAO Charlie-XIAO marked this pull request as ready for review June 18, 2023 12:14
Ypred = X @ self.coef_.T
return Ypred + self.intercept_
Ypred = X @ self.coef_.T + self.intercept_
return Ypred.ravel() if self._predict_1d else Ypred
Copy link
Member

Choose a reason for hiding this comment

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

nit: my personal taste/preference is to split this up into a if self._predict_1d: ... else: ... (so spread over four lines). I find it easier to read/spot that there is a if here compared to using the one liner. But others might have other tastes :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion I think it is fine both ways.

Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable and minimal solution to the problem

@betatim betatim added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Jul 11, 2023
Copy link
Contributor

@OmarManzoor OmarManzoor 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 @Charlie-XIAO. Just a few comments.

@OmarManzoor OmarManzoor removed the Waiting for Second Reviewer First reviewer is done, need a second one! label Jul 20, 2023
@OmarManzoor OmarManzoor changed the title ENH ravel prediction of PLSRegression when fitted on 1d y FIX ravel prediction of PLSRegression when fitted on 1d y Jul 20, 2023
@github-actions
Copy link

github-actions bot commented Jul 21, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 53d4907. Link to the linter CI: here

@Charlie-XIAO
Copy link
Contributor Author

@OmarManzoor Thanks for your review! I've resolved the issues you mentioned.

@Charlie-XIAO
Copy link
Contributor Author

Sorry for the late reply @OmarManzoor, committed your suggestions now.

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @Charlie-XIAO

@OmarManzoor OmarManzoor merged commit 75f3e47 into scikit-learn:main Jul 24, 2023
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
@marenwestermann
Copy link
Member

The changelog entry of this PR seems to have accidentally been deleted in the recent restructuring. I think it needs to be added again.

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:cross_decomposition Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PLSRegression not working with VotingRegressor
4 participants