Skip to content

DOC improve random_state docstring random modue #15576

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 4 commits into from
Jan 9, 2020
Merged

DOC improve random_state docstring random modue #15576

merged 4 commits into from
Jan 9, 2020

Conversation

happilyeverafter95
Copy link
Contributor

Reference Issues/PRs

Addresses part of #15222

What does this implement/fix? Explain your changes.

This PR changes the documentation for random_choice_csc by documenting which parts of the algorithm was randomized.

Any other comments?

@jnothman
Copy link
Member

jnothman commented Nov 10, 2019 via email

@jnothman jnothman mentioned this pull request Jan 7, 2020
Closed
74 tasks
@cmarmo
Copy link
Contributor

cmarmo commented Jan 8, 2020

@jnothman, @NicolasHug , I think that @happilyeverafter95 modified this file picking it from the list available in #15222. This list is automatically generated and I'm cleaning it for the next sprints. If you all agree that this file should not be touched I will remove it from the new list, please let us know. Anyway thanks Mandy for your work! And sorry for our late reaction.

@NicolasHug
Copy link
Member

i haven't checked the correctness of the comment, but I don't see why we shouldn't also update private utils like this one

@glemaitre
Copy link
Member

I agree with both points raised by @jnothman and @NicolasHug. Let's focus on public estimators first.

@glemaitre
Copy link
Member

@happilyeverafter95 Feel free to pick up any other docstring from the list (which is an estimator).
Sorry to not have been more reactive.

@glemaitre glemaitre closed this Jan 8, 2020
@NicolasHug
Copy link
Member

Sorry if my comment wasn't clear, but I was suggesting that the PR is IMO relevant and the changes should be considered

@glemaitre
Copy link
Member

Oh no, it is me that did not read correctly your comment. Sorry about that. Let's reopen and merge

@glemaitre glemaitre reopened this Jan 9, 2020
glemaitre
glemaitre approved these changes Jan 9, 2020
View reviewed changes
Copy link
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.

This can be merged when CIs are green.

@glemaitre glemaitre changed the title Documentation for random_state in random.py DOC improve random_state docstring random modue Jan 9, 2020
@glemaitre glemaitre merged commit eb12449 into scikit-learn:master Jan 9, 2020
keyianpai added a commit to keyianpai/scikit-learn that referenced this pull request Jan 11, 2020
@keyianpai
DOC improve random_state docstring model selection modue (scikit-lear…
c6f2751 
…n#15576)
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
@happilyeverafter95 @thomasjpfan
DOC improve random_state docstring random modue (scikit-learn#15576)
8af73f8
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
@happilyeverafter95
DOC improve random_state docstring random modue (scikit-learn#15576)
4662d7f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@happilyeverafter95 @jnothman @cmarmo @NicolasHug @glemaitre