Loosened to dist <= stop_thresh to converge in on 1D constant data
#28951
Conversation
|
Thanks for the PR @akikuno. Please add a non regression test in |
| # Test convergence using 2D constant data | ||
| x = np.concatenate([np.zeros((10, 10)), np.ones((10, 10))]) | ||
| n_iter = MeanShift().fit(x).n_iter_ | ||
| assert n_iter < 300 |
There was a problem hiding this comment.
I would remove the 2d case. The 1d case is enough as non-regression test.
There was a problem hiding this comment.
@ogrisel
Thank you so much for all your guidance! I have learnt a lot.
There was a problem hiding this comment.
@akikuno This comment has not been addressed yet.
There was a problem hiding this comment.
@ogrisel
Sorry, I mistakenly thought it had already been changed.
I have now removed the 2d case.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
doc/whats_new/v1.5.rst
Outdated
| - |Efficiency| The `clustering.MeanShift` class has now improved computational speed as it properly converges for constant data. | ||
| :pr:`28951` by :user:`Akihiro Kuno <akikuno>`. | ||
|
|
There was a problem hiding this comment.
I would consider it a bug and move that in the cluster section of the changelog.
There was a problem hiding this comment.
@jeremiedbb
Thanks for your updates! I have moved the log to the sklearn.cluster section.
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Reference Issues/PRs
Discussed #28926
What does this implement/fix? Explain your changes.
As @ogrisel suggested, I implemented the condition
dist <= stop_threshin the_mean_shift_single_seedfunction to address the issue ofMeanShiftfailing to converge on 1D constant data within 300 iterations.Any other comments?