-
Notifications
You must be signed in to change notification settings - Fork 187
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
KNN select-top-k variants #551
KNN select-top-k variants #551
Conversation
691362a
to
02dcaec
Compare
76b1a8b
to
3eab24b
Compare
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.
Hi Artem, thanks for this PR! Although it is still a work in progress, I think it makes sense to give you an early feedback. Here are my first batch of comments.
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.
Hi Artem, here is a second batch of comments.
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 have went through the raidx top-k kernels, here are my comments.
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.
Hi Artem, I have finished my first review round. Overall it looks good, but as my comments indicate, some more documentation is required at certain places.
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 Artem for addressing the issues and improving the documentation! I had second look at the bitonic sort, here are a few additional comments.
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.
Hi Artem, I had a second look on radix_topk. It looks good overall, just found a few minor things.
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 have finished a second review. Please find a few more suggestions for improving the docs. Apart from these pending items, the PR looks good to me.
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
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 Artem for addressing the issues! Apart from two more nitpicks, the PR looks good to me. Pre-approving.
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 Artem! Just one little thing from me this time around. I don't see any obvious things in these changes that would break cuml downstream but a PR should be opened and pinned to this branch before merging just to make sure.
rerun tests |
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
@gpucibot merge |
Integrate two new implementations for knn's
select_k
function.