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] KNN from RAFT #3603

Merged
merged 12 commits into from
Mar 30, 2021
Merged

[REVIEW] KNN from RAFT #3603

merged 12 commits into from
Mar 30, 2021

Conversation

viclafargue
Copy link
Contributor

@viclafargue viclafargue commented Mar 10, 2021 •
edited
Loading

Closes #3457

This PR switches cuML code to use brute-force KNN from RAFT.

@viclafargue viclafargue requested review from a team as code owners March 10, 2021 17:41
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Mar 10, 2021
@viclafargue viclafargue added 2 - In Progress Currenty a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 10, 2021
@viclafargue viclafargue mentioned this pull request Mar 11, 2021
Merged
@viclafargue viclafargue requested a review from a team as a code owner March 12, 2021 16:28
@github-actions github-actions bot added the CMake label Mar 12, 2021
@viclafargue viclafargue changed the title [WIP] KNN from RAFT [REVIEW] KNN from RAFT Mar 12, 2021
@viclafargue viclafargue added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Mar 12, 2021
@viclafargue
Copy link
Contributor Author

Would need approval and merge of raft:#117

cjnolet
cjnolet requested changes Mar 15, 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.

Very nice. I like the consolidation of allocator and stream into the handle argument for the trustworthiness score.

cpp/test/prims/knn.cu Outdated 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 Mar 16, 2021
@JohnZed JohnZed added the 0 - Blocked Cannot progress due to external reasons label Mar 16, 2021
@github-actions github-actions bot removed the Cython / Python Cython or Python issue label Mar 25, 2021
@viclafargue viclafargue added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 0 - Blocked Cannot progress due to external reasons 4 - Waiting on Author Waiting for author to respond to review labels Mar 25, 2021
@dantegd dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Mar 26, 2021
divyegala
divyegala approved these changes Mar 29, 2021
View reviewed changes
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Very nice and easy to follow changes, LGTM. This is targeted for branch-0.19 still, right?

cjnolet
cjnolet approved these changes Mar 29, 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.

Sorry Victor. I had originally requested changes on this PR and lost sight of it. LGTM

@viclafargue
Copy link
Contributor Author

viclafargue commented Mar 29, 2021 •
edited
Loading

Sorry Victor. I had originally requested changes on this PR and lost sight of it. LGTM

No problem thanks for the review.

Very nice and easy to follow changes, LGTM. This is targeted for branch-0.19 still, right?

Thanks, yes I think it should go into the 0.19 release.

@divyegala divyegala added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Mar 29, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Mar 30, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit aeda29b into rapidsai:branch-0.19 Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divyegala divyegala divyegala approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CMake improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Switch cuml code to use brute-force knn from RAFT
5 participants
@viclafargue @JohnZed @cjnolet @divyegala @dantegd