Skip to content

Add python and rust bindings for Ivf-Flat#82

Merged
rapids-bot[bot] merged 12 commits into
NVIDIA:branch-24.06from
benfred:ivf_flat_python_rust
Apr 30, 2024
Merged

Add python and rust bindings for Ivf-Flat#82
rapids-bot[bot] merged 12 commits into
NVIDIA:branch-24.06from
benfred:ivf_flat_python_rust

Conversation

@benfred

@benfred benfred commented Apr 15, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@benfred benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Apr 15, 2024
@benfred benfred requested review from a team as code owners April 15, 2024 18:29
@benfred benfred marked this pull request as draft April 15, 2024 18:29
@benfred benfred self-assigned this Apr 15, 2024
@benfred benfred marked this pull request as ready for review April 15, 2024 21:31

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

Quick review of the python side, things look good overall there!

Comment thread python/cuvs/cuvs/neighbors/ivf_flat/CMakeLists.txt Outdated
Comment thread python/cuvs/cuvs/neighbors/ivf_flat/ivf_flat.pyx
kmeans_n_iters : int, default = 20
The number of iterations searching for kmeans centers during index
building.
kmeans_trainset_fraction : int, default = 0.5

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 wonder if adding a suggestion of when to change this would be useful for users

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.

I think it could be very helpful! Though - I'm not sure of when to change this myself, and have just copied this comment from the c++ api. Do you have any thoughts here?

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.

Assuming we are talking about kmeans_n_iters here, this question has actually come up quite a bit- and more recently in discussions with some of our partners. @benfred what do you think about adding a quick blurb like The default setting is often fine, but this parameter can be decreased to improve training time wih larger trainset fractions (10M+ vectors) or increased for smaller trainset fractions (very small number of vectors) to improve recall.

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.

@cjnolet that sounds good! I've added in the last commit 3c9efd3 (which also adds this into the docs site)

Comment thread python/cuvs/cuvs/neighbors/ivf_flat/ivf_flat.pyx Outdated
@benfred benfred requested a review from a team as a code owner April 30, 2024 21:18

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

looks great from my side!

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

LGTM!

@benfred

benfred commented Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot rapids-bot Bot merged commit 00616f9 into NVIDIA:branch-24.06 Apr 30, 2024
@benfred benfred deleted the ivf_flat_python_rust branch April 30, 2024 23:47
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 Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants