8000 [MRG] add random_state in tests estimators by kkatrio · Pull Request #8563 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] add random_state in tests estimators #8563

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[MRG] add random_state in tests estimators #8563

wants to merge 5 commits into from

Conversation

kkatrio
Copy link
@kkatrio kkatrio commented Mar 9, 2017

Reference Issue

Fix #8194

What does this implement/fix? Explain your changes.

Pass random_state in all test cases where it was missing in the estimator's invocation.

Any other comments?

Added only in a few tests yet, nosetests ran OK. If it is correct and there are no further problems, I will add all the rest. Also, should I add in the contributing docs that when possible all estimators in new tests must be invoked with random_state set?

@kkatrio kkatrio changed the title [WIP] add random_state in tests estimators [MPR] add random_state in tests estimators Mar 10, 2017
@kkatrio
Copy link
Author
kkatrio commented Mar 10, 2017 •
edited
Loading

@amueller I added the random_state in all cases I could find that made sense to me. I am not sure where to add the suggestion for new tests to pass the random_state in the docs.

@Nuffe 's excellent scipt still suggests some cases but most of them are false positive - I think!

@kkatrio kkatrio changed the title [MPR] add random_state in tests estimators [MRG] add random_state in tests estimators Mar 10, 2017
@raghavrv raghavrv mentioned this pull request Mar 12, 2017
Closed
@kkatrio kkatrio changed the title [MRG] add random_state in tests estimators [WIP] add random_state in tests estimators Mar 12, 2017
@kkatrio kkatrio changed the title [WIP] add random_state in tests estimators [MRG] add random_state in tests estimators Mar 12, 2017
@raghavrv
Copy link
Member

Seems good to me... But I'm sure merging this is going to raise merge conflicts at several PRs ;)

@jnothman / @amueller do you want to have a second look?

raghavrv
raghavrv approved these changes Mar 14, 2017
View reviewed changes
Copy link
Member
@raghavrv raghavrv left a comment

Choose a reason for hiding this comment

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

🎉

@glemaitre glemaitre mentioned this pull request Mar 16, 2017
Open
@amueller
Copy link
Member

this has lots of conflicts :-/

@kkatrio
Copy link
Author
kkatrio commented Jul 13, 2017

I remember at some point a couple of senior devs (I can't find now exactly where, at an email probably) agreeing that random state should not be passed at tests. That's why I left it as it is. In hindsight, my personal --inexperienced-- opinion is that it is not always the best idea. Perhaps each test has different value if it is deterministic or not. If you want I could try to quickly resolve those conflicts but I cannot look into each test and understand if it's the best thing to do.

@amueller
Copy link
Member

Sorry I don't understand you comment. I thought we agreed we wanted to add the random state. What does that have to do with the conflicts?

@kkatrio
Copy link
Author
kkatrio commented Jul 14, 2017

If we want to add the random state at almost all tests, then I will resolve the conflicts. If it is going to be integrated in the master then great. I think there was a discussion here #8194

@amueller
Copy link
Member

ping @lesteve @agramfort @ogrisel @GaelVaroquaux So what's the consensus? I think we should set them.

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Jul 15, 2017 via email

@rth
Copy link
Member
rth commented Jun 1, 2018 •
edited
Loading

Seems like a lot of conflicts..

Would monkeypatching random_state in all estimators in a pytest fixture (see e.g. https://docs.pytest.org/en/latest/monkeypatch.html#example-preventing-requests-from-remote-operations) be too horrible? For instance, with a few lines, one could make it a property there that returns e.g. 42 if it's set with None in __init__, but I guess it will break some of API tests (or at least make results dubious).. ping @lesteve

@lesteve
Copy link
Member
lesteve commented Jun 4, 2018

Just for the record, I am not really convinced that this is worth the pain as I tried to explain in #8194 (comment).

@amueller amueller added the Needs Decision Requires decision label Aug 5, 2019
Base automatically changed from master to main January 22, 2021 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raghavrv raghavrv raghavrv approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Needs Decision Requires decision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should all test cases set the random_state?
6 participants
@kkatrio @raghavrv @amueller @GaelVaroquaux @rth @lesteve
0