Repository navigation
Fix zero-vector items corrupting queryItems results - #140
Open
OsamaAnsar wants to merge 1 commit into
Open
OsamaAnsar wants to merge 1 commit into
OsamaAnsar wants to merge 1 commit into
Conversation
normalizedCosineSimilarity returns NaN for a zero-norm vector, and every comparison with NaN is false. In the bounded min-heap used by queryItems a NaN entry at the root could never be replaced, so a zero-vector item could displace real matches from the top K, and the final sort used an inconsistent comparator that could leave the best match out of first place. The heap and the final sort now compare a rank in which NaN becomes -Infinity, so zero-vector items rank below every real match. The score reported for such an item is still NaN.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
ItemSelector.normalizedCosineSimilarityreturnsNaNfor a zero-norm vector (deliberate, and covered by existing tests). Since #124,LocalIndex.queryItemspicks the top K with a bounded min-heap that compares with>/<. Every comparison withNaNis false, so aNaNentry at the heap root can never be replaced, and the finalheap.sort((a, b) => b.distance - a.distance)then uses an inconsistent comparator. A stored zero-vector item can therefore push real matches out of the results, or leave the best match out of first place.Repro on current
main(builtlib/, Node 22): itemsz=[0,0,0],a=[1,0,0],b=[0.9,0.1,0],c=[0,1,0], query[1,0,0]. The independent numpy oracle gives cosine[nan, 1.0, 0.9938837346736189, 0.0]forz, a, b, c, i.e. the expected order isa, b, c, z(NaN last).With this change the same script prints:
The fix
The heap and the final sort compare a
rankinstead of the raw score: the same value, exceptNaNbecomes-Infinity. Zero-vector items therefore rank below every real match, and the top K is always chosen from the real scores. The final sort compares explicitly (b.rank - a.rankwould itself beNaNfor two-Infinityranks).Choices I made, in case you prefer otherwise:
normalizedCosineSimilarity/cosineSimilarityare untouched; they still returnNaN.score: NaN) when fewer than K items have a real score, which matches what callers got before. I did not drop them from the results, to avoid changing result counts. SkippingNaNitems entirely would be a one-line change if you would rather have that.NaN; behavior there is unchanged (the first K items in index order are returned).Tests
New
queryItems>zero-norm vectorsspecs insrc/LocalIndex.spec.ts:topK1 to 5, must returna, b, c, ztruncated totopKNaNscore when it is returnedVerification:
yarn lint: 0 errorsyarn test: 596 passing (593 onmain)src/LocalIndex.tsreverted tomain, the 3 new tests fail; with the fix restored all 596 pass.docs/changelog.mdhas a new Unreleased entry. No public API change, sollms.txtis untouched.CONTRIBUTING suggests opening an issue for bugs; I went straight to a PR with the repro above, but I'm happy to open an issue as well if you prefer.