Raise ValueError instead of RuntimeWarning for LinearModelLoss #27332
Conversation
a278528 to
a6172b6
Compare
There was a problem hiding this comment.
To fix the codecov error, you need to add non-regression test. Basically, it would be a test that would raise the new ValueError and ensure that we raise the proper error message (this is your original issue).
Then, you need to add an entry in the changelog doc/whats_new/1.4.rst to acknowledge the bug fix. This will solve the over CI failure.
| with np.errstate(all="raise"): | ||
| try: | ||
| grad[:n_features] = X.T @ grad_pointwise + l2_reg_strength * weights | ||
| except FloatingPointError: |
There was a problem hiding this comment.
| except FloatingPointError: | |
| except FloatingPointError as exc: |
There was a problem hiding this comment.
When I use exc as you have in this suggestion, testing on the dataset from I get the following:
---------------------------------------------------------------------------
FloatingPointError Traceback (most recent call last)
File [~/Downloads/scikit-learn/sklearn/linear_model/_linear_loss.py:296](https://file+.vscode-resource.vscode-cdn.net/Users/d0055315_akaash_patel/Downloads/scikit-learn/scratchpad~/~/Downloads/scikit-learn/sklearn/linear_model/_linear_loss.py:296), in LinearModelLoss.loss_gradient(self, coef, X, y, sample_weight, l2_reg_strength, n_threads, raw_prediction)
[295] try:
--> [296] grad[:n_features] = X.T @ grad_pointwise + l2_reg_strength * weights
[297] print("min entry", np.min(grad_pointwise))
FloatingPointError: invalid value encountered in matmul
The above exception was the direct cause of the following exception:
so it still includes the original exception that I am "wrapping" with a ValueError and message. Do you reckon it's better this way?
| raise ValueError( | ||
| "Overflow detected. Try scaling the target variable or" | ||
| " features, or using a different solver" | ||
| ) from None |
There was a problem hiding this comment.
| ) from None | |
| ) from exc |
| grad[:, :n_features] = ( | ||
| grad_pointwise.T @ X + l2_reg_strength * weights | ||
| ) | ||
| except FloatingPointError: |
There was a problem hiding this comment.
| except FloatingPointError: | |
| except FloatingPointError as exc: |
| raise ValueError( | ||
| "Overflow detected. Try scaling the target variable or" | ||
| " features, or using a different solver" | ||
| ) from None |
There was a problem hiding this comment.
| ) from None | |
| ) from exc |
| if self.fit_intercept: | ||
| grad[-1] = grad_pointwise.sum() | ||
| else: | ||
| grad = np.empty((n_classes, n_dof), dtype=weights.dtype, order="F") | ||
| # grad_pointwise.shape = (n_samples, n_classes) | ||
| grad[:, :n_features] = grad_pointwise.T @ X + l2_reg_strength * weights | ||
| with np.errstate(all="raise"): |
There was a problem hiding this comment.
If this is an overflow, I think that using over="raise" would be better.
There was a problem hiding this comment.
I tried using over="raise", however this didn't raise an Exception and the RuntimeWarning appeared instead - when I tested with the dataset I encountered this issue with in the first place #27016. During one of the iterations, there was an infinity in one the gradients which is where I wanted to raise the Exception.
3b8c7e7 to
0a58a75
Compare
0a58a75 to
fce1afc
Compare
There was a problem hiding this comment.
@akaashp2000 Could you address the review and add a changelog entry and a non-regression test?
fce1afc to
d1c7ebf
Compare
d1c7ebf to
9585d68
Compare
|
@akaashp2000 Could you request a review when the PR is ready for review. |
|
@glemaitre Hi, sorry for the slowness on this issue. Yes I'll address the non-regression test and changelog this weekend. A couple of other questions have come up too regarding the remaining suggestions you have made on the PR - which I'll reply to directly. |
|
@glemaitre while trying to write the non-regression test, I'm testing with the breast cancer data. I multiplied all values in the first column of X_breast_cancer by 100, which meant that ValueError was now raised, even though all entries in the gradient vector seemed okay (no infinities or NaNs, there were zeroes and very small values like 0 or 1.03566975e-157). |
Note, similar solution as in scikit-learn#27332
Note, similar solution as in scikit-learn#27332
Reference Issues/PRs
Fixes #27016
What does this implement/fix? Explain your changes.
Fixes an issue where
PoissonRegressor(and other regressors) encounter aRuntimeWarning, without any suggestion of the cause or ways to fix.The fix captures the
RuntimeWarningand raises aValueErrorinstead, in the case where the gradient vector contains invalid entries (e.g. inf, null), during a matmul. It also has a message suggesting scaling of features or target variable, or to use another solver.Any other comments?
So far I have just tested that this exception/message occur on the same test data where I encountered the issue (see the issue #27016).
I have included the same change for the case where the loss is multiclass, but haven't directly tested this on a multiclass problem.
The change caused some pre-existing pytests to start failing, due to warning that would have been raised being replaced by a ValueError as part of the issue. I have included a change to scale the X dataset so that this error doesn't occur to get those tests passing.
Questions: