Skip to content
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

N clusters medoids fix #129

Merged
merged 3 commits into from Oct 19, 2021
Merged

N clusters medoids fix #129

merged 3 commits into from Oct 19, 2021

Conversation

stefanhannie
Copy link
Contributor

I noticed a bug when using init == "random" for KMedoids. You'll at times not get the requested number of clusters. The issue is that the initial medoids are being randomly sampled with replacement. This sets the flag to not sample with replacement.

I've also added a test to ensure n_clusters is correct for all initializations / method combinations.

Here's a repro if you're curious:

kmedoids = sk.cluster.KMedoids(10, init="random", random_state=123)
kmedoids.fit(np.array([(x, x) for x in range(15)]))
assert len(np.unique(out.medoid_indices_)) == 10

TimotheeMathieu
TimotheeMathieu approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@TimotheeMathieu TimotheeMathieu left a comment •
edited

Choose a reason for hiding this comment

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

Looks good to me, thanks for the PR.
The checks should be fixed after merging with PR #131

@stefanhannie
Don't sample with replacement for random placement
dbb26dd 
You'll always have an inertia at least as good as not doing this; i.e.
picking distinct medoids only helps in the optimization.
@stefanhannie
Add test to ensure n_clusters
a0c2069 
It appears to only be the "random" initialization that has the bug, but
it's cheap to test all initializations + methods.
@stefanhannie
The initial medoids changed for determinism
11ed752 
We're now sampling differently so the inital deterministic sample is
different. This updates the test to use the new deterministic medoids.
@stefanhannie
Copy link
Contributor Author

Looks good to me, thanks for the PR. The checks should be fixed after merging with PR #131

You bet. Thanks for the review! I've rebased, so things should be good to merge once the tests finish.

@TimotheeMathieu TimotheeMathieu merged commit ba4855c into scikit-learn-contrib:main Oct 19, 2021
@stefanhannie stefanhannie deleted the n_clusters_medoids_fix branch October 19, 2021 19:12
@TimotheeMathieu TimotheeMathieu mentioned this pull request Jan 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimotheeMathieu TimotheeMathieu TimotheeMathieu 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.

None yet
2 participants
@stefanhannie @TimotheeMathieu