Filter duplicate rows when pruning vectors - #14028
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
Vocab.prune_vectors builds its keep/toss split from one priority entry per lexeme, but more than one key can point at the same vector row (for example after Vectors.add(key, row=...) aliases a spelling or casing variant onto an existing row instead of duplicating the data). When that happens the shared row can end up copied twice into the pruned table, wasting a slot that could have held a distinct vector, or split across the kept and discarded halves so one of the aliased keys gets sent through the nearest-neighbour search even though its exact vector is still present among the rows that were kept. This collapses the priority list down to one entry per row before deciding what to keep, then restores every aliased key afterwards by pointing it at wherever its row's representative key ended up, whether that was kept in place or remapped to a synonym. Fixes explosion#5397
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.
Fixes #5397
Vocab.prune_vectorsbuilds its keep/discard split from one priority entry per lexeme, but more than one key can point at the same vector row. That happens for example afterVectors.add(key, row=...)aliases a spelling or casing variant onto an existing row instead of duplicating the data, which is a common way to save memory in real vector tables.When two keys share a row, the current code can end up copying that row twice into the pruned table (wasting a slot that could have held a genuinely distinct vector), or splitting the pair across the kept and discarded halves so one of them gets sent through the nearest-neighbour search below even though its exact vector is still sitting right there among the rows that were kept.
This collapses the priority list down to one entry per row before deciding what to keep, then afterwards restores every aliased key by pointing it at wherever its row's representative key ended up, whether that representative was kept in place or itself remapped to a synonym.
Added
test_vocab_prune_vectors_duplicate_rowsalongside the existingtest_vocab_prune_vectors: it builds a vocab wherekitty/puppyalias the same rows ascat/dog, and checks that pruning to 2 rows keeps both pairs' vectors intact (rather than wasting a slot duplicating one of them) while the one truly distinct vector still gets remapped to its nearest neighbour as before.I reverted just the
vocab.pyxchange locally and confirmed the new test fails with the exact symptom described in the issue (dog/puppy's shared row gets split, withpuppylanding in the remap output alongside the genuinely discarded word) before restoring the fix.Ran the full
spacy/tests/vocab_vectors/suite (106 passed) andcython-lintonvocab.pyx(same 19 pre-existing warnings as upstream master, no new ones introduced).