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

Opening up SearchParameters for IndexBinaryHNSW and IndexBinaryIVF #3503

Open
heemin32 opened this issue Jun 11, 2024 · 9 comments
Open

Opening up SearchParameters for IndexBinaryHNSW and IndexBinaryIVF #3503

heemin32 opened this issue Jun 11, 2024 · 9 comments

Comments

@heemin32
Copy link

Summary

IndexBinaryHNSW and IndexBinaryIVF is blocking SearchParameters. Can we open it up? Will there be any issue that I am not aware of?
https://github.com/facebookresearch/faiss/blob/main/faiss/IndexBinaryHNSW.cpp#L192

@mdouze
Copy link
Contributor

mdouze commented Jun 17, 2024

what do you mean with "blocking"?

@heemin32
Copy link
Author

what do you mean with "blocking"?

    FAISS_THROW_IF_NOT_MSG(
            !params, "search params not supported for this index");

@mdouze
Copy link
Contributor

mdouze commented Jun 17, 2024

Right, search parameters are not supported for most binary indexes. Enabling support is relatively simple, feel free to submit a PR.

@gustavz
Copy link

gustavz commented Sep 23, 2024

Hi @mdouze is there an update on this or a PR to track? I am looking for search params support for IndexBinaryFlat.
If there is currently no work on it, can you point me to a reference PR that could be used as a base to enable support for that index?

@heemin32
Copy link
Author

@gustavz You can take a look at this commit as a reference. heemin32@4d374aa
You can remove following line

    if (params_in && params_in->grp) {
        using RH = GroupedHeapBlockResultHandler<HNSW::C>;
        RH bres(n, distances_f, labels, k, params_in->grp);

        hnsw_search(this, n, x, bres, params_in);
    } else {

@gustavz
Copy link

gustavz commented Oct 23, 2024

@heemin32 so if thats the code that enables it, why dont you open a PR and get it merged?

@heemin32
Copy link
Author

heemin32 commented Oct 24, 2024

@gustavz I was having hard time to build faiss and add unit test with my mac environment. Especially the python version of unit test, I could not make it runnable.

@gustavz
Copy link

gustavz commented Dec 4, 2024

@mdouze would you mind making the PR to extend BinaryIndexFlat to support search params?
My C skills are unfortunately not enough :/
I bet this would be a highly appreciated feature as many use binary indexes for RAG applications and they all want to be able to pre-filter their searches by IDs.

@gustavz
Copy link

gustavz commented Dec 4, 2024

Here is a PR trying to do so, it's mostly cursor generated though: #4055

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants