Skip to content

Fix/check duplicate negative similarity#979

Closed
shafdev wants to merge 6 commits intoMemPalace:developfrom
shafdev:fix/check-duplicate-negative-similarity
Closed

Fix/check duplicate negative similarity#979
shafdev wants to merge 6 commits intoMemPalace:developfrom
shafdev:fix/check-duplicate-negative-similarity

Conversation

@shafdev
Copy link
Copy Markdown
Contributor

@shafdev shafdev commented Apr 17, 2026

fix: clamp similarity scores to 0.0 minimum in tool_check_duplicate and Layer3

Fixes #978

What does this PR do?

Fixes a silent correctness bug in two places where cosine distance was converted to similarity without clamping:

  • mempalace/mcp_server.pytool_check_duplicate()
  • mempalace/layers.pyLayer3.search()

Both used round(1 - dist, 3). With hnsw:space=cosine, ChromaDB distances are in [0, 2] — not [0, 1]. For maximally dissimilar vectors the distance slightly exceeds 1.0, making 1 - dist negative (e.g. dist=1.0035similarity=-0.004).

Before:

similarity = round(1 - dist, 3)          # can produce -0.004

After:

similarity = round(max(0.0, 1 - dist), 3)  # clamped to [0.0, 1.0]

searcher.py line 285 already uses the correct max(0.0, ...) pattern — this PR brings the two remaining sites into line with it.

How to test

Run the new regression tests:

python -m pytest tests/test_mcp_server.py::TestWriteTools::test_check_duplicate_similarity_never_negative -v
python -m pytest tests/test_layers.py::test_layer3_search_similarity_never_negative -v

test_check_duplicate_similarity_never_negative queries with unrelated content (threshold=0.0) and asserts all returned similarity scores are >= 0.0.

test_layer3_search_similarity_never_negative mocks a ChromaDB distance of 1.5 (beyond orthogonal) and asserts the rendered output contains sim=0.0 and not sim=-.

You can also reproduce the original bug directly:

import chromadb
client = chromadb.Client()
col = client.get_or_create_collection("bug_demo", metadata={"hnsw:space": "cosine"})
col.add(ids=["d1"], documents=["Pythagorean theorem: a^2 + b^2 = c^2"], metadatas=[{}])
results = col.query(query_texts=["Chocolate cake recipe"], n_results=1, include=["distances"])
dist = results["distances"][0][0]
print(round(1 - dist, 3))          # -0.004 (bugged)
print(round(max(0.0, 1 - dist), 3)) # 0.0   (fixed)

Checklist

  • Tests pass (python -m pytest tests/ -v)
  • No hardcoded paths
  • Linter passes (ruff check .)

igorls added 3 commits April 18, 2026 00:24
Bumps version across pyproject.toml, mempalace/version.py, README badge,
and uv.lock. Finalizes the 3.3.0 CHANGELOG section (was still labeled
'Unreleased') and adds a 3.3.1 section covering the multi-language
entity-detection infra and the five new locales landed since 2026-04-13.

Highlights:
- Multi-language entity detection infra (MemPalace#911) + script-aware word
  boundaries for combining-mark scripts (MemPalace#932) + BCP 47 case-insensitive
  locale resolution (MemPalace#928) + i18n patterns wired into miner/palace/
  entity_registry (MemPalace#931)
- Five new fully-supported locales: pt-br (MemPalace#156), ru (MemPalace#760), it (MemPalace#907),
  hi (MemPalace#773), id (MemPalace#778)
- UTF-8 encoding fix on read_text() calls for non-UTF-8 Windows locales
  (MemPalace#946)
- KnowledgeGraph lock correctness (MemPalace#884, MemPalace#887)
- Various smaller fixes and improvements
Advisor caught: initial boundary (962776c..develop) skipped PRs that
landed on develop after v3.3.0 tag but before the sync-back merge.
Adds entries for MemPalace#871 MEMPAL_VERBOSE, MemPalace#811 research() local-only
default, MemPalace#866 init .gitignore, MemPalace#864 MCP stdout redirect, MemPalace#863
precompact hook, MemPalace#865 searcher empty results, MemPalace#831 cold-start palace,
MemPalace#862 init help, MemPalace#815 Slack provenance, MemPalace#840 save hook auto-mine.
Also drops the awkward caveat on MemPalace#846 created_at — it's post-v3.3.0.
version-guard workflow checks five sources must agree:
mempalace/version.py, pyproject.toml, .claude-plugin/marketplace.json,
.claude-plugin/plugin.json, .codex-plugin/plugin.json.

Initial release commit missed the three plugin manifests.
@Qodo-Free-For-OSS
Copy link
Copy Markdown

Hi, Layer3.search() now clamps similarity to >= 0, but Layer3.search_raw() still returns round(1 - dist, 3), which can be negative for cosine distances > 1.0. This leaves an inconsistent Layer3 API where raw callers can still receive negative similarity values despite the PR’s stated fix for Layer3.

Severity: action required | Category: correctness

How to fix: Clamp similarity in search_raw

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

Layer3.search_raw() still computes similarity as round(1 - dist, 3), which can be negative under cosine distance when dist > 1.0. The PR clamps Layer3.search() but not search_raw(), leaving the bug reachable and making Layer3’s outputs inconsistent.

Issue Context

Cosine distance in the configured vector store can be in [0, 2], so 1 - dist can be negative.

Fix Focus Areas

  • mempalace/layers.py[300-339]
    • Change "similarity": round(1 - dist, 3) to "similarity": round(max(0.0, 1 - dist), 3).
  • tests/test_layers.py[480-538]
    • Add a regression test for Layer3.search_raw() ensuring returned similarity is never negative when dist > 1.0.

Qodo code review - free for open-source.

@shafdev shafdev force-pushed the fix/check-duplicate-negative-similarity branch from 945c07e to acf537d Compare April 18, 2026 07:10
…nd Layer3

With hnsw:space=cosine, ChromaDB distances are in [0, 2]. For maximally
dissimilar vectors the distance can exceed 1.0, making 1 - dist negative.
searcher.py already guards this with max(0.0, 1 - dist) at line 285;
tool_check_duplicate (mcp_server.py) and Layer3.search() (layers.py)
were missing the same clamp.

Fix: apply max(0.0, 1 - dist) in both locations so similarity is
always in [0.0, 1.0].

Regression tests:
- test_check_duplicate_similarity_never_negative
- test_layer3_search_similarity_never_negative

Fixes MemPalace#978
@jphein
Copy link
Copy Markdown
Collaborator

jphein commented Apr 19, 2026

The Qodo reviewer's search_raw() concern is already addressed — line 333 of layers.py in this diff clamps search_raw() alongside search(), and test_layer3_search_raw_similarity_never_negative covers it. All three sites are fixed with tests.

The scope issue is real: the files to strip before merging are version.py, pyproject.toml, CHANGELOG.md, README.md, .claude-plugin/marketplace.json, .claude-plugin/plugin.json, .codex-plugin/plugin.json, uv.lock. The actual fix is three one-line changes across two source files plus tests — that's what this branch should contain.

eldar702 added a commit to eldar702/mempalace that referenced this pull request Apr 19, 2026
``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).
eldar702 added a commit to eldar702/mempalace that referenced this pull request Apr 19, 2026
Chroma 1.5.x can return ``None`` inside the ``metadatas`` / ``documents``
lists of a query/get result for partially-flushed rows. The codebase
already has a systemic None-guard pattern (merged MemPalace#999, MemPalace#1013, MemPalace#1019)
but three call sites were still unguarded:

* ``mcp_server.tool_check_duplicate`` (``mcp_server.py:487-488``) —
  ``meta = results["metadatas"][0][i]`` followed by ``meta.get(...)``
  raises ``AttributeError: 'NoneType' object has no attribute 'get'``.
  The broad ``except Exception`` wrapper (line 504) swallows it and
  returns an uninformative ``"Duplicate check failed"``.

* ``layers.Layer1.generate`` (``layers.py:126``) — iterates
  ``zip(docs, metas)`` and calls ``meta.get(key)`` in the importance
  loop. A single None metadata blows up the entire wake-up render.

* ``layers.Layer2.retrieve`` (``layers.py:224``) — same pattern, same
  crash path for the on-demand render.

Apply the same ``meta = meta or {}`` / ``doc = doc or ""`` idiom used
by the merged guards in the search path. Three-line additions, no
behaviour change on well-formed results.

Tests added:

* ``test_check_duplicate_handles_none_metadata`` — mocks the collection
  query to return ``None`` for one metadata and document, asserts the
  call does not crash and the sentinel-rendered entry has wing/room "?"
  and empty content.
* ``test_layer1_handles_none_metadata`` / ``_handles_none_document``
* ``test_layer2_handles_none_metadata``

Relationship to other open PRs:

* **MemPalace#1019** guarded ``searcher.py`` loops. This PR extends the same
  guard to the three call sites MemPalace#1019 did not touch.
* **MemPalace#979** fixed ``tool_check_duplicate`` negative similarity but left
  the None-metadata path unguarded.
* Does not overlap **MemPalace#1013** (``Layer3.search_raw``) or **MemPalace#999**.
@shafdev shafdev marked this pull request as draft April 20, 2026 07:22
@shafdev shafdev marked this pull request as ready for review April 20, 2026 13:39
@jphein jphein added bug Something isn't working area/search Search and retrieval labels Apr 21, 2026
@igorls
Copy link
Copy Markdown
Member

igorls commented May 3, 2026

Thanks @ramv-mle! Closing as duplicate — PR #988 lands the same fix at +2/-2 (just the two missing max(0.0, ...) clamps in mcp_server.py and layers.py). Both PRs target the right call sites; we're preferring #988 for minimum-diff review. The additional test coverage in this PR is welcome — feel free to open a separate PR adding those tests once #988 lands. Really appreciate the analysis.

@igorls igorls closed this May 3, 2026
igorls pushed a commit to eldar702/mempalace that referenced this pull request May 6, 2026
``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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/search Search and retrieval bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tool_check_duplicate in mcp_server.py and Layer3.search() in layers.py can return negative similarity scores for very dissimilar content.

4 participants