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

matrix::select_k: move selection and warp-sort primitives #1085

Merged
merged 42 commits into from
Jan 23, 2023

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Dec 9, 2022

Refactor and move a set of implementations for batch-selecting top K largest/smallest values:

  • Move device warp-wide primitives bitonic_sort.cuh to the public raft::util namespace, add tests.
  • Create a new public matrix::select_k interface.
  • Deprecate the legacy public raft::spatial::knn::select_k interface.
  • Copy/adapt select_k tests.
  • Move/adapt select_k benchmarks.
  • Rework the internals of select_warpsort.cuh to enable more implementations.

Closes #853

@achirkin achirkin added improvement Improvement / enhancement to an existing function 2 - In Progress Currenty a work in progress non-breaking Non-breaking change labels Dec 9, 2022
@achirkin achirkin self-assigned this Dec 13, 2022
@cjnolet
Copy link
Member

cjnolet commented Dec 13, 2022

rerun tests

@achirkin
Copy link
Contributor Author

Note, this PR does not add any template specialization at this point. There are two reasons for this: (1) it's not clear to which component the specializations would belong, (2) the impact on the compile times needs to be investigated more thoroughly.

@achirkin achirkin marked this pull request as ready for review December 15, 2022 07:45
@achirkin achirkin requested review from a team as code owners December 15, 2022 07:45
@achirkin achirkin added 3 - Ready for Review and removed 2 - In Progress Currenty a work in progress labels Dec 15, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (dc3043c) compared to base (0e96662).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-23.02    #1085   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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 the changes and discussions thus far. I think this is almost ready. Mostly minor things at this point.

cpp/include/raft/matrix/select_k.cuh Show resolved Hide resolved
cpp/include/raft/matrix/select_k.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/knn.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/knn.cuh Show resolved Hide resolved
cpp/bench/matrix/select_k.cu Show resolved Hide resolved
@achirkin achirkin requested a review from cjnolet January 20, 2023 10:32
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.

I think we can continue the discussion to move code shared by test and bench in follow-ons in 23.04. I think this looks great for now!

@cjnolet
Copy link
Member

cjnolet commented Jan 21, 2023

/merge

@rapids-bot rapids-bot bot merged commit 0076101 into rapidsai:branch-23.02 Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

[FEA] Move the new select-top-k code to the matrix namespace
3 participants