search

facebookresearch / faiss

A library for efficient similarity search and clustering of dense vectors.
https://faiss.ai
MIT License
30.69k stars 3.58k forks source link

add hnsw unit test for PR 3840 #3849

Closed mengdilin closed 2 weeks ago

mengdilin commented 2 weeks ago

Summary: https://github.com/facebookresearch/faiss/issues/3845

Add unit tests for helper search utilities for HNSW. These utility functions live inside an anonymous namespace and each has a reference version gated behind a const bool, I refactored them so the reference version is a flag for the function which defaults to false.

If we are concerned about the performance overhead of the extra if branching (whether to use reference version or not) inside these utility functions, I'm happy to lift out the reference versions to their own functions inside the unit test

Differential Revision: D62510014

facebook-github-bot commented 2 weeks ago

This pull request was exported from Phabricator. Differential Revision: D62510014

mengdilin commented 2 weeks ago

@alexanderguzhva I followed your advise and wrote a unit test for your fix. Would appreciate a look

alexanderguzhva commented 2 weeks ago

@mengdilin 1) why is it needed to put things into hnsw_utils namespace? 2) are there any real-world scenarios where a switch to a reference version is really useful (besides testing)?

mengdilin commented 2 weeks ago

@alexanderguzhva the utility functions were in an anonymous namespace before so I have to move them to a namespace. Happy to move it to just the faiss namespace if that's the convention.

We don't have production usecase for the referenced version. The only benefit of branching is that we are keeping the referenced version close to the source location of the code. I can move them to just unit tests similar to how we have done it for HeapPopMin's unit test

facebook-github-bot commented 2 weeks ago

This pull request was exported from Phabricator. Differential Revision: D62510014

facebook-github-bot commented 2 weeks ago

This pull request was exported from Phabricator. Differential Revision: D62510014

facebook-github-bot commented 2 weeks ago

This pull request was exported from Phabricator. Differential Revision: D62510014

facebook-github-bot commented 2 weeks ago

This pull request has been merged in facebookresearch/faiss@52ce3f55ae91846a1fad9a427918f6a95ab271d3.