Repository navigation
Conversation
vishal-bala
left a comment
There was a problem hiding this comment.
Thanks for the clear issue and the fix! The client-side clamp is right: on Redis 8.4, the 200 self-matches from #822 now score exactly 1 for both VectorQuery and VectorRangeQuery, where on main 32 (FLOAT32) and 85 (FLOAT16) scored above 1.
The same overshoot happens on the server. AggregateHybridQuery (aggregate.py:186) and MultiVectorQuery (:344) compute the similarity in an APPLY expression, (2 - @distance)/2, which never reaches norm_cosine_distance. With this PR applied, their scores still go above 1 on exactly the same self-matches. Could you clamp those two as well, so #822 is fixed for every query type?
Why clamp at exactly 1, not just remove the negative sign. (2 - abs(@distance))/2 is the obvious expression, but it turns the noise into a penalty instead of removing it. On FLOAT16 the noise reaches a distance of about −3.7e-4, so a self-match would score 0.99982 and rank below a different document at distance 1e-4, which scores 0.99995. Clamping keeps the self-match first and makes these queries agree with VectorQuery.
Redis 8.0 has no case, and APPLY has no max. case(@distance < 0, 1, ...) fails on Redis 8.0 with "Unknown function name". This identity works on 8.0, 8.2 and 8.10:
def _cosine_similarity(distance_field: str) -> str:
"""APPLY expression for (2 - max(d, 0)) / 2.
APPLY has no max(), and case() needs Redis 8.2+, so max(d, 0) is
written as (d + abs(d)) / 2.
"""
d = f"@{distance_field}"
return f"(2 - ({d} + abs({d}))/2)/2"Tests. One integration test with a self-match is enough: assert vector_similarity <= 1 for AggregateHybridQuery and score_0 <= 1 for MultiVectorQuery. A unit test of the expression string wouldn't prove anything the server does.
Please also mention the two aggregate queries in the Release Notes and the Reviewer Brief.
|
Addressed in 09310ec.
Added Redis integration coverage for both |
|
|
||
| try: | ||
| rng = np.random.default_rng(0) | ||
| vector = rng.standard_normal((8, 384)).astype(np.float32)[7] |
There was a problem hiding this comment.
Could you add a short comment saying vector 7 is picked because its self-distance comes out slightly negative? Without it, this looks arbitrary and could be swapped for a vector that never overshoots, which would quietly stop the test catching the bug.
|
Could you also update the Release Notes and Reviewer Brief to cover |
|
Thanks for the review, both sections are updated now. The Release Notes name |
Fixes #822.
This PR clamps normalized COSINE scores to
[0, 1], preventing floating-point rounding artifacts from producing scores above the documented range.Motivation
When
normalize_vector_distance=True, small floating-point errors can cause Redis COSINE distances to fall slightly below zero. Normalizing those values produces scores slightly above1, violating the documented[0, 1]range and causing downstream validation warnings.Changes
Clamp normalized COSINE scores
Before:
A slightly negative distance could therefore normalize above
1.After:
Normalized scores are now clamped to
[0, 1]. Unit coverage includes floating-point overshoot, boundary values, an intermediate value, and distances above the valid COSINE range.Release Notes
Normalized COSINE similarity scores from
VectorQuery,VectorRangeQuery,AggregateHybridQuery, andMultiVectorQuerynow remain within[0, 1]when floating-point rounding produces a slightly negative distance.Reviewer Brief
Start here:
redisvl/query/aggregate.py,_cosine_similarity(). This is the server-side counterpart tonorm_cosine_distance()and prevents negative rounding noise from producing scores above1in bothAggregateHybridQueryandMultiVectorQuery.Not verified: the exact 200-vector reproduction on Redis 8.10.3 and the FLOAT16 aggregate path; local runtime coverage used a deterministic FLOAT32 self-match on Redis 8.0 and Redis 8.4.
Beyond CI: the aggregate regression produced
1.0000000596before the server-side clamp and now passes; the targeted regression also passed against the minimum Redis 8.0 runtime.Note
Low Risk
Boundary clamping on cosine score math with focused tests; no auth, data migration, or API contract changes beyond enforcing the documented score range.
Overview
Fixes normalized cosine similarity overshooting 1 when Redis returns slightly negative distances from float rounding (e.g. self-matches), which broke the documented [0, 1] range and downstream validation.
Client-side normalization:
norm_cosine_distance()now caps results withmin(..., 1)in addition to the existing floor at 0, so every path that uses this helper (including normalized vector queries) stays in range.Aggregation queries:
AggregateHybridQueryandMultiVectorQueryno longer use raw(2 - @distance)/2in RedisAPPLY. They share a new_cosine_similarity()expression that treats distance asmax(distance, 0)via(distance + abs(distance))/2, matching the Python helper whereAPPLYhas nomax.Tests add integration coverage that hybrid and multi-vector aggregate scores stay ≤ 1, expand unit cases for
norm_cosine_distance, and update expected query strings.Reviewed by Cursor Bugbot for commit 09310ec. Bugbot is set up for automated code reviews on this repo. Configure here.