{"action":"created","issue":{"url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/8194","repository_url":"https://api.github.com/repos/scikit-learn/scikit-learn","labels_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/8194/labels{/name}","comments_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/8194/comments","events_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/8194/events","html_url":"https://github.com/scikit-learn/scikit-learn/issues/8194","id":200468177,"node_id":"MDU6SXNzdWUyMDA0NjgxNzc=","number":8194,"title":"Should all test cases set the random_state?","user":{"login":"erikcs","id":7185264,"node_id":"MDQ6VXNlcjcxODUyNjQ=","avatar_url":"https://avatars2.githubusercontent.com/u/7185264?v=4","gravatar_id":"","url":"https://api.github.com/users/erikcs","html_url":"https://github.com/erikcs","followers_url":"https://api.github.com/users/erikcs/followers","following_url":"https://api.github.com/users/erikcs/following{/other_user}","gists_url":"https://api.github.com/users/erikcs/gists{/gist_id}","starred_url":"https://api.github.com/users/erikcs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/erikcs/subscriptions","organizations_url":"https://api.github.com/users/erikcs/orgs","repos_url":"https://api.github.com/users/erikcs/repos","events_url":"https://api.github.com/users/erikcs/events{/privacy}","received_events_url":"https://api.github.com/users/erikcs/received_events","type":"User","site_admin":false},"labels":[{"id":12995769,"node_id":"MDU6TGFiZWwxMjk5NTc2OQ==","url":"https://api.github.com/repos/scikit-learn/scikit-learn/labels/Easy","name":"Easy","color":"02d7e1","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2017-01-12T20:14:10Z","updated_at":"2018-06-01T09:50:43Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"In [this](https://github.com/scikit-learn/scikit-learn/pull/7591) PR some newly added test cases made some unrelated tests fail. The culprit was mutating state due to missing `random_state`(s) in the unrelated tests' function/class invocations. @amueller also mention that:\r\n\r\n> Ideally we would explicitly pass random states everywhere in all tests. To fix the test failure, maybe fix the random states in the test that's failing. You can also add it in the test that you added.\r\n\r\nI made a simple python script [here](https://gist.github.com/nuffe/3afcc7d6682b05d71e59a99afcf0cbfe) that goes through each of `sklearn`'s test files and look for estimators/classes that take a `random_state` parameter, and see if this was passed in the invocation(s). If not then print out the class/function name followed by a list of line numbers with missing invocations.\r\n\r\nOn the latest master branch (*ca687ba*) there are 1 155 cases in 154 test files where a class/function that takes a `random_state` is called without setting it to the canonical `42`:\r\n\r\n```bash\r\nerik  scikit-learn$ find * -type f | grep \"/test_.*py$\" | python rcheck.py\r\nsklearn/tree/tests/test_tree.py\r\n\tDecisionTreeClassifier [398, 1173, 1210, 1231, 1237, 1531]\r\n\tDecisionTreeRegressor [1532]\r\nsklearn/tree/tests/test_export.py\r\n        DecisionTreeClassifier [197, 212]\r\n.\r\n.\r\n.\r\nsklearn/model_selection/tests/test_search.py\r\n\tDecisionTreeClassifier [868]\r\n\tDecisionTreeRegressor [868]\r\n\tGroupShuffleSplit [242]\r\n\tKFold [241, 250, 491, 503, 919, 1172, 1193]\r\n\tLinearSVC [292, 356, 363, 376, 383]\r\n\tLinearSVCNoScore [101, 210]\r\n\tParameterSampler [1093, 1096, 1113]\r\n\tRandomizedSearchCV [265, 704, 708, 753, 798, 839, 886, 946, 983]\r\n\tSGDClassifier [1126, 1141]\r\n\tSVC [292, 301, 305, 325, 333, 340, 348, 356, 363, 376, 383, 417, 441, 644, 647, 704, 708, 752, 753, 795, 798, 838, 839, 1003]\r\n\tStratifiedKFold [250, 919]\r\n\tStratifiedShuffleSplit [250]\r\n\r\nFound 1155 missing in 154 files\r\n```\r\nThe full output is [here](https://gist.github.com/nuffe/62fa22280117e0f90e988ee15aa2c966)\r\n\r\nIf desirable, I could work on a PR for a find/replace on all these cases? If so, then the scikit-learn contributing docs should perhaps be explicit in stating that all added test cases that invoke an estimator that takes a `random_state`, should also set it / or add some kind of flake plugin that checks this?\r\n\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/comments/393831965","html_url":"https://github.com/scikit-learn/scikit-learn/issues/8194#issuecomment-393831965","issue_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/8194","id":393831965,"node_id":"MDEyOklzc3VlQ29tbWVudDM5MzgzMTk2NQ==","user":{"login":"rth","id":630936,"node_id":"MDQ6VXNlcjYzMDkzNg==","avatar_url":"https://avatars0.githubusercontent.com/u/630936?v=4","gravatar_id":"","url":"https://api.github.com/users/rth","html_url":"https://github.com/rth","followers_url":"https://api.github.com/users/rth/followers","following_url":"https://api.github.com/users/rth/following{/other_user}","gists_url":"https://api.github.com/users/rth/gists{/gist_id}","starred_url":"https://api.github.com/users/rth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rth/subscriptions","organizations_url":"https://api.github.com/users/rth/orgs","repos_url":"https://api.github.com/users/rth/repos","events_url":"https://api.github.com/users/rth/events{/privacy}","received_events_url":"https://api.github.com/users/rth/received_events","type":"User","site_admin":false},"created_at":"2018-06-01T09:50:43Z","updated_at":"2018-06-01T09:50:43Z","author_association":"MEMBER","body":"> IMO the picture is not so clear-cut and I am not in favour of enforcing through a automated tool that random_state is always set.\r\n\r\nThere was some related work with LGTM queries in https://github.com/scikit-learn/scikit-learn/issues/10416"}}
{"actor":{"display_login":"rth"}}