[WIP] Fix ZeroDivisionWarning in Matern Kernel #7385
Conversation
sklearn/gaussian_process/kernels.py
Outdated
| K_gradient[~np.isfinite(K_gradient)] = 0 | ||
| dist = np.ma.masked_array(np.sqrt(D.sum(2))[:, :, np.newaxis]) | ||
| K_gradient = K[..., np.newaxis] * D / dist | ||
| K_gradient = K_gradient.filled(0) |
There was a problem hiding this comment.
K_gradient remains a masked_array afterwards?
There was a problem hiding this comment.
no, http://docs.scipy.org/doc/numpy-1.10.1/reference/generated/numpy.ma.MaskedArray.filled.html
The notes say "The result is not a masked array"
|
Can you please add a non regression test with |
|
done. hope that was sufficient. |
| @@ -317,4 +317,4 @@ def test_repr_kernels(): | |||
|
|
|||
| def test_no_warnings_matern_eval_gradient(): | |||
| mat = Matern(length_scale=[0.5, 2.0], nu=0.5) | |||
| assert_no_warnings(mat, X, eval_gradient=True) | |||
There was a problem hiding this comment.
You don't need to pass __call__. The original line was already correct.
|
Sorry about pushing directly. I was not able to reproduce the failure by doing |
|
I was facing some issues in using masked arrays with regards to the warnings. I removed the masked arrays logic and replaced it with a less elegant (but working) solution using initialization of These are the following issues that I faced during the usage of masked arrays.
1 / np.ma.masked_equal([1, 0], 0)It is necessary to do this in those cases. np.ma.masked_array([1]) / np.ma.masked_equal([1, 0], 0)
I also feel it is necessary to use |
|
LGTM |
| K_gradient = K[..., np.newaxis] * D \ | ||
| / np.sqrt(D.sum(2))[:, :, np.newaxis] | ||
| K_gradient[~np.isfinite(K_gradient)] = 0 | ||
| K_gradient = np.zeros_like(D) |
There was a problem hiding this comment.
Is this faster or slower than the previous solution? Alternatively we could just catch the warning, because we expect it and treat it, right?
There was a problem hiding this comment.
Yeah, I don't like the solution that I wrote, but I wasn't sure if using the with warnings.catch_warnings(): warnings.simplefilter block was encouraged.
There was a problem hiding this comment.
You should catch only the ZeroDivisionError. It is expected and then treated, so it's ok to catch it (imho)
|
we use |
|
This is marked WIP and has a LGTM. What's the story, @MechCoder? Do we want this in 0.20? |
|
The story is that I got busy with other things to do :) I'm going for a
short vacation this week and I plan on spending some time on sklearn after
that (and before my job). However the quick fix would be to just silence
the warnings in the tests.
…On Jun 14, 2017 6:16 AM, "Joel Nothman" ***@***.***> wrote:
This is marked WIP and has a LGTM. What's the story, @MechCoder
<https://github.com/mechcoder>? Do we want this in 0.20?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7385 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABx9EIIPgxDJGpwP0uYQFU8HKVUfY22Zks5sDy1UgaJpZM4J5owl>
.
|
|
The fix was done in #24245. This PR can be closed. |
Reference Issue
Fixes what @amueller thought was the issue in #5664,
though that's not really the issue [edited by @amueller so that the issue remains open after this fix is merged]
What does this implement/fix? Explain your changes.
The previous code had got around the case where the distance equals zero by setting the gradient to zero after finding where it becomes infinite that raises a
ZeroDivisionWarning. This is another solution using masked arrays in NumPy. I am unable to reproduce any other warning on master with the latest versions of NumPy and SciPyThis change is