model.wv.accuracy messes up the model object

[ahmedahmedov]

I was playing around with model.wv.accuracy to test the accuracy of my model. It worked fine when the only parameter I set was the filename:

model.wv.accuracy("questions-words.txt")
Things get weird though when I set the 'most_similar' parameter of the function:

model.wv.accuracy("questions-words.txt", most_similar=gensim.models.KeyedVectors.most_similar_cosmul)

First, it gives an error saying that gensim.models.KeyedVectors.most_similar_cosmul doesn't have a parameter named 'retrict_vocabulary'.

Most importantly, right after that, the model vocabulary is messed up for some reason. That is, when calling model.wv.accuracy("question-words.txt") again, I get the following error:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-36-41dd7cacf846> in <module>()
----> 1 model.wv.accuracy("questions-words.txt", most_similar=gensim.models.KeyedVectors.most_similar_cosmul)
      2 model.wv.evaluate_word_pairs("wordsim353.tsv")

/home/user/anaconda3/lib/python3.5/site-packages/gensim/models/keyedvectors.py in accuracy(self, questions, restrict_vocab, most_similar, case_insensitive)
    772 
    773         """
--> 774         ok_vocab = [(w, self.vocab[w]) for w in self.index2word[:restrict_vocab]]
    775         ok_vocab = {w.upper(): v for w, v in reversed(ok_vocab)} if case_insensitive else dict(ok_vocab)
    776 

/home/user/anaconda3/lib/python3.5/site-packages/gensim/models/keyedvectors.py in <listcomp>(.0)
    772 
    773         """
--> 774         ok_vocab = [(w, self.vocab[w]) for w in self.index2word[:restrict_vocab]]
    775         ok_vocab = {w.upper(): v for w, v in reversed(ok_vocab)} if case_insensitive else dict(ok_vocab)
    776 

KeyError: 'the'

calling model.wv.most_similar also gives the same error.

[ahmedahmedov]

I think I know why it happened. For some reason, when setting case_insensitive to True, the vocabulary is converted to upper case. model.accuracy should not make any changes to the internal state of the model.

 original_vocab = self.vocab
 self.vocab = ok_vocab
 ignore = {a, b, c}  # input words to be ignored
 predicted = None
 # find the most likely prediction, ignoring OOV words and input words
 sims = most_similar(self, positive=[b, c], negative=[a], topn=False, restrict_vocab=restrict_vocab)
 self.vocab = original_vocab

The problem is, when sims = most_similar(self, positive=[b, c], negative=[a], topn=False, restrict_vocab=restrict_vocab) fails, self.vocab is not set back to the original vocabulary, thus the model object ends up with the restricted, upper_cased vocabulary.

Again, I think that it is not a good idea to change the internal state of the object. Instead, the restricted vocabulary should either be sent as a parameter or initiated as another field in the class.

[piskvorky]

Definitely a bug. I'd also expect the accuracy function to be 100% "const" (not modifying the object). Thanks for reporting! Can you submit a PR with a fix?

[ahmedahmedov]

I am guessing in order to fix the problem while keeping the "case_insentive" parameter intact, the most_similar and most_similar_consum functions need to be modified completely since these functions do not have "case_insensitive" parameter themselves, which, I think, is a minus. It would help greatly if we could search the vocabulary while ignoring the case.

I do not know how that would solve "collisions".
That is, for instance, let's say I search for Apple. In the vocabulary, I have "Apple", "apple", as well as "APPLE". When searching for most_similar("Apple", case_insensitive=True) which one should the method search? Or, can we say that only do case_insentive search when we encounter "KeyError"?

I think this problem needs a more fundamental solution than just a patch.

[piskvorky]

1. (b - a) + c ~= d is the idea behind analogies. You determine the relative vector (b-a) and apply it to a new origin point (c).

2. reverse=True means we want to loop over the most similar words first = the highest value in sims.

3. not for each -- I think you missed the break. Once an acceptable word is found, where acceptable means # find the most likely prediction, ignoring OOV words and input words, the loop ends.

4. see above

5. correct.

I don't see any issue with the code, the comments are OK too. What do you see as the problem?

[ahmedahmedov]

yeah, sorry, I deleted the comment later. I was a little hasty to post it.

[piskvorky]

Regarding the Apple/apple/APPLE: I'm in favour of doing what the original word2vec tool did. Which is use the most frequent word that fits. The words are pre-ordered by their frequency in the file format, for this purpose.

[menshikh-iv]

Hello @ahmedahmedov

First, it gives an error saying that gensim.models.KeyedVectors.most_similar_cosmul doesn't have a parameter named 'retrict_vocabulary'.

yes, the signature of most_similar function should be like https://github.com/RaRe-Technologies/gensim/blob/5677ab300e4e3dc4645806762b693902b97c13c3/gensim/models/keyedvectors.py#L363

more accurately (as this used in accuracy)

sims = most_similar(self, positive=[b, c], negative=[a], topn=False, restrict_vocab=restrict_vocab)

most_similar_cosmul doesn't match to this signature (probably should be, looks like a bug for me).
I also agree with Radim (about internal state), can you make a PR with fix @ahmedahmedov (this will be really great)?

[ahmedahmedov]

I would like to suggest to replace https://github.com/RaRe-Technologies/gensim/blob/5677ab300e4e3dc4645806762b693902b97c13c3/gensim/models/keyedvectors.py#L979-L992

with

predicted = None
# find the most likely prediction, ignoring OOV words and input words
predicted_tuple = model.most_similar(positive=[b, c], negative=[a], topn=1, restrict_vocab=restrict_vocab)
predicted = predicted_tuple[0].upper() if case_insensitive else predicted_tuple[0]
model.wv.vocab = original_vocab

if predicted not in ignore and predicted==expected:
    section['correct'].append((a, b, c, word[0]))

else:
    section['incorrect'].append((a, b, c, expected))

The idea is, there is no need to calculate similarity stuff separately (unless there is an important reason behind this design choice). It is already done inside the most_similar method. Just set topn=1 and see if the result matches the expected value. If yes, then update section['correct'], if not update section['incorrect']. In addition, we can set topn as a parameter to accuracy to give users the flexibility to decide what they consider as accurate.

[ahmedahmedov]

it seems that case_insensitive dictionary search is not that easy in Python. Any ideas how we might find a workaround to modify the most_similar method to be able to search in a case_insensitive manner without having to create an additional vocabulary?
Python dictionary case_insensitive search

[piskvorky]

How does your suggestion handle the ignoring OOV words and input words?

[ahmedahmedov]

if predicted not in ignore and predicted==expected handles input words. If the word is not found in the vocabulary, we can simply skip it, right? Then, surrounding most_similar with try-except clause and ignoring when KeyIsNotFound exception is raised should solve the OOV case.

[piskvorky]

I'd say no, that would change the logic in a fundamental way. We want to keep the "standard" logic from the C tool, which looks for the nearest word that is not input/OOV.

[ahmedahmedov]

@piskvorky I think I will need to analyze the code more thoroughly to clear my confusion about how the accuracy is really evaluated. Once clear, I will explain how I see the problem should be solved in a holistic manner.

To iterate over the problem again:

1. The model.accuracy method has two parameters - restrict_vocabulary and case_insensitive which cause a few problems.
2. There are two methods used for word analogy: most_similar and most_similar_cosmul. These two should have identical parameter signatures. Currently, most_similar_cosmul doesn't have restrict_vocabulary parameter which results in an error if the most_similar parameter of the model.accuracy method is set to most_similar_cosmul.
3. Currently, in order to have the case_insensitive parameter work, model.accuracy modifies model.vocabulary (which it should not do absolutely) by creating a clone of the vocabulary, all uppercased, and assigning it to model.vocabulary while keeping the original vocabulary in a temporary variable. After the accuracy is computed, it then assigns the original vocabulary back to method.vocabulary. This step messes up the model if, for some reason, the accuracy method is halted during the execution or results in a runtime error.
4. There are two options to solve the case_insentive problem without messing up the model: First one is easier: add the case_insensitive parameter to the model.most_similar and model.most_similar_cosmul methods, then create a copy of the vocabulary, all uppercased, finally search thru the newly created uppercased_vocabulary. This will result in an additional (potentially large) memory usage. Besides, it is not clear what happens to the vocabulary items which become the same after uppercased? Which vector should be kept? The second one is more difficult and needs a more thorough discussion: how to search the original vocabulary in a case_insensitive manner, without having to create a copy of it? Case_insensitive dictionary search in python is problematic. What to do if we have several matches for a case_insensitive search? (Apple vs APPLE vs apple vs aPPle etc. ). Which one to return?

I will edit this comment to add more remarks as soon as get some more time.