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

Use slicing kernel to copy distances inside NN Descent #2380

Merged
merged 17 commits into from
Jul 22, 2024

Conversation

jinsolp
Copy link
Contributor

@jinsolp jinsolp commented Jul 12, 2024

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
@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
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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants