Skip to content

TST Ignore Kmeans test failures on MacOS#12648

Merged
qinhanmin2014 merged 4 commits intoscikit-learn:masterfrom
qinhanmin2014:kmeans-test
Apr 30, 2019
Merged

TST Ignore Kmeans test failures on MacOS#12648
qinhanmin2014 merged 4 commits intoscikit-learn:masterfrom
qinhanmin2014:kmeans-test

Conversation

@qinhanmin2014
Copy link
Copy Markdown
Member

@qinhanmin2014 qinhanmin2014 commented Nov 22, 2018

See #12644 (comment)
Tag #12644 from 0.20.1 to 0.21

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Nov 22, 2018

I would rather submit this just to the 0.20.X branch and in the mean time try to actually fix the issue on master.

@qinhanmin2014
Copy link
Copy Markdown
Member Author

qinhanmin2014 commented Nov 22, 2018

Maybe we'll add MacOS CI in master before the fix and I guess it's not harmful to include it in master?
Apologies I'll be offline for several hours so feel free to take it if you don't want it in master.

@qinhanmin2014 qinhanmin2014 added this to the 0.20.1 milestone Nov 22, 2018
@qinhanmin2014 qinhanmin2014 changed the base branch from master to 0.20.X November 22, 2018 15:10
@qinhanmin2014 qinhanmin2014 changed the base branch from 0.20.X to master November 22, 2018 15:10
@amueller
Copy link
Copy Markdown
Member

amueller commented Nov 22, 2018

why skip instead of v-measure? But I don't have a strong opinion.

@qinhanmin2014
Copy link
Copy Markdown
Member Author

qinhanmin2014 commented Nov 22, 2018

We'll eventually fix the issue so I guess there's not so much difference. I use pytest.xfail because we don't need to rewrite the test after we fix it (I guess in this case, we should get identical labels).

@jeremiedbb
Copy link
Copy Markdown
Member

jeremiedbb commented Nov 22, 2018

I agree with @qinhanmin2014, fit_predict and fit.predict should give the same labels, not a permutation.
There is something that needs to be fixed in kmeans, not in the test. I think my kmeans PR fixes it. Can someone try on mac ?

@amueller
Copy link
Copy Markdown
Member

amueller commented Nov 22, 2018

Right now I'm mostly concerned about the failure on conda-forge: conda-forge/scikit-learn-feedstock#81

@rth
Copy link
Copy Markdown
Member

rth commented Nov 22, 2018 •
edited
Loading

Untagging from 0.20.1 since this same PR was merged to 0.20.X directly in #12651 as suggested by Olivier

That leaves more time to come up with a proper solution for it on master..

@rth rth modified the milestones: 0.20.1, 0.21 Nov 22, 2018
@qinhanmin2014 qinhanmin2014 mentioned this pull request Dec 19, 2018
Merged
@jnothman jnothman mentioned this pull request Apr 9, 2019
Closed
@qinhanmin2014
Copy link
Copy Markdown
Member Author

qinhanmin2014 commented Apr 10, 2019

Seems that the failure disappears. Let's reopen if the error persists.

@qinhanmin2014 qinhanmin2014 deleted the kmeans-test branch April 10, 2019 11:30
@qinhanmin2014 qinhanmin2014 restored the kmeans-test branch April 30, 2019 08:41
@qinhanmin2014 qinhanmin2014 reopened this Apr 30, 2019
@qinhanmin2014 qinhanmin2014 mentioned this pull request Apr 30, 2019
Closed
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Apr 30, 2019 via email

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Apr 30, 2019

Please note my comment in the code, @qinhanmin2014, and merge if happy.

@qinhanmin2014 qinhanmin2014 merged commit 17dff1a into scikit-learn:master Apr 30, 2019
@qinhanmin2014 qinhanmin2014 deleted the kmeans-test branch April 30, 2019 14:25
jnothman pushed a commit that referenced this pull request Apr 30, 2019
@qinhanmin2014 @jnothman
TST Ignore Kmeans test failures on MacOS (#12648)
6627503
ogrisel
ogrisel reviewed Apr 30, 2019
View reviewed changes
sklearn/cluster/tests/test_k_means.py
# which appears to be where it fails on some MacOS setups.
# NB: This test is largely redundant with respect to test_predict and
# test_predict_equal_labels. This test has the added effect of
# testing idempotence of the fittng procesdure which appears to
Copy link
Copy Markdown
Member

@ogrisel ogrisel Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

typo: "fittng procesdure" but fair enough :)

Copy link
Copy Markdown
Member

@jnothman jnothman Apr 30, 2019

Choose a reason for hiding this comment

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

I'm still not used to the keyboard on these new macbooks

marcelobeckmann pushed a commit to marcelobeckmann/scikit-learn that referenced this pull request May 1, 2019
@qinhanmin2014 @marcelobeckmann
TST Ignore Kmeans test failures on MacOS (scikit-learn#12648)
2b1a697
marcelobeckmann pushed a commit to marcelobeckmann/scikit-learn that referenced this pull request May 1, 2019
@qinhanmin2014 @marcelobeckmann
TST Ignore Kmeans test failures on MacOS (scikit-learn#12648)
9c09d9b
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@qinhanmin2014 @koenvandevelde
TST Ignore Kmeans test failures on MacOS (scikit-learn#12648)
47ae68a
@rth
Copy link
Copy Markdown
Member

rth commented Feb 21, 2020

@jeremiedbb can we revert this xfailed test with the new KMeans implementation?

@jeremiedbb
Copy link
Copy Markdown
Member

jeremiedbb commented Feb 21, 2020

I'd say probably, but there's still a possibility of failure because with elkan, the predict method still uses lloyd algo whereas in fit_predict it's elkan all the way.

I'm working on a refactoring of kmeans which will fix that. So I think we can keep the skip a liitle more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

@ogrisel ogrisel ogrisel left review comments

@amueller amueller amueller approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.21

Development

Successfully merging this pull request may close these issues.

6 participants

@qinhanmin2014 @ogrisel @amueller @jeremiedbb @rth @jnothman