Repository: piskvorky/gensim
Pull request: Change similarity strategy when finding n best
Fixes #2584 

Problem statement:
gensim.similarities.Similarity merges results from shards incorrectly (LSI model)
If "num_best" is used, `gensim.similarities.Similarity` runs the query against each of the shards (MatrixSimilarity objects) and then merges the results.

MatrixSimilarity uses `matutils.full2sparse_clipped()` to pick "num_best" results which sorts by the absolute value.

gensim.similarities.Similarity on the other hand, just uses `heapq.nlargest` (in `__getitem__`) to merge the results from each of the shards. So negative sims are either pushed down the list or cut off completely.

Hints from issue discussion:
You're right. This seems an inconsistency / bug introduced by PR #811.

@avoskresensky in your use case – is `abs` desirable, or is the raw similarity desirable?
If my assumption that it's the absolute values that indicate similarity is correct, then I don't care that much about the raw values. `abs` values are okay. That's how we ended up "fixing" this inconsistency in out current implementation: we're just abs'ing the results in our `Similarity.query_shards` override.

But if I'm wrong and sign of the sim values does mean some form of dissimilarity, then the whole idea of sorting while merging becomes questionable.
Negative similarity values (close to -1) mean "opposite vectors", which is can also be interpreted as "very similar", because it indicates a strong semantic connection between the two inputs. Cossim values around 0.0 mean perpendicular = unrelated.

Whether "opposite" really means "similar" or not will depend on how you generated your vectors, and what you're using them for. That's why I ask: this is an "upstream" question, not a technical one.

Either way: can you open a PR that fixes Similarity to sort on `abs` values as well? We definitely want this logic consistent across the similarity classes. Cheers.
@avoskresensky Ping on this. Are you able to make a PR?
Not in the next couple of weeks.

Commit message: Find largest by absolute value

Patch to review:
diff --git a/gensim/similarities/docsim.py b/gensim/similarities/docsim.py
index 692d76e18a..b7a244e630 100755
--- a/gensim/similarities/docsim.py
+++ b/gensim/similarities/docsim.py
@@ -539,7 +539,7 @@ def convert(shard_no, doc):
             if not is_corpus:
                 # user asked for num_best most similar and query is a single doc
                 results = (convert(shard_no, result) for shard_no, result in enumerate(shard_results))
-                result = heapq.nlargest(self.num_best, itertools.chain(*results), key=lambda item: item[1])
+                result = heapq.nlargest(self.num_best, itertools.chain(*results), key=lambda item: abs(item[1]))
             else:
                 # the trickiest combination: returning num_best results when query was a corpus
                 results = []
@@ -548,7 +548,7 @@ def convert(shard_no, doc):
                     results.append(shard_result)
                 result = []
                 for parts in zip(*results):
-                    merged = heapq.nlargest(self.num_best, itertools.chain(*parts), key=lambda item: item[1])
+                    merged = heapq.nlargest(self.num_best, itertools.chain(*parts), key=lambda item: abs(item[1]))
                     result.append(merged)
         if pool:
             # gc doesn't seem to collect the Pools, eventually leading to


Code review comments:
diff_hunk: @@ -539,7 +539,7 @@ def convert(shard_no, doc):
             if not is_corpus:
                 # user asked for num_best most similar and query is a single doc
                 results = (convert(shard_no, result) for shard_no, result in enumerate(shard_results))
-                result = heapq.nlargest(self.num_best, itertools.chain(*results), key=lambda item: item[1])
+                result = heapq.nlargest(self.num_best, itertools.chain(*results), key=lambda item: abs(item[1]))
path: gensim/similarities/docsim.py
text: @user1:
This is getting a little bit too hairy. We're multiple indentation levels in, and we're duplicating a bit of code. Let's split out a function:

```python
def _nlargest(n, results):
    return heapq.nlargest(n, itertools.chain(*results), key=lambda item: abs(item[1]))
```

and then use it inside as:

```suggestion
                result = _nlargest(self.num_best, results)
```

@user2:
Can `lambda` accept a tuple argument? If so, `key=lambda (feature, weight): abs(weight)` more readable than `item[1]`.

@author:
@user2 Unfortunately it was removed from python3 (you can check it [here](https://www.python.org/dev/peps/pep-3113/))

@user1 I guess `_nlargest()` can be tested, whereas creating test for the whole `Similarity` object is complicated (I guess it involves changing the corpus, that is used for testing and fixing all other test OR maybe adding explicit test and corpus for `Similarity` that checks, that right objects are extracted with `_nlargest`). Which is more preferable? 

@user1:
Write tests for nlargest for now.