Skip to content

fix: default collection to cosine distance and clamp similarity scores#568

Closed
arnoldwender wants to merge 3 commits intoMemPalace:developfrom
arnoldwender:fix/cosine-distance-default
Closed

fix: default collection to cosine distance and clamp similarity scores#568
arnoldwender wants to merge 3 commits intoMemPalace:developfrom
arnoldwender:fix/cosine-distance-default

Conversation

@arnoldwender
Copy link
Copy Markdown
Contributor

@arnoldwender arnoldwender commented Apr 10, 2026 •
edited
Loading

Summary

  • palace.py: create_collection() now passes metadata={"hnsw:space": "cosine"} so new palaces use cosine distance instead of ChromaDB's L2 default
  • mcp_server.py: Same fix applied to get_or_create_collection() in the MCP path
  • searcher.py + mcp_server.py: Similarity clamped with max(0, 1 - dist) to prevent negative scores from existing L2-based palaces

Problem

Similarity is calculated as round(1 - dist, 3) throughout the codebase. This formula is correct for cosine distance (range 0-1) but produces negative similarity scores for L2/Euclidean distance (ChromaDB's default). Collections created via palace.py have no distance metric specified, so they default to L2. Only repair.py creates with hnsw:space=cosine.

Any palace that hasn't been through mempalace repair silently produces nonsensical negative similarity values.

Migration note

This fix only applies to new palaces. Existing palaces created before this change still use L2 distance internally — get_or_create_collection does not retroactively change the distance metric on an existing collection. Users with existing palaces should run mempalace repair to rebuild the collection with cosine distance. The max(0, ...) clamp provides a safe fallback in the meantime.

Test plan

  • Create a fresh palace with mempalace init + mempalace mine — verify collection uses cosine
  • Search results should show similarity in 0.0–1.0 range
  • Existing L2 palaces should clamp to 0 instead of going negative
  • mempalace repair still works correctly (already uses cosine)

web3guru888
web3guru888 approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown

@web3guru888 web3guru888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a real problem we've hit ourselves.

The L2 vs cosine mismatch is subtle but devastating: new palaces silently produce negative similarities, and since the code just displays the number users may not notice something is wrong — they just see low scores and think their queries are bad.

Two things I especially like about this approach:

  1. **Setting at collection creation in both and ** — fixes the root cause for new palaces in both code paths.
  2. ** clamp** — correct backwards-compat strategy for existing L2 palaces. A negative clamp to 0 is honest ("not similar") rather than misleading.

One gap worth noting: only applies cosine to but not (the retrieval path). That's correct — you can't change a collection's metric after creation — but users migrating from L2 won't automatically get the fix without running
No palace found at /root/.mempalace/palace. Worth a sentence in the PR description or CHANGELOG noting that existing palaces need
No palace found at /root/.mempalace/palace to adopt cosine.

We hit this exact issue when first deploying — our similarity scores looked fine after repair but were broken on fresh test palaces created via before repair ran. Approved.

jphein added a commit to jphein/mempalace that referenced this pull request Apr 10, 2026
@jphein @claude
fix: cherry-pick upstream bug fixes — cosine distance, WAL rotation, …
3dd85b6 
…compress keys

- Default new collections to cosine distance (hnsw:space=cosine) instead
  of L2 — fixes negative similarity scores (MemPalace#568)
- Filter unexpected MCP tool args before dispatch — prevents TypeError
  crash from extra params like top_k (MemPalace#572)
- Rotate WAL at 10 MB with one backup to prevent unbounded growth (MemPalace#573)
- Fix cmd_compress KeyError: align dict keys with compression_stats()
  return values (MemPalace#569)
- Fix spellcheck _load_known_names: read from "people" key, use dict
  keys as canonical names (MemPalace#570)
- Repair command uses cosine distance for rebuilt collection

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@jphein jphein mentioned this pull request Apr 10, 2026
Closed
11 tasks
@jphein
Copy link
Copy Markdown
Collaborator

jphein commented Apr 10, 2026

Incorporated into #562 — both the hnsw:space=cosine metadata on create_collection and the max(0, 1-dist) similarity clamp. Also applied cosine distance to the repair command so rebuilt palaces get it too. Thanks for the fix.

web3guru888
web3guru888 approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown

@web3guru888 web3guru888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, narrow fix. LGTM.

[MemPalace-AGI integration — 215 tests, 710 KG entities]

jphein added a commit to jphein/mempalace that referenced this pull request Apr 11, 2026
@jphein @claude
perf: batch ChromaDB writes, concurrent mining, bulk mtime pre-fetch
6865660 
- Batch upserts (100 docs/call) in both general miner and convo_miner
  instead of one-at-a-time writes — 3-5x faster on large projects
- _prepare_file() separates read/chunk (thread-safe) from write,
  enabling ThreadPoolExecutor concurrency with --workers flag
- bulk_check_mined() pre-fetches all source_file/mtime pairs in one
  paginated scan instead of per-file ChromaDB queries
- Epsilon mtime comparison (abs < 0.01) instead of float == (MemPalace#483)
- Cosine distance default for new collections (hnsw:space=cosine)
- SKIP_PATTERNS/JUNK_FILE_SIZE filter minified JS, lockfiles, large dumps
- detect_room() uses word-boundary regex and exact-match priority
- chunk_text() accepts configurable size/overlap/min params
- Entity detector STOPWORDS expanded (73 technical terms)
- Configurable chunk_size/chunk_overlap/min_chunk_size in palace config

Split from MemPalace#562 per maintainer request. Closes MemPalace#483, MemPalace#568.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@jphein jphein mentioned this pull request Apr 11, 2026
Closed
4 tasks
@bensig bensig changed the base branch from main to develop April 11, 2026 22:21
jphein added a commit to jphein/mempalace that referenced this pull request Apr 12, 2026
@jphein @claude
perf: batch ChromaDB writes, concurrent mining, bulk mtime pre-fetch
f4f43bc 
- Batch upserts (100 docs/call) in both general miner and convo_miner
  instead of one-at-a-time writes — 3-5x faster on large projects
- _prepare_file() separates read/chunk (thread-safe) from write,
  enabling ThreadPoolExecutor concurrency with --workers flag
- bulk_check_mined() pre-fetches all source_file/mtime pairs in one
  paginated scan instead of per-file ChromaDB queries
- Epsilon mtime comparison (abs < 0.01) instead of float == (MemPalace#483)
- Cosine distance default for new collections (hnsw:space=cosine)
- SKIP_PATTERNS/JUNK_FILE_SIZE filter minified JS, lockfiles, large dumps
- detect_room() uses word-boundary regex and exact-match priority
- chunk_text() accepts configurable size/overlap/min params
- Entity detector STOPWORDS expanded (73 technical terms)
- Configurable chunk_size/chunk_overlap/min_chunk_size in palace config

Split from MemPalace#562 per maintainer request. Closes MemPalace#483, MemPalace#568.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
z3tz3r0 and others added 2 commits April 11, 2026 23:06
@z3tz3r0
fix: disambiguate hook block reasons to name MemPalace explicitly (Me…
f138d49 
…mPalace#666)

Replace "your memory system" with explicit MemPalace references and
tool names (mempalace_diary_write, mempalace_add_drawer, mempalace_kg_add)
in stop and precompact hook block reasons. This prevents Claude Code from
misinterpreting the hook as a native auto-memory save instruction.

Updated in both Python (hooks_cli.py) and standalone shell scripts.

Also fix CONTRIBUTING.md Getting Started to show the fork-first workflow,
matching the PR Guidelines section.
@arnoldwender
fix: default collection to cosine distance and clamp similarity scores
99eb38d
@arnoldwender arnoldwender force-pushed the fix/cosine-distance-default branch from d68c73c to 99eb38d Compare April 12, 2026 19:37
@arnoldwender arnoldwender requested a review from igorls as a code owner April 12, 2026 19:37
@arnoldwender
fix: use 0.0 literal in max() to guarantee float similarity scores
f860cb8 
max(0, negative_float) returns int 0 in Python when the L2 distance
exceeds 1.0, causing isinstance(score, float) to fail.

Replace max(0, ...) with max(0.0, ...) in both clamp sites so the
return type is always float regardless of the distance value.
jphein added a commit to jphein/mempalace that referenced this pull request Apr 12, 2026
@jphein @claude
perf: batch ChromaDB writes, concurrent mining, bulk mtime pre-fetch
9801d6c 
- Batch upserts (100 docs/call) in both general miner and convo_miner
  instead of one-at-a-time writes — 3-5x faster on large projects
- _prepare_file() separates read/chunk (thread-safe) from write,
  enabling ThreadPoolExecutor concurrency with --workers flag
- bulk_check_mined() pre-fetches all source_file/mtime pairs in one
  paginated scan instead of per-file ChromaDB queries
- Epsilon mtime comparison (abs < 0.01) instead of float == (MemPalace#483)
- Cosine distance default for new collections (hnsw:space=cosine)
- SKIP_PATTERNS/JUNK_FILE_SIZE filter minified JS, lockfiles, large dumps
- detect_room() uses word-boundary regex and exact-match priority
- chunk_text() accepts configurable size/overlap/min params
- Entity detector STOPWORDS expanded (73 technical terms)
- Configurable chunk_size/chunk_overlap/min_chunk_size in palace config

Split from MemPalace#562 per maintainer request. Closes MemPalace#483, MemPalace#568.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@arnoldwender
Copy link
Copy Markdown
Contributor Author

arnoldwender commented Apr 13, 2026

Incorporated into #562 by @jphein — closing.

jphein added a commit to jphein/mempalace that referenced this pull request Apr 13, 2026
@jphein @claude
perf: batch ChromaDB writes, concurrent mining, bulk mtime pre-fetch
9dda557 
- Batch upserts (100 docs/call) in both general miner and convo_miner
  instead of one-at-a-time writes — 3-5x faster on large projects
- _prepare_file() separates read/chunk (thread-safe) from write,
  enabling ThreadPoolExecutor concurrency with --workers flag
- bulk_check_mined() pre-fetches all source_file/mtime pairs in one
  paginated scan instead of per-file ChromaDB queries
- Epsilon mtime comparison (abs < 0.01) instead of float == (MemPalace#483)
- Cosine distance default for new collections (hnsw:space=cosine)
- SKIP_PATTERNS/JUNK_FILE_SIZE filter minified JS, lockfiles, large dumps
- detect_room() uses word-boundary regex and exact-match priority
- chunk_text() accepts configurable size/overlap/min params
- Entity detector STOPWORDS expanded (73 technical terms)
- Configurable chunk_size/chunk_overlap/min_chunk_size in palace config

Split from MemPalace#562 per maintainer request. Closes MemPalace#483, MemPalace#568.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milla-jovovich milla-jovovich Awaiting requested review from milla-jovovich milla-jovovich is a code owner

@bensig bensig Awaiting requested review from bensig bensig is a code owner

@igorls igorls Awaiting requested review from igorls igorls is a code owner

1 more reviewer

@web3guru888 web3guru888 web3guru888 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arnoldwender @jphein @web3guru888 @z3tz3r0