Repository: piskvorky/gensim
Pull request: streamlining most_similar_cosmul and evaluate_word_analogies
Closes: https://github.com/RaRe-Technologies/gensim/issues/2535

This is my first PR for gensim so all comments are welcome. 
To be honest I have no idea how to test `restrict_vocab` for `most_similar_cosmul` or `most_similar` for `evaluate_word_analogies`. I wanted to write something similar to already existing tests for these keywords but did not find any (nor tests for `restrict_vocab` keyword in case of `most_similar` function and nor `most_similar` keyword in case of `accuracy` function)

Summary:

- Added new `restrict_vocab` parameter to `most_similar_cosmul`
- Improved `most_similar_cosmul` shorthand to handle both positive and negative cases
- Parameterized similarity function in `evaluate_word_analogies`

Problem statement:
streamlining most_similar_cosmul
this is a feature request to make most_similar_cosmul() more similar (pun intended) to most_similar() by the following changes:

1. add restrict_vocab argument
2. allow positive and negative to be strings instead of list-of-strings in case of single words
3. allow usage in evaluate_word_analogies

Hints from issue discussion:
Hi, can I have a go at this?
Yes.

Commit message: streamlining most_similar_cosmul

Patch to review:
diff --git a/gensim/models/keyedvectors.py b/gensim/models/keyedvectors.py
index 467b740ed7..243a4e2a26 100644
--- a/gensim/models/keyedvectors.py
+++ b/gensim/models/keyedvectors.py
@@ -777,7 +777,9 @@ def nbow(document):
         # Compute WMD.
         return emd(d1, d2, distance_matrix)
 
-    def most_similar_cosmul(self, positive=None, negative=None, topn=10):
+    def most_similar_cosmul(
+            self, positive=None, negative=None, topn=10, restrict_vocab=None
+        ):
         """Find the top-N most similar words, using the multiplicative combination objective,
         proposed by `Omer Levy and Yoav Goldberg "Linguistic Regularities in Sparse and Explicit Word Representations"
         <http://www.aclweb.org/anthology/W14-1618>`_. Positive words still contribute positively towards the similarity,
@@ -799,6 +801,11 @@ def most_similar_cosmul(self, positive=None, negative=None, topn=10):
         topn : int or None, optional
             Number of top-N similar words to return, when `topn` is int. When `topn` is None,
             then similarities for all words are returned.
+        restrict_vocab : int or None, optional
+            Optional integer which limits the range of vectors which are searched for most-similar values.
+            For example, restrict_vocab=10000 would only check the first 10000 node vectors in the vocabulary order.
+            This may be meaningful if vocabulary is sorted by descending frequency.
+
 
         Returns
         -------
@@ -818,10 +825,13 @@ def most_similar_cosmul(self, positive=None, negative=None, topn=10):
 
         self.init_sims()
 
-        if isinstance(positive, string_types) and not negative:
+        if isinstance(positive, string_types):
             # allow calls like most_similar_cosmul('dog'), as a shorthand for most_similar_cosmul(['dog'])
             positive = [positive]
 
+        if isinstance(negative, string_types):
+            negative = [negative]
+
         all_words = {
             self.vocab[word].index for word in positive + negative
             if not isinstance(word, ndarray) and word in self.vocab
@@ -841,8 +851,9 @@ def most_similar_cosmul(self, positive=None, negative=None, topn=10):
 
         # equation (4) of Levy & Goldberg "Linguistic Regularities...",
         # with distances shifted to [0,1] per footnote (7)
-        pos_dists = [((1 + dot(self.vectors_norm, term)) / 2) for term in positive]
-        neg_dists = [((1 + dot(self.vectors_norm, term)) / 2) for term in negative]
+        vectors_norm = self.vectors_norm[:restrict_vocab] if restrict_vocab else self.vectors_norm
+        pos_dists = [((1 + dot(vectors_norm, term)) / 2) for term in positive]
+        neg_dists = [((1 + dot(vectors_norm, term)) / 2) for term in negative]
         dists = prod(pos_dists, axis=0) / (prod(neg_dists, axis=0) + 0.000001)
 
         if not topn:
@@ -1017,7 +1028,9 @@ def _log_evaluate_word_analogies(section):
             logger.info("%s: %.1f%% (%i/%i)", section['section'], 100.0 * score, correct, correct + incorrect)
             return score
 
-    def evaluate_word_analogies(self, analogies, restrict_vocab=300000, case_insensitive=True, dummy4unknown=False):
+    def evaluate_word_analogies(
+            self, analogies, restrict_vocab=300000, case_insensitive=True,
+            dummy4unknown=False, most_similar=most_similar):
         """Compute performance of the model on an analogy test set.
 
         This is modern variant of :meth:`~gensim.models.keyedvectors.WordEmbeddingsKeyedVectors.accuracy`, see
@@ -1046,6 +1059,8 @@ def evaluate_word_analogies(self, analogies, restrict_vocab=300000, case_insensi
         dummy4unknown : bool, optional
             If True - produce zero accuracies for 4-tuples with out-of-vocabulary words.
             Otherwise, these tuples are skipped entirely and not used in the evaluation.
+        most_similar : function, optional
+            Function used for similarity calculation.
 
         Returns
         -------
@@ -1098,7 +1113,7 @@ def evaluate_word_analogies(self, analogies, restrict_vocab=300000, case_insensi
                     predicted = None
                     # find the most likely prediction using 3CosAdd (vector offset) method
                     # TODO: implement 3CosMul and set-based methods for solving analogies
-                    sims = self.most_similar(positive=[b, c], negative=[a], topn=5, restrict_vocab=restrict_vocab)
+                    sims = most_similar(self, positive=[b, c], negative=[a], topn=5, restrict_vocab=restrict_vocab)
                     self.vocab = original_vocab
                     for element in sims:
                         predicted = element[0].upper() if case_insensitive else element[0]
diff --git a/gensim/test/test_fasttext.py b/gensim/test/test_fasttext.py
index 4a5626eb30..3df7bdb1a9 100644
--- a/gensim/test/test_fasttext.py
+++ b/gensim/test/test_fasttext.py
@@ -363,6 +363,9 @@ def test_most_similar_cosmul(self):
         self.assertEqual(
             self.test_model.wv.most_similar_cosmul('nights'),
             self.test_model.wv.most_similar_cosmul(positive=['nights']))
+        self.assertEqual(
+            self.test_model.wv.most_similar_cosmul('the', 'and'),
+            self.test_model.wv.most_similar_cosmul(positive=['the'], negative=['and']))
 
     def test_lookup(self):
         # In vocab, sanity check


Code review comments:
diff_hunk: @@ -818,10 +825,13 @@ def most_similar_cosmul(self, positive=None, negative=None, topn=10):
 
         self.init_sims()
 
-        if isinstance(positive, string_types) and not negative:
+        if isinstance(positive, string_types):
             # allow calls like most_similar_cosmul('dog'), as a shorthand for most_similar_cosmul(['dog'])
             positive = [positive]
 
+        if isinstance(negative, string_types):
path: gensim/models/keyedvectors.py
text: @user1:
If I understand correctly, this enables behavior like:

    most_similar_cosmul('dog', 'cat')

where `dog` is positive and `cat` is negative. That's helpful shorthand, but without documentation, people won't find out about it.

Can you please add a paragraph to the docstring explaining the above shorthand?

@user2:
Note that this just makes the special type-testing treatment of `negative` match that of `positive` (in both `most_similar()` and `most_similar_cosmul()`) – but that special treatment, while used extensively in examples, isn't currently documented even in the `most_similar()` case! I'd suggest that treating `negative` symmetrically with `positive` is a good idea, and should also be done in `most_similar()` for consistency, and both of their doc-comments should be improved/harmonized to explain this behavior. 

@author:
@user1 added shorthand in docstring for clarity, is it okay or is there some other place for docs as well that this should be added?