[MRG] Create public kmeans_plusplus including index output #17937

g-walsh · Jul 16, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Implement a public kmeans_plusplus function so that it can be used more widely (as explained in #17915) and for use as initialisation outside of KMeans (for example for gmm in #11101)

Any other comments?

Still very much a WIP but looking for quick review/suggestions on this before much further is done. It's currently a function in _kmeans.py but unsure if this is an appropriate implementation.

Still todo:

Docstrings (essentially the same as the private _k_init ?)
Tests
Full documentation and examples

jeremiedbb

Thanks @g-walsh, looking good. I made a first pass of comments.

I think we can also rename it to be more explicit, like _kmeans_plusplus.
It will require some tests.

Also making it public also requires adding it to the doc, especially in doc/modules/classes.rst.
There's also a little paragraph about k-means++ in doc/module/clustering.rst, in KMeans' section. We could mention the kmeans_plusplus function here.

sklearn/cluster/_kmeans.py

g-walsh · Jul 17, 2020

Thanks for the review

I think we can also rename it to be more explicit, like _kmeans_plusplus.

@jeremiedbb just to be clear you mean renaming _k_init to _kmeans_plusplus?

One question for the docstrings, are you happy for them to be essentially the same text for both _k_init and kmeans_plusplus?

I'll work on the testing and full documentation/examples next.

jeremiedbb · Jul 17, 2020

just to be clear you mean renaming _k_init to _kmeans_plusplus?

yes :)

One question for the docstrings, are you happy for them to be essentially the same text for both _k_init and kmeans_plusplus?

It should yes, the exception being the type of the parameters. For instance, random_state doesn't have to be a RandomState instance but can also be an int.

sklearn/cluster/_kmeans.py

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

…eansplusplus_public

jeremiedbb

A new batch of comments.

sklearn/cluster/_kmeans.py

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

g-walsh · Jul 22, 2020

If you're happy with the test coverage then I'll next start working on adding the documentation and examples

jeremiedbb

Looks good. Just small comments.

sklearn/cluster/_kmeans.py

sklearn/cluster/tests/test_k_means.py

jeremiedbb · Sep 29, 2020

@g-walsh that's nice !

I'm not sure that it requires a example on its own but others might disagree, let's wait for the opinion of other reviewers.
It also needs an example in the docstring. We are trying to put examples in the docstring of each public function/class of sklearn. You could simply adapt this example:

scikit-learn/sklearn/cluster/_kmeans.py

Line 746 in f605021

Examples

other than that looks good to me, thanks !

…eansplusplus_public

ogrisel · Oct 20, 2020

I merged master to trigger a rebuild.

ogrisel

LGTM besides the following comments.

sklearn/cluster/tests/test_k_means.py

sklearn/cluster/_kmeans.py

sklearn/cluster/tests/test_k_means.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

jeremiedbb

lgtm also. @g-walsh please add a what's entry.

jeremiedbb · Nov 11, 2020

Thanks @g-walsh !

…xture Model PR#11101, focused on allowing sample-based and k-means++ based GMM initialization.. Also made the enhancements compatible with PRs scikit-learn#17937 and scikit-learn#20030.

…sian Mixture Model PR#11101, focused on allowing sample-based and k-means++ based GMM initialization.. Also made the enhancements compatible with PRs scikit-learn#17937 and scikit-learn#20030.

initial wip push for public kmeans plus plus

bae53c8

github-actions bot added the module:cluster label Jul 16, 2020

jeremiedbb reviewed Jul 17, 2020

View reviewed changes

Changes from review and add more parameter and data checks

5118b6c

jeremiedbb reviewed Jul 17, 2020

View reviewed changes

sklearn/cluster/_kmeans.py Outdated Show resolved Hide resolved

sklearn/cluster/_kmeans.py Outdated Show resolved Hide resolved

sklearn/cluster/_kmeans.py Outdated Show resolved Hide resolved

g-walsh and others added 8 commits July 20, 2020 16:05

Update sklearn/cluster/_kmeans.py

928dd27

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

78501e8

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Changes from review, add docstrings, remove dead link

fda7b47

Add initial tests

d4525a7

Merge branch 'master' of github.com:scikit-learn/scikit-learn into km…

3cb55bf

…eansplusplus_public

update _k_init after merge and fix linting error

f769a8c

last linting problem...

6747232

fix kmeansplusplus initialisation tracking of incides

9686d1c

jeremiedbb reviewed Jul 21, 2020

View reviewed changes

g-walsh and others added 11 commits July 22, 2020 09:24

Update sklearn/cluster/_kmeans.py

4ce57f8

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

fdaca67

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

92a5478

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

4bffce5

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

62bc503

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/tests/test_k_means.py

05b1404

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

896ae0f

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/_kmeans.py

aa71c89

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/tests/test_k_means.py

9273078

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Update sklearn/cluster/tests/test_k_means.py

9ae566e

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Updates for review and add test for data order

38b7a10

jeremiedbb reviewed Jul 22, 2020

View reviewed changes

Resolve outstanding comments

ade1b4c

Initial push of documentation and examples

6aeff59

glemaitre self-requested a review October 20, 2020 13:50

ogrisel requested review from ogrisel and removed request for glemaitre October 20, 2020 13:50

ogrisel changed the title ~~[WIP] Create public kmeans_plusplus including index output~~ [MRG] Create public kmeans_plusplus including index output Oct 20, 2020

Merge branch 'master' of github.com:scikit-learn/scikit-learn into km…

6943300

…eansplusplus_public

ogrisel approved these changes Oct 22, 2020

View reviewed changes

g-walsh and others added 3 commits October 27, 2020 10:33

Apply suggestions from code review

026e8af

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Add extra tests from review, example to docstring

6204bd8

Fix issues TypeErrors in output test

1501818

jeremiedbb approved these changes Nov 10, 2020

View reviewed changes

Add whats new enhancement

518389c

jeremiedbb merged commit 11f9742 into scikit-learn:master Nov 11, 2020

jeremiedbb mentioned this pull request Dec 11, 2020

Is it possible to make k++ not private method? #6959

Closed

jeremiedbb mentioned this pull request Mar 12, 2021

MNT Move kmeans_plusplus next to _kmeans_plusplus #19666

Merged

alceballosa mentioned this pull request Jun 26, 2021

Solve Gaussian Mixture Model sample/kmeans++-based initialisation merge conflicts #11101 #20408

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[MRG] Create public kmeans_plusplus including index output #17937

[MRG] Create public kmeans_plusplus including index output #17937

Uh oh!

Conversation

g-walsh commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

g-walsh commented Jul 17, 2020

Uh oh!

jeremiedbb commented Jul 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

g-walsh commented Jul 22, 2020

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremiedbb commented Sep 29, 2020

Uh oh!

ogrisel commented Oct 20, 2020

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

jeremiedbb commented Nov 11, 2020

Uh oh!

Uh oh!

g-walsh commented Jul 16, 2020 •

edited

Loading