fix: clamp similarity scores to [0,1] to prevent negative values#988
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes negative similarity scores produced when converting ChromaDB cosine distances to similarity, ensuring downstream clients don’t see meaningless negative values (per Issue #978).
Changes:
- Clamp
tool_check_duplicate()similarity to floor at0.0whendist > 1. - Clamp
Layer3.search()similarity to floor at0.0whendist > 1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| mempalace/mcp_server.py | Floors duplicate-check similarity at 0.0 to prevent negative values. |
| mempalace/layers.py | Floors Layer3 formatted search similarity at 0.0 to prevent negative values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dist = results["distances"][0][i] | ||
| similarity = round(1 - dist, 3) | ||
| similarity = round(max(0.0, 1 - dist), 3) | ||
| if similarity >= threshold: |
There was a problem hiding this comment.
This change modifies the JSON similarity value returned by tool_check_duplicate() when dist > 1 (it now floors at 0.0). Since tests/test_mcp_server.py already covers tool_check_duplicate(), please add a regression test that mocks/forces a distance slightly above 1.0 and asserts the returned similarity is clamped to 0.0 (not negative).
| for i, (doc, meta, dist) in enumerate(zip(docs, metas, dists), 1): | ||
| similarity = round(1 - dist, 3) | ||
| similarity = round(max(0.0, 1 - dist), 3) | ||
| wing_name = meta.get("wing", "?") |
There was a problem hiding this comment.
Layer3.search() now clamps similarity to avoid negative values, but Layer3.search_raw() below still computes round(1 - dist, 3) (see the similarity field in the raw hit dict). That means negative similarity scores can still leak to API consumers via search_raw. Consider applying the same max(0.0, 1 - dist) clamp there as well for consistency and to fully address the underlying issue.
| for i, (doc, meta, dist) in enumerate(zip(docs, metas, dists), 1): | ||
| similarity = round(1 - dist, 3) | ||
| similarity = round(max(0.0, 1 - dist), 3) | ||
| wing_name = meta.get("wing", "?") |
There was a problem hiding this comment.
This change alters similarity semantics when dist > 1 (similarity now floors at 0.0). There are existing tests for Layer3.search in tests/test_layers.py; please add a regression test case with a mocked distance slightly > 1.0 to assert the rendered sim= value is 0.0 (and not negative).
|
#979 (2026-04-17) covers the same fix: #979 does bundle unrelated scope (version bumps in |
|
|
``search_memories`` computes ``effective_dist = dist - boost`` where ``boost`` can be as large as ``CLOSET_RANK_BOOSTS[0] == 0.40`` for a rank-0 closet hit. When the raw drawer distance is small — any near-exact match — the subtraction goes negative. Two downstream effects: 1. Line 418 returns ``round(max(0.0, 1 - effective_dist), 3)`` as ``similarity``. With ``effective_dist = -0.30`` that yields ``similarity = 1.30``, outside the documented ``[0, 1]`` range. The ``max(0.0, ...)`` only prevents negative similarities; it does not cap above 1. 2. Line 427 stores ``_sort_key: effective_dist`` and line 435 sorts ``scored`` ascending by that key. A negative key drops *below* the rest, so the strongest hybrid matches end up sorting after weaker ones — ranking inversion under the exact conditions hybrid retrieval is supposed to serve best. Clamp ``effective_dist`` to the valid cosine-distance range ``[0, 2]``. The boost still wins (closet-backed hit still ranks first), it just no longer flips the order. Test added: mock drawer_col (base dist 0.08 / 0.35 for two sources) + closet_col (rank-0 closet for the 0.08 source) → assert all hits have ``0 <= similarity <= 1`` and ``0 <= effective_distance <= 2``, and that the closet-boosted source still ranks first. Relationship to other PRs: * **MemPalace#988** clamps the output ``similarity`` alone. That does not fix the sort-key inversion or the invalid ``effective_distance`` in the returned dict. This PR clamps at the arithmetic source so both downstream users of the value stay in range. * Orthogonal to **MemPalace#979** (``tool_check_duplicate`` negative similarity).
|
Bumping — verified REAL on develop: Note: there's a competing PR #979 covering the same ground but at +147/-11 across 12 files vs your +2/-2 surgical fix. We're going to close #979 as duplicate and prefer this one. Please confirm CI green on the latest commit and we can land it. |
igorls
force-pushed
the
clawoss/fix/978-negative-similarity
branch
from
fbe287d to
ad68ff6
Compare
|
Rebased onto develop Note: the lint check on develop is pre-existing-failing from #1305 (E402 in |
``search_memories`` computes ``effective_dist = dist - boost`` where ``boost`` can be as large as ``CLOSET_RANK_BOOSTS[0] == 0.40`` for a rank-0 closet hit. When the raw drawer distance is small — any near-exact match — the subtraction goes negative. Two downstream effects: 1. Line 418 returns ``round(max(0.0, 1 - effective_dist), 3)`` as ``similarity``. With ``effective_dist = -0.30`` that yields ``similarity = 1.30``, outside the documented ``[0, 1]`` range. The ``max(0.0, ...)`` only prevents negative similarities; it does not cap above 1. 2. Line 427 stores ``_sort_key: effective_dist`` and line 435 sorts ``scored`` ascending by that key. A negative key drops *below* the rest, so the strongest hybrid matches end up sorting after weaker ones — ranking inversion under the exact conditions hybrid retrieval is supposed to serve best. Clamp ``effective_dist`` to the valid cosine-distance range ``[0, 2]``. The boost still wins (closet-backed hit still ranks first), it just no longer flips the order. Test added: mock drawer_col (base dist 0.08 / 0.35 for two sources) + closet_col (rank-0 closet for the 0.08 source) → assert all hits have ``0 <= similarity <= 1`` and ``0 <= effective_distance <= 2``, and that the closet-boosted source still ranks first. Relationship to other PRs: * **MemPalace#988** clamps the output ``similarity`` alone. That does not fix the sort-key inversion or the invalid ``effective_distance`` in the returned dict. This PR clamps at the arithmetic source so both downstream users of the value stay in range. * Orthogonal to **MemPalace#979** (``tool_check_duplicate`` negative similarity).
igorls
force-pushed
the
clawoss/fix/978-negative-similarity
branch
from
ad68ff6 to
f2bed92
Compare
Fix: Clamp similarity scores to [0,1]
Both \ and \ were returning negative similarity scores
for very dissimilar content.
Root Cause
Both used \ to convert cosine distance to similarity.
With , ChromaDB distances can exceed 1.0 for maximally
dissimilar vectors, making \ negative.
The rest of the codebase already handles this correctly:
\ line 285 uses .
Changes
Fixes
tool_check_duplicateinmcp_server.pyandLayer3.search()inlayers.pycan return negative similarity scores for very dissimilar content. #978.