TST use global_random_seed in sklearn/_loss/tests/test_link.py #23751
Conversation
test_link_inverse_identity test_link_out_argument
sklearn/_loss/tests/test_link.py
Outdated
| @@ -73,15 +73,15 @@ def test_link_inverse_identity(link): | |||
| # we do not need to distinguish. | |||
| raw_prediction = rng.normal(loc=0, scale=10, size=(n_samples)) | |||
|
|
|||
| assert_allclose(link.link(link.inverse(raw_prediction)), raw_prediction) | |||
| assert_allclose(link.link(link.inverse(raw_prediction)), raw_prediction, rtol=1e-2) | |||
There was a problem hiding this comment.
This rtol is a bit high. Is it specific for a certain link function?
There was a problem hiding this comment.
Agreed. Good point regarding the link function. I checked and all test failures (in both tests) are related to the LogitLink link function. Should it therefore maybe be tested separately?
There was a problem hiding this comment.
I thinks raw_prediction = rng.normal(loc=0, scale=10) sometimes creates very large (or very negative) values. Logit link then takes the exponential and the logarithms which leads to loss of precision.
We can use different random numbers, how about rng.uniform(-30, 30)?
There was a problem hiding this comment.
I think this is worth trying. We should document as well with a comment this behaviour and why we do this choice. This is not a trivial thing.
There was a problem hiding this comment.
I tried your suggestion but it results in the same test failures compared to using rng.normal, i.e. all test failures are related to the LogitLink function and the failures occur with the same random seeds.
There was a problem hiding this comment.
Let's go with rng.uniform(-20, 20). The reason is that for the logit link
from scipy.special import expit, logit
x=20
logit(expit(x))the term expit(x) comes very close to 1 for large positive x and therefore loses precision. For large negative values, expit(x) is close to zero an therefore does not lose precision.
With float64, logit(expit(20)) is correct to about rtol 2e-9.
|
I found a solution by using |
|
Also, to debug the issue, here's a minimal reproducible of one of the failing tests: import numpy as np
import pandas as pd
from numpy.testing import assert_allclose
from sklearn._loss.link import (
_LINKS,
MultinomialLogit,
)
rng = np.random.RandomState(8)
link = _LINKS['logit']()
n_samples, n_classes = 100, None
if link.is_multiclass:
n_classes = 10
raw_prediction = rng.normal(loc=0, scale=10, size=(n_samples, n_classes))
if isinstance(link, MultinomialLogit):
raw_prediction = link.symmetrize_raw_prediction(raw_prediction)
else:
# So far, the valid interval of raw_prediction is (-inf, inf) and
# we do not need to distinguish.
raw_prediction = rng.normal(loc=0, scale=10, size=(n_samples))
print(f"stats: {pd.DataFrame(raw_prediction).describe()}")
assert_allclose(link.link(link.inverse(raw_prediction)), raw_prediction)which fails with: The maximum number of mismatched elements in the failing tests is 3 (as shown in this example). |
|
Thanks @marenwestermann LGTM. |
|
Thanks a lot for the reviews, I learned a lot! :) |
|
Thanks @marenwestermann 🙏 |
…t-learn#23751) Co-authored-by: Maren Westermann <maren.westermann@free-now.com>
Reference Issues/PRs
towards #22827
What does this implement/fix? Explain your changes.
I used global_random_seed in the functions
test_link_inverse_identityandtest_link_out_argument. With respect to both tests, this caused a number of test failures, all related to the default value ofrtolinassert_allclose. I increased this value in both tests which made all tests pass. Please check if the increased tolerance is acceptable.Any other comments?
ping @lorentzenchr