8000 TST fix unstable test_newrand_set_seed by ogrisel · Pull Request #25940 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

TST fix unstable test_newrand_set_seed #25940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 22, 2023
Merged

TST fix unstable test_newrand_set_seed #25940

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
518d878
TST fix unstable test_newrand_set_seed
ogrisel Mar 22, 2023
b7fdb4b
Improve code style of new test
ogrisel Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions sklearn/svm/tests/test_bounds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,24 @@ def test_ill_posed_min_c():
_MAX_UNSIGNED_INT = 4294967295


@pytest.mark.parametrize("seed, val", [(None, 81), (0, 54), (_MAX_UNSIGNED_INT, 9)])
def test_newrand_set_seed(seed, val):
def test_newrand_default():
"""Test that bounded_rand_int_wrap without seeding respects the range

Note this test should pass either if executed alone, or in conjunctions
with other tests that call set_seed explicit in any order: it checks
invariants on the RNG instead of specific values.
"""
generated = [bounded_rand_int_wrap(100) for _ in range(10)]
assert all(0 <= x < 100 for x in generated)
assert not all(x == generated[0] for x in generated)


@pytest.mark.parametrize("seed, expected", [(0, 54), (_MAX_UNSIGNED_INT, 9)])
def test_newrand_set_seed(seed, expected):
"""Test that `set_seed` produces deterministic results"""
if seed is not None:
set_seed_wrap(seed)
x = bounded_rand_int_wrap(100)
assert x == val, f"Expected {val} but got {x} instead"
set_seed_wrap(seed)
generated = bounded_rand_int_wrap(100)
assert generated == expected


@pytest.mark.parametrize("seed", [-1, _MAX_UNSIGNED_INT + 1])
Expand All @@ -91,6 +102,9 @@ def test_newrand_set_seed_overflow(seed):
@pytest.mark.parametrize("range_, n_pts", [(_MAX_UNSIGNED_INT, 10000), (100, 25)])
def test_newrand_bounded_rand_int(range_, n_pts):
"""Test that `bounded_rand_int` follows a uniform distribution"""
# XXX: this test is very seed sensitive: either it is wrong (too strict?)
# or the wrapped RNG is not uniform enough, at least on some platforms.
set_seed_wrap(42)
Copy link
Member Author
@ogrisel ogrisel Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to tweak the tests parameters to make it pass with global_random_seed = "all" but this is very brittle. I am not sure if this is a problem of the RNG or a problem of the test and fixing this test was not the original purpose of the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it tests statisical properties, I guess it can fail with a non-zero probability

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but it fails very quickly, even if I increase the number of points or relax the quantile used in the second part. I suspect there is something really fishy going on but I wasted enough time on this and I don't think it will significantly impact users of our SVM models in practice.

n_iter = 100
ks_pvals = []
uniform_dist = stats.uniform(loc=0, scale=range_)
Expand Down
0