FIX RuntimeWarning by dividing by zero in test_kernel_gradient #19396
Conversation
|
Hi @nuka137 thanks for this proposition! Could you add a test to assert that the warning is not thrown with this correction? 🙂 |
|
Thanks! I added the no warning test. |
| @@ -19,7 +19,7 @@ | |||
|
|
|||
| from sklearn.utils._testing import (assert_almost_equal, assert_array_equal, | |||
| assert_array_almost_equal, | |||
| assert_allclose, | |||
| assert_allclose, assert_no_warnings, | |||
There was a problem hiding this comment.
Maybe add a line break here to respect the structure of the imports.
There was a problem hiding this comment.
Thanks, I added a line break.
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
sklearn/gaussian_process/kernels.py
Outdated
| denominator = np.sqrt(D.sum(2))[:, :, np.newaxis] | ||
| denominator[denominator == 0.0] = 1e-8 | ||
| K_gradient = K[..., np.newaxis] * D / denominator |
There was a problem hiding this comment.
I find it a bit cleaner to only divide where it's non zero
denominator = np.sqrt(D.sum(2))[:, :, np.newaxis]
K_gradient = K[..., np.newaxis] * np.divide(D, denominator, where=denominator != 0)There was a problem hiding this comment.
Won't it change the normalisation?
There was a problem hiding this comment.
D = 0 where denominator = 0, so K_gradient = 0 where denominator = 0, regardless of the normalization
There was a problem hiding this comment.
Yeah, that is much cleaner way.
I applied your suggestion, thanks.
| @@ -52,7 +53,7 @@ | |||
| @pytest.mark.parametrize('kernel', kernels) | |||
| def test_kernel_gradient(kernel): | |||
| # Compare analytic and numeric gradient of kernels. | |||
| K, K_gradient = kernel(X, eval_gradient=True) | |||
| K, K_gradient = assert_no_warnings(kernel, X, eval_gradient=True) | |||
There was a problem hiding this comment.
I don't think we need to check that no warning is issued. Otherwise we'd have to to that for every line of code. The fact that the warning disappears with this PR should be enough to me.
There was a problem hiding this comment.
I reverted this line to original.
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
rth
changed the title
Reference Issues/PRs
#19334
What does this implement/fix? Explain your changes.
The runtime error is caused by
Matern::__call__function.The error will be raised when
Dhas a zero element.So, I added small value
1e-8tonp.sqrt(D.sum(2))[:, :, np.newaxis]before dividingK[..., np.newaxis]by it.Because
K[..., np.newaxis] * Dwill be zero, I think above tweaking is no problem.