Skip to content

Commit 02ea143

Browse files
mayuriphadmeta-codesync[bot]
authored andcommitted
Fix range_search_max_results to respect all similarity metrics (#5533)
Summary: ## Bug `range_search_max_results()` (and its helper `apply_maxres()`) in `contrib/exhaustive_search.py` decide whether to keep the highest- or lowest-scoring results when pruning the accumulated results down to `max_results`, based on: ```python keep_max=index.metric_type == faiss.METRIC_INNER_PRODUCT ``` This hardcodes the assumption that `METRIC_INNER_PRODUCT` is the only "similarity" metric (higher = better). But `faiss.is_similarity_metric()` (see `faiss/MetricType.h`) also treats `METRIC_Jaccard` as a similarity metric, and this same, correct check is already used a few lines above in `range_search_gpu()` in the very same file (`keep_max = faiss.is_similarity_metric(index_gpu.metric_type)`). As a result, when `range_search_max_results` is used with a `METRIC_Jaccard` index and the result set grows past `max_results`, the pruning logic incorrectly discards the *highest*-similarity matches and keeps the *lowest*-similarity ones, and moves the returned radius in the wrong direction. ## Root cause Two call sites in `contrib/exhaustive_search.py` compare `index.metric_type` directly against `faiss.METRIC_INNER_PRODUCT` instead of using the library's own `faiss.is_similarity_metric()` helper, which is the single source of truth for which metrics are "larger is better". ## Fix Replace both occurrences of `index.metric_type == faiss.METRIC_INNER_PRODUCT` with `faiss.is_similarity_metric(index.metric_type)`, matching the existing pattern used in `range_search_gpu` in the same module. Pull Request resolved: #5533 Test Plan: Added `test_query_iterator_similarity_metric` to `tests/test_contrib.py`. It exercises `range_search_max_results` against a lightweight fake index using `METRIC_Jaccard` (real `IndexFlat.range_search` doesn't support `METRIC_Jaccard` directly, so a fake index is used to isolate the pruning logic under test) and asserts that pruning keeps the highest-scoring results and raises the radius accordingly. - Confirmed the new test fails on `main` (old code keeps the *lowest*-similarity results and lowers the radius) and passes after the fix, by swapping the module in-place against an installed `faiss-cpu` and running the assertions directly (full repo `pytest` collection isn't possible in this environment without rebuilding the C++ extension for `main`, since the installed wheel is older and `tests/common_faiss_tests.py` on `main` references APIs from a newer build). - The fix is a pure two-line change that only affects the boolean passed to `apply_maxres`, verified by direct comparison of before/after behavior with the reproduction script. Reviewed By: mnorris11 Differential Revision: D117001619 Pulled By: alibeklfc fbshipit-source-id: 27e6757951d63a4d4f4f998640a1726ef8f6c220
1 parent bd70874 commit 02ea143

2 files changed

Lines changed: 53 additions & 2 deletions

File tree

contrib/exhaustive_search.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ def range_search_max_results(
347347
radius, totres = apply_maxres(
348348
res_batches,
349349
min_results,
350-
keep_max=index.metric_type == faiss.METRIC_INNER_PRODUCT,
350+
keep_max=faiss.is_similarity_metric(index.metric_type),
351351
)
352352
t2 = time.time()
353353
t_search += t1 - t0
@@ -366,7 +366,7 @@ def range_search_max_results(
366366
radius, totres = apply_maxres(
367367
res_batches,
368368
min_results,
369-
keep_max=index.metric_type == faiss.METRIC_INNER_PRODUCT,
369+
keep_max=faiss.is_similarity_metric(index.metric_type),
370370
)
371371

372372
nres = np.hstack([nres_i for nres_i, dis_i, ids_i in res_batches])

tests/test_contrib.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,57 @@ def matrix_iterator(xb, bs):
194194
ref_lims, ref_D, ref_I, new_lims, new_D, new_I
195195
)
196196

197+
def test_query_iterator_similarity_metric(self):
198+
"""range_search_max_results must keep the highest-scoring results
199+
(and raise the radius) for any similarity metric, not just
200+
METRIC_INNER_PRODUCT. Regression test for a bug where pruning
201+
for METRIC_Jaccard (also a similarity metric, see
202+
faiss.is_similarity_metric) kept the lowest-scoring results
203+
instead of the highest-scoring ones."""
204+
205+
class FakeSimilarityIndex:
206+
"""Stands in for an Index using a similarity metric (higher
207+
score = better match), exposing only what
208+
range_search_max_results touches."""
209+
210+
def __init__(self, sims, metric_type):
211+
self.sims = sims
212+
self.metric_type = metric_type
213+
214+
def range_search(self, xq, radius):
215+
lims = [0]
216+
D = []
217+
I = []
218+
for i in range(len(xq)):
219+
s = self.sims[i]
220+
idx = np.nonzero(s > radius)[0]
221+
D.append(s[idx])
222+
I.append(idx.astype("int64"))
223+
lims.append(lims[-1] + len(idx))
224+
return (
225+
np.array(lims, dtype="uint64"),
226+
np.hstack(D).astype("float32"),
227+
np.hstack(I).astype("int64"),
228+
)
229+
230+
rs = np.random.RandomState(1)
231+
nq, ndb = 5, 200
232+
sims = [rs.rand(ndb).astype("float32") for _ in range(nq)]
233+
index = FakeSimilarityIndex(sims, faiss.METRIC_Jaccard)
234+
235+
xq = np.zeros((nq, 1), dtype="float32") # unused by the fake index
236+
radius = 0.0 # keep everything initially
237+
total = sum(len(s) for s in sims)
238+
max_results = total // 2
239+
240+
new_radius, lims, D, I = range_search_max_results(
241+
index, iter([xq]), radius, max_results=max_results
242+
)
243+
244+
self.assertGreaterEqual(new_radius, radius)
245+
self.assertGreater(len(D), 0)
246+
self.assertGreaterEqual(D.min(), new_radius - 1e-6)
247+
197248

198249
class TestInspect(unittest.TestCase):
199250

0 commit comments

Comments
 (0)