-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
TST Extend tests for scipy.sparse.*array
in sklearn/cluster/tests/test_k_means.py
#27179
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Please consider the following naming suggestion. I did not made suggestions everywhere because it was getting tedious to do it all view the GitHub web UI but I would rather avoid using _container
in variable names both to describe the name of the data type or constructor on the one hand (as used in other similar PRs) and the name of the data variable that results from this conversion.
You can take inspirations from my suggestions to resolve this ambiguity but this should be done systematically throughout the PR.
@ogrisel Thank you a lot! I will notify you when I finish naming issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from those small changes, LGTM.
Signed-off-by: Nurseit Kamchyev <bncer.ml@gmail.com>
Signed-off-by: Nurseit Kamchyev <bncer.ml@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Signed-off-by: Nurseit Kamchyev <bncer.ml@gmail.com>
Signed-off-by: Nurseit Kamchyev <bncer.ml@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Signed-off-by: Nurseit Kamchyev <bncer.ml@gmail.com>
@ogrisel please, take a look on changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I solved the conflict in the what's new.
@OmarManzoor do you want to have a second look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @bncer
Reference Issues/PRs
Towards #27090
What does this implement/fix? Explain your changes.
This PR should support
scipy.sparse.*array
in cluster.kmeans_plusplus module.Added tests cases of scipy's sparse array into
sklearn/cluster/tests/test_k_means.py
fileAny other comments?
Nope