Skip to content

[DOC] Fixing n_jobs doc param in multiclass.py#17489

Merged
thomasjpfan merged 7 commits intoscikit-learn:masterfrom
annejeevan:fixing_doc_multiclass
Jun 6, 2020
Merged

[DOC] Fixing n_jobs doc param in multiclass.py#17489
thomasjpfan merged 7 commits intoscikit-learn:masterfrom
annejeevan:fixing_doc_multiclass

Conversation

@annejeevan
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Reference Issue #14228

What does this implement/fix? Explain your changes.

This fixes n_jobs param documentation in multiclass.py

Any other comments?

Submitted with @emdupre for the #DataUmbrella June sprint

Comment on lines +163 to +164
The number of jobs to use for the computation, where each job computes
a classifier in parallel.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for the PR @annejeevan

I feel like the current formulation may suggest that the internal implementation of the classifier is parallelized which is usually not the case

How about

The number of jobs to use for the computation: the n_classes one-vs-rest problems are computed in parallel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback @NicolasHug .
Yes, it looks like. Sure. That looks good. I will try to modify the doc string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @NicolasHug, I have modified accordingly. Can you please check the PR? Thanks!


n_jobs : int or None, optional (default=None)
The number of jobs to use for the computation.
The number of jobs to use for the computation: the n_classes
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
The number of jobs to use for the computation: the n_classes
The number of jobs to use for the computation: the `n_classes`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @NicolasHug, I have modified accordingly. Can you please check the PR? Thanks!


n_jobs : int or None, optional (default=None)
The number of jobs to use for the computation.
The number of jobs to use for the computation: the n_classes
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case there are n_classes * (n_classes - 1) / 2 OVO problems

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @NicolasHug, I have modified accordingly. Can you please check the PR? Thanks!

Comment on lines +738 to +739
The number of jobs to use for the computation: the code_size
Output-Code multiclass problems are computed in parallel.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks like the actual number is tricky here so let's just go with "the multiclass problems are computed in parallel."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @NicolasHug, I have modified accordingly. Can you please check the PR? Thanks!

n_jobs : int or None, optional (default=None)
The number of jobs to use for the computation, where each job computes
a classifier in parallel.
The number of jobs to use for the computation: the n_classes one-vs-rest problems are computed in parallel.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs to rewrap to make sure we are under 80 characters per line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure @thomasjpfan. I think I have updated it already. Can you please verify the new commit.

Copy link
Copy Markdown
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.

LGTM Thank you @emdupre and @annejeevan !

@thomasjpfan thomasjpfan merged commit d6ed0d0 into scikit-learn:master Jun 6, 2020
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
* Fixing n_jobs doc param in multiclass.py

* Fixing n_jobs doc param in multiclass.py

* Fixing n_jobs doc param in multiclass.py

* Better documenting n_jobs param in multiclass

* Better documenting n_jobs param in multiclass

* Better documenting n_jobs param in multiclass

* Better documenting n_jobs param in multiclass
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
* Fixing n_jobs doc param in multiclass.py

* Fixing n_jobs doc param in multiclass.py

* Fixing n_jobs doc param in multiclass.py

* Better documenting n_jobs param in multiclass

* Better documenting n_jobs param in multiclass

* Better documenting n_jobs param in multiclass

* Better documenting n_jobs param in multiclass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants