-
Notifications
You must be signed in to change notification settings - Fork 12
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
K-means++ #23
K-means++ #23
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
- Coverage 94.44% 93.15% -1.30%
==========================================
Files 12 14 +2
Lines 432 482 +50
==========================================
+ Hits 408 449 +41
- Misses 24 33 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
_, mask, _ = state | ||
return jnp.sum(mask) < n_clusters | ||
|
||
dists = cdist(X, X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesnt this get passed in? Kind of confusing and can lead to bugs potentially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, don't the inner functions have access to outer variables? I was thinking it would be fine since dists
is not being written to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea it all technically works, its just a matter of code style to prevent bugs. I would recommend avoiding using variables that are instantiated later in the file, and also giving code the opportunity to mutate a shared variable. this is a just a nit, dw about changing it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense! thx for the feedback, I'll change it when I can
Closes #21.