[MRG] Fix missing assert and parametrize some k-means tests #12368
Conversation
| km = KMeans(init=centers.copy(), n_clusters=n_clusters, random_state=42, | ||
| n_init=1) | ||
| km.fit(X) | ||
| @pytest.mark.parametrize('representation', ['dense', 'sparse']) |
There was a problem hiding this comment.
Why not directly
@pytest.mark.parametrize('data', [X, X_csr])
There was a problem hiding this comment.
It's just for the readability when you run pytest.
With your proposition tests will appear as
test_whatever_test_name[data0]
test_whatever_test_name[data1]
Here it will appear as
test_whatever_test_name[dense]
test_whatever_test_name[sparse]
I just find it easier to track which parameters make some test fail.
There was a problem hiding this comment.
FYI, it's possible to provide ids as a workaround,
@pytest.mark.parametrize('data', (X, Xcsr), ids=('dense', 'sparse'))maybe that's a bit more direct?
There was a problem hiding this comment.
I didn't know that. This is better ! Thanks
| km = KMeans(init=centers.copy(), n_clusters=n_clusters, random_state=42, | ||
| n_init=1) | ||
| km.fit(X) | ||
| @pytest.mark.parametrize('representation', ['dense', 'sparse']) |
There was a problem hiding this comment.
FYI, it's possible to provide ids as a workaround,
@pytest.mark.parametrize('data', (X, Xcsr), ids=('dense', 'sparse'))maybe that's a bit more direct?
|
|
||
| # sanity check: predict centroid labels | ||
| pred = mb_k_means.predict(mb_k_means.cluster_centers_) | ||
| assert_array_equal(pred, np.arange(n_clusters)) | ||
|
|
||
| # check that models trained on sparse input also works for dense input at | ||
| # predict time | ||
| assert_array_equal(mb_k_means.predict(X), mb_k_means.labels_) |
There was a problem hiding this comment.
I moved it in a new function : test_predict_minibatch_dense_sparse.
|
|
||
| # sanity check: predict centroid labels | ||
| pred = mb_k_means.predict(mb_k_means.cluster_centers_) | ||
| assert_array_equal(pred, np.arange(n_clusters)) |
There was a problem hiding this comment.
Should we keep these 2 lines as well?
There was a problem hiding this comment.
in test_predict_minibatch, line 559-560
There was a problem hiding this comment.
LGTM, true I missed those :)
Thanks @jeremiedbb and thank you for the review @NicolasHug !
|
BTW, Circle CI doesn't seem to be triggering now. https://status.circleci.com/ looks fine, so I'm not sure what happened. Anyway it should not affect this PR. |
…learn#12368)" This reverts commit 347c272.
…learn#12368)" This reverts commit 347c272.
Noticed a missing
assertin k-means tests, meaning the test would always pass.I took the opportunity to parametrize some of the k-means test. I did not make any changes to the tests, just avoided code redundancy. I was doing it in #11950 but it will be more reviewable if I do it here, in a separate PR.