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.

Sign up for GitHub

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

Jump to bottom

Add MG weighted k-means #3959

Merged
merged 25 commits into from
Jun 29, 2021
Merged

Add MG weighted k-means #3959

merged 25 commits into from
Jun 29, 2021

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Jun 8, 2021

This PR adds support for MG weighted k-means and is a continuation of @akkamesh and @cjnolet work on PR #2126.

akkamesh and others added 20 commits April 22, 2020 16:55
@akkamesh
[kmeans] add support for mg weighted k-means
3e1b003
@akkamesh
[kmeans] update changelog
6463900
@cjnolet
Commiting for merge
b67c60f
@cjnolet
Merge branch 'enh-ext-weighted-kmeans' into enh-ext-mg-weighted-kmeans
822e92e 
Conflicts:
	CHANGELOG.md
@cjnolet
Fixing style
986ecfc
@cjnolet
Added a reasonable test for kmeans weights
2cecda7
@cjnolet
Adding more clusters
351f340
@cjnolet
Adding dask kmeans fit() tests for highly skewed distributions
de5b724
@cjnolet
Checking in tests
1545cab
@cjnolet
Fixing style issue
372b4d7
@cjnolet
Adding sample weight normalization
71997d3
@cjnolet
Merge branch 'branch-0.14' into enh-ext-mg-weighted-kmeans
d567a43 
Conflicts:
	python/cuml/test/dask/test_kmeans.py
	python/cuml/test/test_kmeans.py
@cjnolet
Fixing style
839d849
@lowener
Merge branch 'branch-21.06' into enh-ext-mg-weighted-kmeans
85855b2
@lowener
Add initial C++ weighted MG KMeans
f6acb47
@lowener
Fix changelog
0a8943b
@lowener
Add MG weight normalization
eb18ae8
@lowener
Merge branch 'branch-21.08' into enh-ext-mg-weighted-kmeans
155867d
@lowener
Cleanup unnecessary changes
3cb817a
@lowener
Fix imports
f16a86f
@lowener lowener requested review from a team as code owners June 8, 2021 07:41
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Jun 8, 2021
@lowener lowener added Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 8, 2021
@dantegd dantegd added this to PR-WIP in v21.08 Release via automation Jun 8, 2021
@dantegd dantegd added the 3 - Ready for Review Ready for review by team label Jun 8, 2021
cjnolet
cjnolet requested changes Jun 9, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Thanks for picking this one up. Overall it looks great but we do still have an issue to fix (see comment in the review).

cpp/src/kmeans/kmeans_mg_impl.cuh
@@ -620,6 +657,9 @@ void fit(const raft::handle_t &handle, const KMeansParams &params,
MLCommon::device_buffer<char> workspace(handle.get_device_allocator(),
stream);

// check if weights sum up to n_samples
checkWeights(handle, workspace, weight, stream);
Copy link
Member

Choose a reason for hiding this comment

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

Great you pulled this over from cumlprims! IIRC, the one remaining issue should be that the single-GPU k-means normalizes the weights in predict, however that will cause the multi-gpu version to normalize each partition individually since it's embarrassingly parallel.

The weights are being normalized globally in the Dask-based predict but the single-GPU predict is going to re-normalize them locally. The more straightforward path to fixing this might be to have the C++ predict() function accept a normalize_weights argument which defaults to true but we can have the multi-GPU predict function flip it off. The goal here is to eliminate the need for predict() to use the comms, because then it would no longer be able to execute embarassingly parallel.

v21.08 Release automation moved this from PR-WIP to PR-Needs review Jun 9, 2021
@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 Jun 10, 2021
@dantegd dantegd mentioned this pull request Jun 16, 2021
Closed
@lowener
Copy link
Contributor Author

lowener commented Jun 22, 2021

rerun tests

@lowener lowener requested a review from cjnolet June 22, 2021 20:34
cjnolet
cjnolet approved these changes Jun 23, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

v21.08 Release automation moved this from PR-Needs review to PR-Reviewer approved Jun 23, 2021
@cjnolet
Copy link
Member

cjnolet commented Jun 23, 2021

rerun tests

1 similar comment
@dantegd
Copy link
Member

dantegd commented Jun 29, 2021

rerun tests

@dantegd
Copy link
Member

dantegd commented Jun 29, 2021

Docstring fix identified in CI:

Generating docs for compound /workspace/cpp/include/cuml/cluster/kmeans_mg.hpp:49: error: The following parameter of ML::kmeans::opg::fit(const raft::handle_t &handle, const KMeansParams &params, const float *X, int n_samples, int n_features, const float *sample_weight, float *centroids, float &inertia, int &n_iter) is not documented:

@dantegd
Copy link
Member

dantegd commented Jun 29, 2021

@gpucibot merge

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@3887e32). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #3959   +/-   ##
===============================================
  Coverage                ?   85.46%           
===============================================
  Files                   ?      230           
  Lines                   ?    18116           
  Branches                ?        0           
===============================================
  Hits                    ?    15482           
  Misses                  ?     2634           
  Partials                ?        0
Flag Coverage Δ
dask 48.11% <0.00%> (?)
non-dask 77.73% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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 3887e32...7e09369. Read the comment docs.

@rapids-bot rapids-bot bot merged commit 166667b into rapidsai:branch-21.08 Jun 29, 2021
v21.08 Release automation moved this from PR-Reviewer approved to Done Jun 29, 2021
@lowener lowener deleted the enh-ext-mg-weighted-kmeans branch June 29, 2021 22:10
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@lowener
Add MG weighted k-means (rapidsai#3959)
e81da5c 
This PR adds support for MG weighted k-means and is a continuation of @akkamesh and @cjnolet work on PR rapidsai#2126.

Authors:
  - Micka (https://github.com/lowener)
  - https://github.com/akkamesh
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai#3959
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
4 - Waiting on Author Waiting for author to respond to review CUDA/C++ Cython / Python Cython or Python issue Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
v21.08 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lowener @cjnolet @dantegd @codecov-commenter @akkamesh