Skip to content

TST enable tests K-means for MacOS #21052

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

Merged
merged 8 commits into from
Sep 15, 2021

Conversation

glemaitre
Copy link
Member

Check if the tests are still failing in MacOS

@glemaitre glemaitre marked this pull request as draft September 15, 2021 11:19
@jeremiedbb
Copy link
Member

in test_predict

    # With n_init > 1
    # Due to randomness in the order in which chunks of data are processed when
    # using more than one thread, there might be different rounding errors for
    # the computation of the inertia between 2 runs. This might result in a
    # different ranking of 2 inits, hence a different labeling, even if they
    # give the same clustering. We only check the labels up to a permutation.

This should have been fixed by #20200. Would you mind trying to replace assert_allclose(v_measure_score(pred, labels), 1) by assert_array_equal(pred, labels) ?

@glemaitre glemaitre marked this pull request as ready for review September 15, 2021 12:40
@@ -804,8 +750,8 @@ def test_k_means_function():
assert cluster_centers.shape == (n_clusters, n_features)
assert np.unique(labels).shape[0] == n_clusters

# check that the labels assignment are perfect (up to a permutation)
assert_allclose(v_measure_score(true_labels, labels), 1.0)
Copy link
Member

Choose a reason for hiding this comment

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

For this one we don't expect the labels to match perfectly since the true labels are manually defined. We can only try to remove the v_measure_score when comparing 2 kmeans run that should be identical

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM if green.

Edit: if green on a new [cd build] commit.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @glemaitre !

@glemaitre glemaitre merged commit eff749a into scikit-learn:main Sep 15, 2021
@glemaitre
Copy link
Member Author

It is green then I will merge.

@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
glemaitre added a commit that referenced this pull request Oct 25, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants