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.

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

Metric duck typing for Yin Yang #91

Closed
wants to merge 0 commits into from
Closed

Metric duck typing for Yin Yang #91

wants to merge 0 commits into from

Conversation

cstich
Copy link

@cstich cstich commented Jun 15, 2020

Elkan, Lloyd, and Hamerly all have duck typing for their metric argument, whereas Yin Yang only accepts Euclidean as a distance metric. This tiny pull requests brings the API for Yin Yang in line with Elkan and the others.

I am aware that strictly speaking the convergence of KMeans is only guaranteed for the Euclidean distance, but if there is a reason for only allowing Euclidean for Yin Yang and not the others, that is not clear to me.

@PyDataBlog PyDataBlog requested a review from Arkoniak June 15, 2020 12:29
@Arkoniak
Copy link
Collaborator

The main reason, why we have this limitation for YinYang is that in current state it will produce wrong results for any non-euclidean metric. Main internal functions, such as chunk_update_centroids or point_all_centers! have Square Euclidean logic embedded in them, for example, https://github.com/PyDataBlog/ParallelKMeans.jl/blob/master/src/yinyang.jl#L268

metric argument was added purely for compatibility with other algorithms, but proper removing of Eucledian restriction requires sufficient refactoring of the algorithm as well.

Thanks for bringing attention, I opened #92

@cstich
Copy link
Author

cstich commented Jun 15, 2020

That makes sense. Thanks for the explanation.

@cstich cstich closed this Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants