Skip to content
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

TST ensure that sklearn/_loss/tests/test_loss.py is seed insensitive #22847

Merged

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Mar 15, 2022

Towards #22827

I only modified the tests that were testing mathematical properties of the loss implementations on random data. I skipped tests that are just about shapes, dtypes and error messages as they are unlikely to suffer from seed-sensitivity.

I launched:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" pytest -v sklearn/_loss/tests/test_loss.py

on my local machine and all tests pass. Those tests are already seed-insensitive.

Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

LGTM, only one nitpick.

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 once the disagreement is solved :)

Co-authored-by: Christian Lorentzen <[email protected]>
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 these test with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" on a M1 CPU and a AMD CPU and they all passed.

LGTM

@jeremiedbb jeremiedbb merged commit 751c5cd into scikit-learn:main Mar 15, 2022
@ogrisel ogrisel deleted the seed-insensitive-cython-loss-tests branch March 15, 2022 19:39
@lorentzenchr
Copy link
Member

I feel honored that the loss module got picked as first PR towards #22827 😉 And I feel relieved that it passed!

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
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.

4 participants