Add K-means++ - #12976
Add K-means++#12976leolamien wants to merge 6 commits into
Conversation
|
Hi! |
|
@priya-sundaram-dev, please review. |
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @leolamien — I ran the code locally and the core K-means++ seeding works well: on a synthetic 3-cluster dataset it recovers all three centroids cleanly, and the D²-weighted seeding is implemented correctly. A few suggestions to bring it in line with the repo's conventions and make it more robust:
1. Add doctests to the algorithmic functions. Right now only report_generator has one. TheAlgorithms convention is that each function carries a small, deterministic doctest — and get_initial_centroids_kmeans_plus_plus, assign_clusters, revise_centroids, and compute_heterogeneity are all easy to test with a tiny fixed array + seed. For example:
>>> import numpy as np
>>> data = np.array([[0.0, 0.0], [0.0, 1.0], [10.0, 10.0], [10.0, 11.0]])
>>> centroids = get_initial_centroids_kmeans_plus_plus(data, 2, seed=42)
>>> centroids.shape
(2, 2)2. Empty-cluster robustness. If a cluster loses all its members during iteration, revise_centroids computes member_data_points.mean(axis=0) over an empty slice, which produces nan centroids (I reproduced this). That failure is currently hidden by the module-level warnings.filterwarnings("ignore"). I'd suggest removing the global warning filter (it silences all warnings for anyone importing this module) and instead guarding the empty case explicitly, e.g. keep the previous centroid when a cluster is empty.
3. Dead code. In get_initial_centroids_kmeans_plus_plus, probabilities = [] is assigned and then immediately overwritten a few lines later — it can be dropped.
4. Type hints. The repo requires type hints on function signatures; most functions here are missing them (data: np.ndarray, k: int, seed: int | None = None -> np.ndarray, etc.).
5. Scope. The if False: mock-test block and the large report_generator pandas utility aren't really part of the K-means++ algorithm and add a lot of surface area to the file. Consider replacing the if False: block with proper doctests, and either dropping report_generator or moving it out — a focused, self-contained algorithm file is easier to review and maintain. (Minor: the header lists Python: 3.5, which is well past EOL.)
None of this takes away from the algorithm itself, which is solid. Happy to re-review once the doctests/type hints are in.
(Disclosure: I'm an AI agent helping triage and review contributions; a human maintainer makes the final merge call.)
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @Leonce-Wilson — K-means++ seeding is a genuinely nice addition. I reviewed it carefully; a couple of things should be addressed before merge.
1. This file largely duplicates machine_learning/k_means_clust.py
Nine of the ten functions here (get_initial_centroids, centroid_pairwise_dist, assign_clusters, revise_centroids, compute_heterogeneity, kmeans, the plot_* helpers, and the ~180-line report_generator) are copied almost verbatim from the existing k_means_clust.py. The only genuinely new logic is get_initial_centroids_kmeans_plus_plus.
Carrying two near-identical k-means implementations is a maintenance hazard (fixes/bugs have to be kept in sync). I'd suggest one of:
- add just
get_initial_centroids_kmeans_plus_plus()to the existingk_means_clust.pyand expose it as an optionalinit="k-means++"path, or - keep a small standalone module that contains only the k-means++ seeding function and imports the shared helpers from
k_means_clust.
Either way, please drop the copied report_generator — it's unrelated to k-means++.
2. Correctness bug: division by zero when D² sums to zero
When every remaining point already coincides with a chosen centroid (e.g. k greater than the number of distinct points, or duplicate/identical rows), np.sum(squared_distances) is 0, so probabilities becomes all-NaN and rng.choice raises. Reproducer:
import numpy as np
dup = np.array([[0.0, 0.0]] * 5 + [[1.0, 1.0]] * 5) # only 2 distinct points
get_initial_centroids_kmeans_plus_plus(dup, 4, seed=1)
# ValueError: probabilities contain NaNMinimal guard (falls back to a uniform draw when all D² are zero):
total = np.sum(squared_distances)
probabilities = (
np.full(n, 1.0 / n) if total == 0 else squared_distances / total
)I confirmed this makes both the k > #distinct and all-identical cases return cleanly.
3. Minor
probabilities = []at the top of the loop is dead — it's immediately overwritten. Remove it.- Module-level
warnings.filterwarnings("ignore")hides warnings globally for anyone importing this file; better to not suppress, or scope it narrowly. - The new seeding logic has no doctest, so CI never exercises it. A tiny deterministic doctest (fixed
seed=, well-separated points, asserting shape / that the two far clusters are picked) would lock in behavior — the repo's contributing guide also asks for doctests + type hints on new functions.
Happy to re-review once the duplication is trimmed and the zero-distance guard is in. Nice work getting the D² weighting right.
Describe your change:
Adding K-means++ algorithm based on the existing naive K-means algorithm (k_means_clust.py)
Checklist: