Skip to content

TST Ensure that sklearn/metrics/tests/test_pairwise_distances_reduction.py is seed insensitive #22862

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.

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

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Mar 16, 2022

Reference Issues/PRs

Partially addresses #22827

What does this implement/fix? Explain your changes.

This simply replaces the tests seed parametrisation by the new global fixture for the seed.

Any other comments?

The following passes on my machine:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" pytest -v sklearn/metrics/tests/test_pairwise_distances_reduction.py

Should the previously seed-unparametrised tests use the global fixture as well?

@jeremiedbb
Copy link
Member

jeremiedbb commented Mar 16, 2022

Should the previously seed-unparametrised tests use the global fixture as well?

I think so. They test data dependent results on randomly generated datasets.
I also think that the global_random_seed should be passed to _get_metric_params_list

Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
@jjerphan
Copy link
Member Author

Addressed in 50cf65e.

Tests that are only checking correct behaviours on wrong usages do not use the global fixture, but all the others do.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I ran the tests locally with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" on a AMD CPU and a M1 machine and all tests passed.

LGTM

@thomasjpfan thomasjpfan merged commit 87bb024 into scikit-learn:main Mar 17, 2022
@jjerphan jjerphan deleted the tst/global_random_seed/test_pairwise_distances_reduction.py branch March 17, 2022 06:31
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
…ion.py` is seed insensitive (scikit-learn#22862)

Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants