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

Use slicing kernel to copy distances inside NN Descent #2380

Merged
merged 17 commits into from
Jul 22, 2024
Merged

Use slicing kernel to copy distances inside NN Descent #2380

merged 17 commits into from
Jul 22, 2024

Conversation

jinsolp
Copy link
Contributor

@jinsolp jinsolp commented Jul 12, 2024 •
edited
Loading

Description

This make use of raft's slicing kernel within NN Descent build.
I found that my previous implementation was inefficient (merged in this PR).

Improvements

Time to call NN Descent build() with return_distances=True before and after using this kernel

Dataset Before After
mnist (60000, 784) 1550ms 1020ms
sift (1M, 128) 11342ms 5546ms
gist (1M, 960) 13508ms 9278ms

@jinsolp jinsolp requested a review from a team as a code owner July 12, 2024 18:36
@github-actions github-actions bot added the cpp label Jul 12, 2024
This was referenced Jul 12, 2024
Open
Merged
@jinsolp jinsolp changed the title [IMPROVEMENT] Distance copy kernel inside NN Descent build Distance copy kernel inside NN Descent build Jul 17, 2024
@jinsolp jinsolp changed the title Distance copy kernel inside NN Descent build Use slicing kernel to copy distances inside NN Descent Jul 18, 2024
cjnolet
cjnolet approved these changes Jul 18, 2024
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.

LGTM!

@jinsolp
Copy link
Contributor Author

jinsolp commented Jul 18, 2024 •
edited
Loading

If we try to use the mdspan copy, we get errors compiling.
graph_.h_dists has index type size_t, and the mdspan raft::copy complains if graph_d_dists index type is not the same.
But then slice complains about meaningless comparison with 0 if the index type is not signed, which makes me have to set it to int64_t when getting the const view of graph_d_dists.

I think it would be better to use the raft::copy I was previously using to keep things neat: )! @lowener @cjnolet

@lowener lowener added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 22, 2024
@lowener
Copy link
Contributor

lowener commented Jul 22, 2024

/merge

@rapids-bot rapids-bot bot merged commit 706eb39 into rapidsai:branch-24.08 Jul 22, 2024
73 checks passed
@jinsolp jinsolp deleted the fea-dist-copy-kernel branch August 23, 2024 17:46
@divyegala divyegala mentioned this pull request Oct 21, 2024
Merged
rapids-bot bot pushed a commit to rapidsai/cuvs that referenced this pull request Nov 8, 2024
@divyegala
Migrate feature diff for NN Descent from RAFT to cuVS (#421)
29c0f5b 
This PR is an amalgamation of the diff of 3 PRs in RAFT:

1. rapidsai/raft#2345
2. rapidsai/raft#2380
3. rapidsai/raft#2403

This PR also addresses part 1 and 2 of #419, closes #391 and makes CAGRA use the compiled headers of NN Descent, which seemed to have been a pending TODO https://github.com/rapidsai/cuvs/blob/009bb8de03ce9708d4d797166187250f77a59a36/cpp/src/neighbors/detail/cagra/cagra_build.cuh#L36-L37

Also, batch tests are disabled in this PR due to issue rapidsai/raft#2450. PR #424 will attempt to re-enable them.

Authors:
  - Divye Gala (https://github.com/divyegala)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowener lowener lowener approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
cpp 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.
3 participants
@jinsolp @lowener @cjnolet