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

[REVIEW] Sparse KNN + UMAP Sparse Inputs #2836

Merged
merged 127 commits into from
Nov 19, 2020
Merged

[REVIEW] Sparse KNN + UMAP Sparse Inputs #2836

merged 127 commits into from
Nov 19, 2020

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Sep 17, 2020 •
edited
Loading

Closes #2750
Closes #2854
Closes #2105

Don't be alarmed by all the changes in this PR. This is branched from the Sparse KNN work. Once that PR is merged, the changes will be easier to review.

cjnolet added 30 commits July 24, 2020 15:05
@cjnolet
Adding brute force knn shell to sparse
4dbabf4
@cjnolet
Stubbing out algorithm flow
46b2f09
@cjnolet
Adding initial headers to wrapper
03dbdb8
@cjnolet
Performing idx batching
86ad78a
@cjnolet
Starting to full in cusparse calls
b50611c
@cjnolet
Checking in
33f7e6a
@cjnolet
Beginning to add selection kernel
0342eb4
@cjnolet
Finished header
a3ead77
@cjnolet
Updates. Need to finish populating merge buffer
db4ff22
@cjnolet
Using block select for selecting k and using 3-partition merge buffer
7752e8e
@cjnolet
Logic is just about done.
363b433
@cjnolet
Merge branch 'branch-0.15' into fea-016-sparse_knn
9d70a69
@cjnolet
Checking in changes. Need to swap out cuda 11 cusparse calls for cuda…
29e82af 
… 10.2 version
@cjnolet
Merge branch 'branch-0.15' into fea-016-sparse_knn
2ee8de2
@cjnolet
Everything is building. Need end-to-end test
6f9f6d5
@cjnolet
Running clang format
8c9dfb3
@cjnolet
Updating changelog
87478ad
@cjnolet
Using raft's cusparse_wrappers.h instead of cuml
1bb9c0e
@cjnolet
Merge branch 'imp-016-use_raft_cusparse_wrappers' into fea-016-sparse…
a65f378 
…_knn

Conflicts:
	cpp/src_prims/sparse/cusparse_wrappers.h
@cjnolet
Removing cuda11-required GEMM calls (commenting them out for now, wil…
fa2e64d 
…l swap them out shortly)
@cjnolet
Fixing clang style
8789378
@cjnolet
Separating distance computation from selection from general brute for…
1db4d4d 
…ce algorithm to make pieces more reusable
@cjnolet
Updating clang style
6054863
@cjnolet
Adding batcher to help ease batch state management
d83f523
@cjnolet
Fixing clang style
763d3c1
@cjnolet
MOre clang fixes
18b9891
@cjnolet
Merge branch 'branch-0.16' into fea-016-sparse_knn
82aa14b 
Conflicts:
	CHANGELOG.md
@cjnolet
IP distance is computed using search * index.T.
eabe273
@cjnolet
Making type template for value_t all the way through knn_merge_parts
9a9ada6
@cjnolet
Adding simple googletest for sparse pairwise dists. The transpose con…
6d45c91 
…version seems super expensive, but maybe it's necessary.
@cjnolet cjnolet mentioned this pull request Nov 6, 2020
Closed
JohnZed
JohnZed approved these changes Nov 10, 2020
View reviewed changes
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.

I think it's looking good! Maybe Thejaswi has some outstanding prims comments, but it looks solid to me

cpp/src/umap/knn_graph/algo.cuh Outdated Show resolved Hide resolved
@JohnZed JohnZed 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 Nov 10, 2020
@cjnolet
Copy link
Member Author

cjnolet commented Nov 12, 2020

Ping @teju85. Is this PR still awaiting your re-review? Do you have time to review it? If not, we might need to find someone else.

@teju85
Copy link
Member

teju85 commented Nov 12, 2020

Sorry for the delay in reviewing this. I should be done by tomorrow, @cjnolet .

teju85
teju85 requested changes Nov 12, 2020
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

There seems to be a stray python/debug_out file in this PR?

cpp/src/umap/knn_graph/algo.cuh Outdated Show resolved Hide resolved
cpp/src/umap/runner.cuh Outdated Show resolved Hide resolved
cpp/src/umap/runner.cuh Outdated Show resolved Hide resolved
cpp/src/umap/runner.cuh Outdated Show resolved Hide resolved
cpp/src/umap/umap.cu Outdated Show resolved Hide resolved
teju85
teju85 requested changes Nov 13, 2020
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

I'm done with the second pass (sorry again for the delay). I have some minor comments (and a couple questions).

cpp/src_prims/sparse/selection.cuh Outdated Show resolved Hide resolved
cpp/src_prims/sparse/distance.cuh Show resolved Hide resolved
cpp/src_prims/sparse/distance.cuh Show resolved Hide resolved
cpp/src_prims/sparse/utils.h Show resolved Hide resolved
@teju85 teju85 added 4 - Waiting on Author Waiting for author to respond to review and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Nov 13, 2020
@cjnolet cjnolet mentioned this pull request Nov 18, 2020
Merged
@cjnolet
Merge branch 'branch-0.17' into fea-016-umap_sparse_inputs
fa60ad4 
Conflicts:
	cpp/include/cuml/distance/distance_type.h
	python/cuml/common/base.pyx
	python/cuml/manifold/umap.pyx
	python/cuml/neighbors/nearest_neighbors.pyx
@cjnolet
Running clang format on distance.cuh
9e888c4
@cjnolet
Copy link
Member Author

cjnolet commented Nov 18, 2020

@teju85, I have implemented all of your review feedback. Mind giving it another look when you have a moment?

@teju85
Copy link
Member

teju85 commented Nov 18, 2020

Thank you @cjnolet for your patience. I'm in the middle of something right now. Give me a couple more hours and I should be able to get to this and finish today itself.

teju85
teju85 approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@cjnolet
Copy link
Member Author

cjnolet commented Nov 18, 2020

rerun tests

@cjnolet cjnolet added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Waiting on Author Waiting for author to respond to review labels Nov 19, 2020
@JohnZed
Copy link
Contributor

JohnZed commented Nov 19, 2020

rerun tests

@cjnolet cjnolet merged commit b205e8f into rapidsai:branch-0.17 Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viclafargue viclafargue viclafargue left review comments

@teju85 teju85 teju85 approved these changes

@JohnZed JohnZed JohnZed approved these changes

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CUDA / C++ CUDA issue Cython / Python Cython or Python issue feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Use index template param in UMAP [FEA] Sparse inputs to UMAP [FEA] Sparse KNN
6 participants
@cjnolet @GPUtester @teju85 @codecov-io @JohnZed @viclafargue