Skip to content

DOC default random_state in _fetch_20newsgroup is 42, not None #26725

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

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

mathurinm
Copy link
Contributor

@github-actions
Copy link

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 53a81ed. Link to the linter CI: here

@betatim
Copy link
Member

betatim commented Jun 29, 2023

Should we make the fix in the other direction (change the code, keep the docs)? It seems odd to use 42 as default, the rest of scikit-learn uses None.

@mathurinm
Copy link
Contributor Author

Changing the default to None seems more in line with the rest of the codebase but it would require a deprecation cycle, as it will change the behaviour of existing code, right?

@betatim
Copy link
Member

betatim commented Jun 29, 2023

I'd consider it a bug fix, and for those I don't think we need a deprecation cycle.

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.

Most dataset fetchers that have a random_state have it None by default. Only this one and fetch_olivetti_faces have a non None default, and fetch_olivetti_faces is correctly documented. I'd like to have None by default consistently in all fetchers at some point. For now I'm fine with fixing the doc. Thanks @mathurinm

@jeremiedbb jeremiedbb merged commit a2f1779 into scikit-learn:main Jun 30, 2023
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
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