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

fix ivf_pq n_probes #1456

Merged
merged 1 commit into from
Apr 23, 2023
Merged

fix ivf_pq n_probes #1456

merged 1 commit into from
Apr 23, 2023

Conversation

benfred
Copy link
Member

@benfred benfred commented Apr 22, 2023

The ivf-pq search code was including a guard like

auto n_probes = std::min<uint32_t>(params.n_probes, index.n_lists());

to check to make sure that we weren't selecting more values than are available. However,
this wasn't being used and instead just params.n_probes was being passed to functions
like select_k. This lead to asking select_k to select say 100 items, when there
were only 90 to choose from - and caused some issues downstream when trying to update
the select_k algorithm

Fix.

The ivf-pq search code was including a guard like

```auto n_probes = std::min<uint32_t>(params.n_probes, index.n_lists());```

to check to make sure that we weren't selecting more values than are available. However,
this wasn't being used and instead just `params.n_probes` was being passed to functions
like `select_k`. This lead to asking select_k to select say 100 items, when there
were only 90 to choose from - and caused some issues downstream when trying to update
the select_k algorithm

Fix.
@benfred benfred requested a review from a team as a code owner April 22, 2023 01:00
@github-actions github-actions bot added the cpp label Apr 22, 2023
@benfred benfred marked this pull request as draft April 22, 2023 01:00
@cjnolet cjnolet added bug Something isn't working non-breaking Non-breaking change labels Apr 22, 2023
@benfred benfred marked this pull request as ready for review April 22, 2023 05:51
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @benfred, good catch! LGTM.

@benfred
Copy link
Member Author

benfred commented Apr 23, 2023

/merge

@rapids-bot rapids-bot bot merged commit c0c4d52 into rapidsai:branch-23.06 Apr 23, 2023
@benfred benfred deleted the ivf_pq_fix branch April 24, 2023 04:47
ahendriksen pushed a commit to ahendriksen/raft that referenced this pull request Apr 27, 2023
The ivf-pq search code was including a guard like

```auto n_probes = std::min<uint32_t>(params.n_probes, index.n_lists());```

to check to make sure that we weren't selecting more values than are available. However,
this wasn't being used and instead just `params.n_probes` was being passed to functions
like `select_k`. This lead to asking select_k to select say 100 items, when there
were only 90 to choose from - and caused some issues downstream when trying to update
the select_k algorithm

Fix.

Authors:
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Micka (https://github.com/lowener)
  - Tamas Bela Feher (https://github.com/tfeher)

URL: rapidsai#1456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cpp non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

4 participants