Skip to content

DOC Improve n_jobs docs in sklearn/multioutput.py #18634

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
Merged
merged 5 commits into from
Oct 30, 2020
Merged

DOC Improve n_jobs docs in sklearn/multioutput.py #18634

merged 5 commits into from
Oct 30, 2020

Conversation

tnwei
Copy link
Contributor

@tnwei tnwei commented Oct 17, 2020

Reference Issues/PRs

Addresses part of #14228

What does this implement/fix? Explain your changes.

Updated docs to reflect meaning and usage of n_jobs in sklearn/multioutput.py, affecting:

  • sklearn.multioutput.MultiOutputRegressor
  • sklearn.multioutput.MultiOutputClassifier

Any other comments?

  • Better documentation for n_jobs #14228 is opened to improve / clarify the docs for n_jobs in the sklearn code base. This PR focuses is tailored to address sklearn/multioutput.py.
  • sklearn.multioutput.MultiOutputRegressor has notes on using n_jobs > 1 for fast estimators potentially leading to slowdowns, moved to dedicated Notes section. Also replicated in MultiOutputClassifier for parity.

@tnwei tnwei mentioned this pull request Oct 17, 2020
Open
adrinjalali
adrinjalali reviewed Oct 19, 2020
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Thanks @tnwei

adrinjalali
adrinjalali approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tnwei

@tnwei
Copy link
Contributor Author

tnwei commented Oct 27, 2020

Thanks @adrinjalali for review, would like to ask you to label this PR as hacktoberfest-accepted if you don't mind

@adrinjalali
Copy link
Member

Only PRs in repos with hacktoberfest topic are included, and our repo is not. I'm afraid PRs in our repo won't count towards hacktoberfest.

@tnwei
Copy link
Contributor Author

tnwei commented Oct 28, 2020

@adrinjalali You're right that repos can opt-in for at the repo level via specifying the hacktoberfest topic, however any individual PR can count as well if they are labelled with hacktoberfest-accepted (https://hacktoberfest.digitalocean.com/hacktoberfest-update)

Aside from that, I have submitted a sister PR (#18633), which has been updated following changes to this one. Hope you can take a look if time permits, TQVM!

thomasjpfan
thomasjpfan reviewed Oct 29, 2020
View reviewed changes
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.

Thank you for the PR @tnwei !

@tnwei
Copy link
Contributor Author

tnwei commented Oct 30, 2020

Resolved comments!

thomasjpfan
thomasjpfan approved these changes Oct 30, 2020
View reviewed changes
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.

Thank you @tnwei!

LGTM

@thomasjpfan thomasjpfan merged commit 38a50f4 into scikit-learn:master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
Documentation hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tnwei @adrinjalali @thomasjpfan