Skip to content

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 <[email protected]>
@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 <[email protected]>
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