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

[MRG+1] break the tie in Meanshift in case cluster intensities are the same #11901

Merged
merged 7 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@adrinjalali
Contributor

adrinjalali commented Aug 23, 2018

See MacPython/scikit-learn-wheels#7 (comment)

MeanShift is undeterministic if intensities of clusters are the same. This uses the cluster center values to sort in case of a tie.

adrin jalali added some commits Aug 23, 2018

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 23, 2018

Member
Member

jnothman commented Aug 23, 2018

@adrinjalali

This comment has been minimized.

Show comment
Hide comment
@adrinjalali

adrinjalali Aug 23, 2018

Contributor

yeah, I added a test for that case. Which is basically the example which was failing.

Contributor

adrinjalali commented Aug 23, 2018

yeah, I added a test for that case. Which is basically the example which was failing.

adrin jalali
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 23, 2018

Member

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user: please also add MeanShift to the changed models section up top

Member

jnothman commented Aug 23, 2018

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user: please also add MeanShift to the changed models section up top

adrin jalali
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 23, 2018

Member

This doesn't lead to a failure right now so isn't critical, right?

Member

amueller commented Aug 23, 2018

This doesn't lead to a failure right now so isn't critical, right?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 24, 2018

Member

Well it was leading to a failure. Did they change?

Member

jnothman commented Aug 24, 2018

Well it was leading to a failure. Did they change?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 24, 2018

Member
Member

jnothman commented Aug 24, 2018

@adrinjalali adrinjalali changed the title from break the tie in Meanshift in case cluster intensities are the same to [MRG+1] break the tie in Meanshift in case cluster intensities are the same Aug 28, 2018

@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Sep 5, 2018

Member

LGTM as well, indeed it's useful to merge for 0.20 because of #12014.

Member

ogrisel commented Sep 5, 2018

LGTM as well, indeed it's useful to merge for 0.20 because of #12014.

@ogrisel ogrisel merged commit a9c6ad9 into scikit-learn:master Sep 5, 2018

7 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 92.89%)
Details
codecov/project 92.89% (+<.01%) compared to 2fe58e5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adrinjalali adrinjalali deleted the adrinjalali:meanshift branch Sep 5, 2018

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Sep 6, 2018

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Sep 17, 2018

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