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

Migrate BFKNN from raft #118

Merged
merged 4 commits into from
May 17, 2024
Merged

Conversation

benfred
Copy link
Member

@benfred benfred commented May 14, 2024

No description provided.

@benfred benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 14, 2024
@benfred benfred requested review from a team as code owners May 14, 2024 19:12
@benfred benfred marked this pull request as draft May 14, 2024 19:12
@benfred benfred marked this pull request as ready for review May 15, 2024 03:53
* the dataset. If the dataset is in host memory, it will be copied to the device and the
* index will own the device memory.
*/
template <typename data_accessor>
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure we are instantiating these for the relevant supported types so they are only being compiled once.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed this data_accessor template - and changed to having seperate constructors for host/device datasets , with the implementation in the /src/ directory - since I think its a bit cleaner like this

@@ -18,8 +18,11 @@

#include "ann_types.hpp"
#include <cuvs/neighbors/ann_types.hpp>
#include <raft/core/copy.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I believe copy brings in device code, I think. Either way, we should break these index files apart into strict interface and compile a source file for the implementation. Then we don't have to worry about exposing anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

good callout - I've moved the implementations for the update_dataset code and the constructors to the src/ directory in the last two commits.

I've left the implementations for the getters in the header file since they are trivial, and can be inlined by the compiler then - but can move to src if you think thats better

@cjnolet
Copy link
Member

cjnolet commented May 17, 2024

/merge

@rapids-bot rapids-bot bot merged commit 36ce0ff into rapidsai:branch-24.06 May 17, 2024
57 checks passed
@benfred benfred deleted the migrate_bfknn branch June 14, 2024 20:17
difyrrwrzd added a commit to difyrrwrzd/cuvs that referenced this pull request Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants