Fix 1651: make word_vec() return immutable vector #1662
Conversation
gensim/models/keyedvectors.py
Outdated
| else: | ||
| return self.syn0[self.vocab[word].index] | ||
| else: |
There was a problem hiding this comment.
Why you remove this branch? We must raise an exception if word not in the dictionary.
gensim/models/keyedvectors.py
Outdated
| raise KeyError("word '%s' not in vocabulary" % word) | ||
| result = self.syn0[self.vocab[word].index] | ||
|
|
||
| result.setflags(write=False) |
There was a problem hiding this comment.
Please add test to check this behaviour (with Exception catching)
gensim/test/test_doc2vec.py
Outdated
| @@ -386,6 +386,12 @@ def test_delete_temporary_training_data(self): | |||
| self.model_sanity(model, keep_training=False) | |||
| self.assertTrue(hasattr(model, 'syn1neg')) | |||
|
|
|||
| def test_word_vec(self): | |||
There was a problem hiding this comment.
More-descriptive name would be better, such as test_word_vec_non_writeable().
|
Ideally PR names should be descriptive even if issue-numbers (like |
|
On github, PR titles do not link through to the issues (the pingback doesn't appear in the linked issue, no easy to way to visit the issue from the PR). Please use the native Github formatting, it makes maintenance easier. |
|
Thanks @CLearERR, congratz with the first PR 🥇 |
fix for issue about mutable vector returned by KeyedVectors.word_vector