Skip to content

[MRG+1] Fix bug in AffinityPropagation#13334

Merged
agramfort merged 2 commits intoscikit-learn:masterfrom
wdevazelhes:fix_affinity_propagation
Mar 1, 2019
Merged

[MRG+1] Fix bug in AffinityPropagation#13334
agramfort merged 2 commits intoscikit-learn:masterfrom
wdevazelhes:fix_affinity_propagation

Conversation

@wdevazelhes
Copy link
Copy Markdown
Contributor

This PR introduces a fix and a non regression test on a bug of AffinityPropagation (see #13246, and comment https://github.com/scikit-learn/scikit-learn/pull/13246/files#r260758475)
.size on a sparse array of zeros returned 0 doing as if the array was empty which it was not

@wdevazelhes wdevazelhes changed the title Fix bug in AffinityPropagation [WIP] Fix bug in AffinityPropagation Feb 28, 2019
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Have you changed it back to WIP because Windows failed? It looks like something in the system, not your code. This looks fine.

def test_affinity_propagation_convergence_warning_dense_sparse(centers):
"""Assert that no convergence warning is printed for the sparse case and
the dense case if we have clusters, even if these clusters are zeros (non
regression test because of a bug that raised the warning for sparse
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just say "non-regression, see #13334"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Allright, thanks
Done

@wdevazelhes
Copy link
Copy Markdown
Contributor Author

Have you changed it back to WIP because Windows failed? It looks like something in the system, not your code. This looks fine.

Yes exactly, good, I'll set it in MRG as soon as I address your comment

@wdevazelhes wdevazelhes changed the title [WIP] Fix bug in AffinityPropagation [MRG] Fix bug in AffinityPropagation Mar 1, 2019
@wdevazelhes
Copy link
Copy Markdown
Contributor Author

I addressed your comments @jnothman, tell me how that sounds

wdevazelhes pushed a commit to wdevazelhes/scikit-learn that referenced this pull request Mar 1, 2019
@agramfort agramfort changed the title [MRG] Fix bug in AffinityPropagation [MRG+1] Fix bug in AffinityPropagation Mar 1, 2019
@wdevazelhes
Copy link
Copy Markdown
Contributor Author

all checks are green @agramfort @jnothman

@agramfort agramfort merged commit ac79dff into scikit-learn:master Mar 1, 2019
@agramfort
Copy link
Copy Markdown
Member

thx @wdevazelhes

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
* Fix bug in AffinityPropagation

* address joel's review
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
* Fix bug in AffinityPropagation

* address joel's review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants