Skip to content
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

[REVIEW] Hide silhouette_score Python binding due to memory issue #3258

Merged
merged 8 commits into from
Dec 5, 2020

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Dec 4, 2020

Remove silhouette_score Python binding due to memory issues when handling more than a modest number of samples

Remove this feature due to memory issues in C++ implementation for
anything but modest numbers of samples
@wphicks wphicks requested a review from a team as a code owner December 4, 2020 19:09
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@wphicks wphicks added breaking Breaking change bug Something isn't working labels Dec 4, 2020
@wphicks wphicks added the 3 - Ready for Review Ready for review by team label Dec 4, 2020
@wphicks wphicks changed the title Hide silhouette_score Python binding due to memory issue [REVIEW] Hide silhouette_score Python binding due to memory issue Dec 4, 2020
@wphicks
Copy link
Contributor Author

wphicks commented Dec 4, 2020

This is a temporary change due to #3255. This functionality will be restored once the C++ implementation is rewritten in a more memory-friendly way.

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

One small comment, and also need to remove the entry from https://github.com/rapidsai/cuml/blob/branch-0.17/docs/source/api.rst so that it doesn't appear in the docs

python/cuml/metrics/cluster/__init__.py Show resolved Hide resolved
@dantegd dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Dec 4, 2020
@wphicks wphicks added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Dec 4, 2020
@dantegd dantegd added 6 - Okay to Auto-Merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Dec 4, 2020
@wphicks wphicks added 4 - Waiting on Author Waiting for author to respond to review and removed 6 - Okay to Auto-Merge labels Dec 4, 2020
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Also, should update docstring per discussion.

CHANGELOG.md Show resolved Hide resolved
Also remove sillhouette_score from api.rst docs
@JohnZed JohnZed added 6 - Okay to Auto-Merge and removed 4 - Waiting on Author Waiting for author to respond to review labels Dec 4, 2020
@codecov-io
Copy link

codecov-io commented Dec 4, 2020

Codecov Report

Merging #3258 (9b3e8c3) into branch-0.17 (a4c8de5) will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #3258      +/-   ##
===============================================
- Coverage        71.49%   71.39%   -0.10%     
===============================================
  Files              206      205       -1     
  Lines            16648    16612      -36     
===============================================
- Hits             11902    11860      -42     
- Misses            4746     4752       +6     
Impacted Files Coverage Δ
python/cuml/metrics/cluster/__init__.py 100.00% <ø> (ø)
python/cuml/dask/preprocessing/label.py 36.06% <0.00%> (-2.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4c8de5...9b3e8c3. Read the comment docs.

@JohnZed JohnZed merged commit 40b089b into rapidsai:branch-0.17 Dec 5, 2020
divyegala added a commit to divyegala/cuml that referenced this pull request Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants