Skip to content

[FEA] expose python & C API for prefiltered brute force - #174

Merged
rapids-bot[bot] merged 25 commits into
NVIDIA:branch-24.08from
rhdong:rhdong/pbf-python
Jul 29, 2024
Merged

[FEA] expose python & C API for prefiltered brute force#174
rapids-bot[bot] merged 25 commits into
NVIDIA:branch-24.08from
rhdong:rhdong/pbf-python

Conversation

@rhdong

@rhdong rhdong commented Jun 5, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@rhdong
rhdong requested review from benfred and cjnolet June 5, 2024 00:05
@rhdong
rhdong requested review from a team as code owners June 5, 2024 00:05
@rhdong rhdong added feature request New feature or request non-breaking Introduces a non-breaking change C and removed cpp CMake labels Jun 5, 2024
benfred
benfred reviewed Jun 5, 2024
View reviewed changes
Comment thread python/cuvs/cuvs/neighbors/brute_force/brute_force.pyx Outdated
benfred
benfred reviewed Jun 5, 2024
View reviewed changes

@benfred benfred left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks great! thanks for getting this in.

some minor comments / feedback below:

Comment thread python/cuvs/cuvs/neighbors/brute_force/brute_force.pyx Outdated
Comment thread python/cuvs/cuvs/neighbors/brute_force/brute_force.pxd Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/prefilters.pyx Outdated
@copy-pr-bot

copy-pr-bot Bot commented Jun 6, 2024

Copy link
Copy Markdown

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.

@rhdong
rhdong requested a review from benfred June 6, 2024 01:49
@rhdong

rhdong commented Jun 6, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@rhdong
rhdong force-pushed the rhdong/pbf-python branch from eb49a91 to 5ce3b8e Compare June 6, 2024 06:35
@rhdong
rhdong force-pushed the rhdong/pbf-python branch from 5ce3b8e to 3a50b90 Compare June 6, 2024 06:36
@benfred
benfred requested a review from a team as a code owner June 6, 2024 19:18
@rhdong

rhdong commented Jun 26, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@rhdong

rhdong commented Jun 26, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@rhdong
rhdong requested a review from benfred June 26, 2024 22:32
cjnolet
cjnolet requested changes Jul 1, 2024
View reviewed changes

@cjnolet cjnolet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this API is coming along great! Moslty minor things at this point, but I think this should be ready very soon.

Comment thread cpp/include/cuvs/neighbors/prefilters.h
Comment thread cpp/include/cuvs/neighbors/prefilters.h
*/
enum cuvsPrefilterType {
/* No filter */
NO_FILTER,

@cjnolet cjnolet Jun 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We will need a way to specify a custom filter function (maybe to accept a function pointer?) also. While bfknn currently only supports bitmap / bitset, we also need to expose the filtering APIs for the other ANN indexes (most ideally through the same API).

I'm okay pushing this to a future change, since it's going to be needed ASAP so that we can expose the remaining filtering functions for the other algorithm. Can you create an issue for this?

@rhdong rhdong Jul 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will!

Comment thread cpp/test/neighbors/brute_force_c.cu Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/__init__.py Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/prefilters.pxd Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/prefilters.pyx
@rhdong

rhdong commented Jul 2, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@rhdong
rhdong requested a review from cjnolet July 2, 2024 02:27
cjnolet
cjnolet reviewed Jul 2, 2024
View reviewed changes
Comment thread cpp/test/neighbors/brute_force_c.cu
};

template <typename T, typename IdxT, typename bitmap_t = uint32_t>
void recall_eval_with_filter(T* query_data,

@cjnolet cjnolet Jul 2, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please make sure these are testing all possible combinations of the filter struct/type? Eg NULL, NO_FILTER, BITMAP, and BITSET?

We should set a good precedent for testing these early, so that all follow-on filtering implementations can do the same thing.

@rhdong rhdong Jul 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. I just checked the code, the NO_FILTER, BITMAP have been covered(pls refer to https://github.com/rapidsai/cuvs/pull/174/files#diff-1b162130023c13d62c06d91f3aab663d943091dddd8a38813c27f65e7d6c61dfR93), and the BITSET is reserved for other algorithms to compatible which is not be implemented in brute-force. Would you like me to implement BITSET in brute-force and test it?

@rhdong
rhdong requested a review from cjnolet July 4, 2024 21:36
benfred
benfred approved these changes Jul 17, 2024
View reviewed changes

@benfred benfred left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm!

@benfred

benfred commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

/ok to test

@benfred

benfred commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

/ok to test

@cjnolet

cjnolet commented Jul 29, 2024

Copy link
Copy Markdown
Contributor

/merge

@rapids-bot
rapids-bot Bot merged commit 2517826 into NVIDIA:branch-24.08 Jul 29, 2024
divyegala pushed a commit to divyegala/cuvs that referenced this pull request Jul 31, 2024
@rhdong @divyegala
[FEA] expose python & C API for prefiltered brute force (NVIDIA#174)
a56a74c 
Authors:
  - rhdong (https://github.com/rhdong)
  - Ben Frederickson (https://github.com/benfred)

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

URL: NVIDIA#174
landrumb pushed a commit to landrumb/cuvs that referenced this pull request Jul 8, 2026
@rhdong
[FEA] expose python & C API for prefiltered brute force (NVIDIA#174)
a174a1f 
Authors:
  - rhdong (https://github.com/rhdong)
  - Ben Frederickson (https://github.com/benfred)

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

URL: NVIDIA#174
@nvzm123 nvzm123 mentioned this pull request Aug 14, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjnolet cjnolet cjnolet approved these changes
+1 more reviewer
@benfred benfred benfred approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@rhdong rhdong

Labels

C CMake cpp feature request New feature or request non-breaking Introduces a non-breaking change Python

Projects

Unstructured Data Processing
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rhdong @benfred @cjnolet @josephine-wolf-oberholtzer