Skip to content

DOC link to example explaining init usage in KMeans #26981

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 16 commits into from
Aug 12, 2023

Conversation

cynthias13w
Copy link
Contributor

Reference Issues/PRs

Towards #26927

What does this implement/fix? Explain your changes.

Added a link to https://scikit-learn.org/stable/modules/generated/sklearn.cluster.KMeans.html

Any other comments?

cc @adrinjalali @glemaitre

Thank you ☺️🙏

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

✔️ Linting Passed

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

Generated for commit: 3d10de2. Link to the linter CI: here

@glemaitre glemaitre changed the title DOC added example link DOC link to example explaining init usage in KMeans Aug 2, 2023
@glemaitre glemaitre assigned glemaitre and unassigned glemaitre Aug 2, 2023
@glemaitre glemaitre self-requested a review August 2, 2023 08:17
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.

A couple of improvements

@@ -1240,6 +1240,10 @@ class KMeans(_BaseKMeans):
If a callable is passed, it should take arguments X, n_clusters and a
random state and return an initialization.

Copy link
Member

Choose a reason for hiding this comment

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

I think that I would like to use item for the different options above (using -). It will make it obvious which part of the discussion is about the options for init and which part is to go further (the new link to the example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Guillaume, thank you for reviewing😄
What do you mean by 'using -'?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by 'using -'?

Transforming the sentence into a list, e.g.

This, that, and everything

vs

- This
- That
- Everything

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood the original suggestion correctly, I believe it was more intended to separate "If an array is passed..." and "If a callable is passed..." as separate list entries, as opposed to the inputs of the callable that is now listed.

Copy link
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 your reply!
I modified it.

Copy link
Contributor

@Micky774 Micky774 Aug 8, 2023

Choose a reason for hiding this comment

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

Lines underneath a list symbol * that contain text to be included in the body of the list entry need to be indented with two spaces (I think. May be four spaces?). The formatting is a bit tricky sometimes. I still struggle with it myself😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
I fixed the indentation 😊

cynthias13w and others added 2 commits August 12, 2023 10:35
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre glemaitre merged commit 9376b78 into scikit-learn:main Aug 12, 2023
@glemaitre
Copy link
Member

LGTM Thanks @cynthias13w Merging.

TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023
…6981)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
akaashpatelmns pushed a commit to akaashp2000/scikit-learn that referenced this pull request Aug 25, 2023
…6981)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
…6981)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
…6981)

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.

3 participants