Merge shard results by metric type rather than assuming similarity - #5509
Open
wilyan09007 wants to merge 1 commit into
Open
Merge shard results by metric type rather than assuming similarity#5509wilyan09007 wants to merge 1 commit into
wilyan09007 wants to merge 1 commit into
Conversation
IndexShardsTemplate::search chose the merge comparator by testing for METRIC_L2, so every other metric was merged as a similarity. Distance metrics such as METRIC_L1, METRIC_Linf and METRIC_Lp therefore returned the farthest vectors first once results crossed a shard boundary, while the same data in a single unsharded index returned them nearest first. Select the comparator with is_similarity_metric() instead. METRIC_L2 and METRIC_INNER_PRODUCT keep their existing behaviour, as does IndexBinary, whose metric_type defaults to METRIC_L2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IndexShardsTemplate::searchpicked the comparator for merging per-shard results by testing forMETRIC_L2:Everything that is not
METRIC_L2was therefore merged as a similarity.METRIC_L1,METRIC_LinfandMETRIC_Lpreturn distances, so once results crossed a shard boundary they came back farthest-first, while the same vectors in a single unsharded index came back nearest-first.This PR selects the comparator with the existing
is_similarity_metric()helper fromMetricType.h, which is exactly the distinction the branch needs.Fixes #5503
Behaviour change
is_similarity_metric()is true only forMETRIC_INNER_PRODUCTandMETRIC_Jaccard, so:METRIC_L2CMinCMin(unchanged)METRIC_INNER_PRODUCTCMaxCMax(unchanged)METRIC_L1,METRIC_Linf,METRIC_Lp,METRIC_Canberra,METRIC_BrayCurtis,METRIC_JensenShannonCMaxCMinMETRIC_JaccardCMaxCMax(unchanged)IndexShardsTemplate<IndexBinary>is also unaffected:IndexBinary::metric_typedefaults toMETRIC_L2, which keepsCMinas before.Reproduction
Against the released
faiss-cpu1.15.0 wheel, using the reporter's example:Test
Adds
Shards::test_shards_distance_metric_orderingtotests/test_meta_index.py. It splits the dataset across threeMETRIC_L1shards and checks that each result row is ordered nearest-first and matches the distances an unshardedIndexFlat(METRIC_L1)returns. Distances rather than labels are compared so that equidistant neighbours may be returned in either order.The test fails on
main(the rows come back reversed) and passes with this change.Testing notes
I reproduced the bug against the released 1.15.0 wheel as shown above, but I was not able to build faiss from source on this machine (Windows, no local C++ toolchain), so I have not executed the C++ build or run the test suite locally. The added test's pass/fail claim above follows from the comparator change rather than from a local run. Please treat CI as the gate, and I am happy to adjust if anything in the suite disagrees.