-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
FIX ravel prediction of PLSRegression
when fitted on 1d y
#26602
Conversation
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 |
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.
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 :-/
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.
In my opinion I think it is fine both ways.
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.
I think this is a reasonable and minimal solution to the problem
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 @Charlie-XIAO. Just a few comments.
PLSRegression
when fitted on 1d y
PLSRegression
when fitted on 1d y
@OmarManzoor Thanks for your review! I've resolved the issues you mentioned. |
Sorry for the late reply @OmarManzoor, committed your suggestions 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! Thanks @Charlie-XIAO
The changelog entry of this PR seems to have accidentally been deleted in the recent restructuring. I think it needs to be added again. |
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.