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

feat: method for clustering new data kmeans added #238

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

feat: method for clustering new data kmeans added #238

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
82e9ab2
feat: method for clustering new data kmeans added
davidbp Oct 28, 2022
9cccfb2
Update test/kmeans.jl
davidbp Oct 28, 2022
32cc28e
Update src/kmeans.jl
davidbp Oct 28, 2022
86bc032
Update src/kmeans.jl
davidbp Oct 28, 2022
f33eedd
refactor: update docstring
davidbp Oct 28, 2022
546805d
refactor: move assign clusters to utils
davidbp Oct 28, 2022
a11a636
fix: inputs to cluster_assignment
davidbp Oct 28, 2022
ca29e80
refactor: remove added lines
davidbp Oct 29, 2022
48a8c0e
test: add not implemented check
davidbp Apr 9, 2023
30c98f9
test: add test with pairwise and without it
davidbp Apr 9, 2023
022c2f7
refactor: use specific kmeans method within kmeans
davidbp Apr 9, 2023
467b4be
fix: runtest bad import
davidbp Apr 9, 2023
7d4b7d4
refactor: separate kwargs from other args in method description
davidbp Apr 11, 2023
d51e5d8
refactor: rewrite kwargs description in kmeans and utils
davidbp Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Clustering.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ module Clustering
Hclust, hclust, cutree,

# MCL
mcl, MCLResult
mcl, MCLResult,

# utils
assign_clusters

## source files

Expand Down
34 changes: 34 additions & 0 deletions src/utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,37 @@ function updatemin!(r::AbstractArray, x::AbstractArray)
end
return r
end


"""gi
assign_clusters(X::AbstractMatrix{<:Real}, R::ClusteringResult; ...) -> Vector{Int}
davidbp marked this conversation as resolved.
Show resolved Hide resolved

Assign the samples specified as the columns of `X` to the corresponding clusters from `R`.

# Arguments
- `X`: Input data to be clustered.
- `R`: Fitted clustering result.
davidbp marked this conversation as resolved.
Show resolved Hide resolved
"""
function assign_clusters(
Copy link
Member

Choose a reason for hiding this comment

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

There's some misunderstanding of how the generic assign_clusters() should be implemented.
In src/utils.jl (here) you should define the generic assign_clusters() method, which should throw "not implemented" exception, something like:

assign_clusters(X::AbstractMatrix, R::ClusteringResult; kwargs...) =
    error("assign_clusters(X, R::$(typeof(R))) not implemented")

Your current implementation can only work with R::KmeansResults, e.g. because it uses R.centers, which might be not available for any other ClusteringResults descendant, but also because assigning point to a cluster based on the distance to its center is valid only for the specific clustering types. You should move the best distance-based code you have here back to the src/kmeans.jl where you have originally put it, and use the more specific signature for it:

assign_clusters(X::AbstractMatrix, R::KMeansResult; distance::SemiMetric = SqEuclidean())

So in the end we will have the two implementations of the assign_clusters() method: the generic one, and the KMeans one, which would be automatically selected for R::KMeansResults, because its signature is more specific. For any clustering other than k-means the "not implemented" exception would be thrown by the generic method.

Pls let me know if you have any questions regarding this logic.

Copy link
Author

Choose a reason for hiding this comment

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

hopefully the new PRs adress this with a "fallback" implementation that returns not implemented (in utils.jl)

function assign_clusters(
    X::AbstractMatrix{T}, 
    R::ClusteringResult;
    distance::SemiMetric = SqEuclidean(),
    pairwise_computation::Bool = true) where {T} 

    if !(typeof(R) <: KmeansResult)
        throw(MethodError(assign_clusters,
              "NotImplemented: assign_clusters not implemented for R of type $(typeof(R))"))
    end

end

and a specific kmeans implementation (in kmeans.jl) that does the computation

X::AbstractMatrix{T},
R::ClusteringResult,
distance::SemiMetric = SqEuclidean()) where {T}
davidbp marked this conversation as resolved.
Show resolved Hide resolved

cluster_assignments = zeros(Int, size(X, 2))

Threads.@threads for n in axes(X, 2)
min_dist = typemax(T)
cluster_assignment = 0

for k in axes(R.centers, 2)
dist = distance(@view(X[:, n]), @view(R.centers[:, k]))
if dist < min_dist
min_dist = dist
cluster_assignment = k
end
end
cluster_assignments[n] = cluster_assignment
end
Copy link
Member

Choose a reason for hiding this comment

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

I have seen your benchmarks (thank you!). I'm still not sure what kind of BLAS you use, and how the numbers change as the features or the number of samples grow. Anyway, I still think it is out of Clustering.jl scope and should be addressed by Distances.jl. I suggest you show your benchmark results in Distances.jl via issues or discussions (making a reference to this PR) -- I suspect the other people may have come across the same issue.

I agree that in some cases the low memory footprint method should be preferred, but we cannot make it the default. I am also not a fan of implicit multi-threading: the user might be already calling assign_clusters() from the multi-threaded code, and your Threads.@threads for would be interfering with the anticipated threads allocation.
Ideally, the problem should be addressed in Distances.jl, and assign_clusters() could pass through the keyword argument to the Distances.pairwise() to specify the preferred implementation.

For now, to avoid blocking this PR, please use the pairwise()-based implementation. We should be able to address your particular situation in the later PRs once we will get the feedback from Distances.jl community.

Copy link
Author

@davidbp davidbp Apr 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Distances is not implementing the find ids of the closest vectors to some query vectors. We can either import NearestNeighbor.jl for this or simply add the method I suggested. I have added a boolean flag to choose the implementation, but maybe using a string would be better? So that future implementations might be added with 'sensible names' that tell the user what will happen underneath.


return cluster_assignments
end
7 changes: 7 additions & 0 deletions test/kmeans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,11 @@ end
end
end

@testset "get cluster assigments" begin
Copy link
Member

Choose a reason for hiding this comment

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

Please also add the testset to test/utils.jl (it would be the new file that should be included from runtests.jl before all others) testing that assign_clusters(.., R) throws "not implemented" exception for an arbitrary ClusteringResult object other than KmeansResult, e.g. for KMedoidsResult.

Copy link
Author

@davidbp davidbp Apr 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've added the test to cover the case assign_clusters does not have correct implementation for non kmeans ClusteringResult.

X = rand(5, 100)
R = kmeans(X, 10; maxiter=200)
reassigned_clusters = assign_clusters(X, R);
@test R.assignments == reassigned_clusters
end

end