[MRG] add random_state in tests estimators

[kkatrio]

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]

@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!

[raghavrv]

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]

🎉

[amueller]

this has lots of conflicts :-/

[kkatrio]

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]

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]

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]

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

[GaelVaroquaux]

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

I am clearly convinced that we should set them. How bad would be the conflict situation with other PRs?

[rth]

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]

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