FIX Fixes PLSRegression regression for constant Yk #19922
Conversation
sklearn/cross_decomposition/_pls.py
Outdated
| @@ -232,6 +232,11 @@ def fit(self, X, Y): | ||
| # paper. | ||
| Y_eps = np.finfo(Yk.dtype).eps | ||
| for k in range(n_components): | ||
| if np.all(np.dot(Yk.T, Yk) < Y_eps): | ||
| # Yk constant | ||
| warnings.warn('Y residual constant at iteration %s' % k) |
There was a problem hiding this comment.
😮 no fstrings?
| warnings.warn('Y residual constant at iteration %s' % k) | |
| warnings.warn('Y residual is constant at iteration %s' % k) |
sklearn/cross_decomposition/_pls.py
Outdated
| @@ -232,6 +232,11 @@ def fit(self, X, Y): | ||
| # paper. | ||
| Y_eps = np.finfo(Yk.dtype).eps | ||
| for k in range(n_components): | ||
| if np.all(np.dot(Yk.T, Yk) < Y_eps): |
There was a problem hiding this comment.
Shouldn't we catch the StopIteration (in the nipals algorithm above) instead?
This is potentially a pretty expensive check to run (matrix multiplication), and I'm not sure at first sight that it's more accurate?
There was a problem hiding this comment.
Yea that makes sense. I moved the logic to catching the exception and breaking out of the loop then.
There was a problem hiding this comment.
LGTM. @NicolasHug Could you have a second look such that we can include in 0.24.2
| _get_first_singular_vectors_power_method( | ||
| Xk, Yk, mode=self.mode, max_iter=self.max_iter, | ||
| tol=self.tol, norm_y_weights=norm_y_weights) | ||
| try: |
There was a problem hiding this comment.
I was more thinking about doing the try/except on the offending line:
y_score = next(col for col in Y.T if np.any(np.abs(col) > eps))
Is there a way we can move it there while keeping the logic here reasonably sane?
My concern is that it's not obvious which line can raise a StopIteration if we leave the try/except here.
There was a problem hiding this comment.
If we catch it at y_score, we still need to raise another exception to tell the caller to break out of the loop.
I added 3d4b6d2 (#19922) to make the control flow a little more obvious.
|
I will backport it in the PR for the release now |
Reference Issues/PRs
Fixes #19831
What does this implement/fix? Explain your changes.
Places the check on y from 0.23.2 back into the loop.