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

Integrate KNN implementation: ivf-flat #652

Merged
merged 140 commits into from
Jul 27, 2022

Conversation

achirkin
Copy link
Contributor

Integrate a new KNN implementation.

@achirkin achirkin requested review from a team as code owners May 16, 2022 13:32
@achirkin achirkin added enhancement New feature or request breaking Breaking change 2 - In Progress Currenty a work in progress and removed cpp CMake labels May 16, 2022
@achirkin achirkin self-assigned this May 16, 2022
@achirkin achirkin added feature request New feature or request and removed enhancement New feature or request labels May 16, 2022
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @mdoijade for preparing the initial version of this PR, and for @achirkin for continuing the work with RAFT integration! Since the PR is large, I am sharing my comments in smaller chunks, so that @achirkin can address them when he gets there.

cpp/include/raft/spatial/knn/ann.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

One more batch of comments.

cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_ivf_flat.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_utils.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_utils.cuh Outdated Show resolved Hide resolved
@achirkin achirkin requested a review from cjnolet July 21, 2022 07:14
achirkin added a commit to achirkin/cuml that referenced this pull request Jul 21, 2022
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.

Discussed further changes offline to get this in for 22.08.

@cjnolet
Copy link
Member

cjnolet commented Jul 21, 2022

rerun tests

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Jul 21, 2022

rerun tests

@tfeher
Copy link
Contributor

tfeher commented Jul 22, 2022

Thanks Artem for the updates! I had a look at the changes since my last review, and overall it looks good to me. I have some smaller comments (mostly about docs) we can address those in #747.

@cjnolet
Copy link
Member

cjnolet commented Jul 27, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jul 27, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e6c0fe2 into rapidsai:branch-22.08 Jul 27, 2022
rapids-bot bot pushed a commit that referenced this pull request Nov 17, 2022
…rim and improve performance for thin matrices (small `n_cols`) (#979)

This follows up on a discussion at #652 (comment). The main goal of this PR is to make this helper accessible as a raft primitive.

I also used the opportunity to look at the performance of this primitive, and have improved it for:

- Thin matrices: less than 32 threads per row with shuffle-based reductions.
- Thick matrices: cub-based reduction doing one row per block.

Here is an overview of the before/after performance on A100:

![2022-11-11_normalize_perf_float_int32](https://user-images.githubusercontent.com/17441062/201403965-bf68d368-b64b-4a1f-92f0-a5de03b9d1a8.png)

Authors:
  - Louis Sugy (https://github.com/Nyrio)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)
  - Corey J. Nolet (https://github.com/cjnolet)
  - Artem M. Chirkin (https://github.com/achirkin)

URL: #979
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress breaking Breaking change CMake cpp feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants