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

Enable distance return for NN Descent #2345

Merged
merged 31 commits into from
Jun 14, 2024

Conversation

jinsolp
Copy link
Contributor

@jinsolp jinsolp commented May 30, 2024

  • Enable NN Descent to return the distances array as well (previously only returning indices array)
    • Added a return_distances flag in index_params. When set to 1 (true), allocates a distance array to return.
  • Test for checking distances recall compared to naive knn

@jinsolp jinsolp requested a review from a team as a code owner May 30, 2024 21:31
@github-actions github-actions bot added the cpp label May 30, 2024
cpp/include/raft/neighbors/nn_descent_types.hpp Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent_types.hpp Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent_types.hpp Outdated Show resolved Hide resolved
@jinsolp jinsolp requested a review from divyegala May 31, 2024 22:19
cpp/include/raft/neighbors/detail/nn_descent.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent_types.hpp Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent_types.hpp Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent_types.hpp Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh Outdated Show resolved Hide resolved
Copy link

copy-pr-bot bot commented Jun 6, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jinsolp jinsolp force-pushed the fea-nndescent-api branch from 0e5b3d5 to 3f49752 Compare June 6, 2024 02:21
@jinsolp
Copy link
Contributor Author

jinsolp commented Jun 6, 2024

/ok to test

1 similar comment
@divyegala
Copy link
Member

/ok to test

@jinsolp jinsolp requested a review from divyegala June 10, 2024 16:21
cpp/include/raft/neighbors/detail/nn_descent.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/nn_descent.cuh Outdated Show resolved Hide resolved
@divyegala divyegala added feature request New feature or request breaking Breaking change labels Jun 13, 2024
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Pre-approving based on offline discussion

@jinsolp
Copy link
Contributor Author

jinsolp commented Jun 14, 2024

This is no longer a breaking change

@jinsolp jinsolp requested a review from divyegala June 14, 2024 17:10
@divyegala divyegala added non-breaking Non-breaking change and removed breaking Breaking change labels Jun 14, 2024
@divyegala
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 877644a into rapidsai:branch-24.08 Jun 14, 2024
69 checks passed
@jinsolp jinsolp changed the title [REVIEW] Enable distance return for NN Descent Enable distance return for NN Descent Jun 14, 2024
@jinsolp jinsolp deleted the fea-nndescent-api branch July 12, 2024 00:11
rapids-bot bot pushed a commit that referenced this pull request Jul 22, 2024
This make use of raft's slicing kernel within NN Descent build.
I found that my previous implementation was inefficient (merged in [this PR](#2345)).

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

Authors:
  - Jinsol Park (https://github.com/jinsolp)

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

URL: #2380
rapids-bot bot pushed a commit to rapidsai/cuvs that referenced this pull request Nov 8, 2024
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
Labels
cpp feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants