Skip to content

[MRG] docs for random_state in decomposition module#11902

Merged
glemaitre merged 6 commits intoscikit-learn:masterfrom
SirR4T:docs-for-random_state-decomposition
Jan 8, 2020
Merged

[MRG] docs for random_state in decomposition module#11902
glemaitre merged 6 commits intoscikit-learn:masterfrom
SirR4T:docs-for-random_state-decomposition

Conversation

@SirR4T
Copy link
Copy Markdown
Contributor

@SirR4T SirR4T commented Aug 23, 2018

Reference Issues/PRs

Working towards #10548 .

What does this implement/fix? Explain your changes.

This PR updates the docstrings for random_state parameter, adding more info where I understood the changes, and specifying that output should be reproducible when an int is passed (where applicable).

Any other comments?

Fixes only for decomposition module.

Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I like this generally. But I'm not sure about the wording of "reproducible output across multiple function calls"... I'd rather the more general "reproducible results across multiple calls" but maybe I'm being overly precise.

Update the wording for reproducible results.
@SirR4T
Copy link
Copy Markdown
Contributor Author

SirR4T commented Aug 30, 2018

agree with the wording, pushed updates.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Sep 1, 2018

  • , optional is kind of a numpydoc convention see this, I would keep it.
  • I am not too sure about the None (default), I have a slight preference for , optional (default=None). This seems to agree with the git grep popularity contest below.
(base) ❯ git grep -P '\w+ :.+optional.+\(default=' | wc -l
582

(base) ❯ git grep -P '\w+ :.+\(default\)' | wc -l                   
63

Other than this LGTM.

@SirR4T
Copy link
Copy Markdown
Contributor Author

SirR4T commented Sep 1, 2018

Sure, pushed updates.

@jnothman could we also update this comment here, so that we could have a template inline with all the suggestions? this could be helpful for other PR authors.

@glemaitre
Copy link
Copy Markdown
Member

I'm going to solve the conflict and merge with the new guideline. The PR is already in a good stage.

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Thanks @SirR4T. Again sorry for the delay.

@glemaitre
Copy link
Copy Markdown
Member

I will merge the test pass

@glemaitre glemaitre merged commit cd6e4d5 into scikit-learn:master Jan 8, 2020
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
…learn#11902)

* [MRG] Fix random_state docstrings in decomposition module

* Add more details, and specify reproducibility

* [MRG] docs for random_state in decomposition module

Update the wording for reproducible results.

* [MRG] Update doc for optional parameter to match numpy doc style

* apply new style

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
…learn#11902)

* [MRG] Fix random_state docstrings in decomposition module

* Add more details, and specify reproducibility

* [MRG] docs for random_state in decomposition module

Update the wording for reproducible results.

* [MRG] Update doc for optional parameter to match numpy doc style

* apply new style

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
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.

5 participants