MAINT: Truncate the warning msg about ill-conditioning #7980
Conversation
|
@rgommers Can we squeeze this into 1.0.0 or should we delay it? |
|
Doesn't look right to me: monkeypatching of standard modules. Instead of this, please add the stacklevel= argument as discussed elsewhere. |
|
@pv Oh, I've forgotten the stacklevel arg indeed. But this is just to modify the output message. The stacklevel will only effect the trailing line (see the first image under the rcond value |
|
Yes, please change the message and add the stacklevel, and remove the stuff modifying the warnings module. I think the issue is probably not major enough to warrant a backport (ie. it's just about the precise formatting of the warning message rather than the bug). |
scipy/linalg/basic.py
Outdated
| 'condition number/precision: {} / {}'.format(rcond, E), | ||
| RuntimeWarning) | ||
| 'condition number{:.6e}'.format(rcond), | ||
| RuntimeWarning, stacklevel=1) |
There was a problem hiding this comment.
If solve is called directly then stacklevel=3 will report -2th item on the tracestack instead of basic.py. I've highlighted the message below.
There was a problem hiding this comment.
Yes, and that's the correct place to emit the warning.
You can also find Numpy's np.deprecate decorator emits the warning with that stacklevel.
There was a problem hiding this comment.
Yes. Just to confirm if I understand correctly, this is only going to fix for linalg.solve warning and other functions which embed linalg.solve will be left as is (which was what Eric mentioned in the linked issue).
There was a problem hiding this comment.
Yes, I agree how to best deal with the warnings situation of nested calls to e.g. solve() is an open problem, given that python's warnings module is not fully up to the task imho.
Aside from solutions that require patching the warnings module (which I think we should not start doing), setting the stacklevel at least solves the issue in simplest cases, and we can hope the non-simple cases are not that pressing...
Another thing is that the emitted warning probably should be in a separate category to make these warnings easier to filter, warnings.warn(..., category=scipy.linalg.LinAlgWarning) (with class LinAlgWarning(RuntimeWarning): ... is added to scipy.linalg).
I agree, after a release candidate we only want to backport critical bug fixes. |
|
Ah, we don't want to change the warning message then? I guess we can close this one instead because that's the only thing this PR does. |
scipy/linalg/basic.py
Outdated
| @@ -34,10 +40,13 @@ def _solve_check(n, info, lamch=None, rcond=None): | |||
| return | |||
| E = lamch('E') | |||
| if rcond < E: | |||
| orig_formatwarning = warnings.formatwarning | |||
| warnings.formatwarning = _truncated_warning_msg | |||
There was a problem hiding this comment.
This stuff is not ok (and not really required afaics after setting the stacklevel correctly).
There was a problem hiding this comment.
I've picked it up from the official docs as the recommended practice to keep things local
There was a problem hiding this comment.
can you show where it's recommended? (some googling indicates at least one core python dev thinks otherwise)
in any case, I don't think it's required here, and moreover even if it is, it's not scipy's job to prettify the warning display
There was a problem hiding this comment.
Here it is https://docs.python.org/3.6/library/warnings.html#warnings.formatwarning
But I get your point then we can close this PR.
scipy/linalg/basic.py
Outdated
| 'versions of SciPy.', DeprecationWarning) | ||
| warn('Use of the "debug" keyword is deprecated ' | ||
| 'and this keyword will be removed in future ' | ||
| 'versions of SciPy.', DeprecationWarning, stacklevel=3) |
There was a problem hiding this comment.
stacklevel=2 probably
Also, ditto below
There was a problem hiding this comment.
ah yes, forgot about _solve_check
|
@pv Done. Since it is an untested keyword I've skipped the CI builds. |
Fixes #7951
The resulting warning message does not have the file name, which is always
../.../linalg/basic.pyanyways and the line number which is also fixed. Also it doesn't have the trailing line indicator.The current version
This PR