Fix #1230. Fix word2vec reset_from bug in v1.0.1 and added unittest#1234
Conversation
Kreiswolke
changed the title
|
Please fix the typo to make unit tests pass. |
tmylk
changed the title
|
Typo fixed, tests passed. |
| """Test if exception is raised when reset_from is used""" | ||
| model = word2vec.Word2Vec(sentences, min_count=1) | ||
| model2 = word2vec.Word2Vec(new_sentences, min_count=1) | ||
| self.assertRaises(AttributeError, model.reset_from(model2)) |
There was a problem hiding this comment.
why does it raise an error? A positive outcome is expected. Please change this test to a positive test.
| try: | ||
| model.reset_from(model2) | ||
| except AttributeError: | ||
| self.fail('AttributeError in reset_from()') |
There was a problem hiding this comment.
what is the purpose of this line? what would be different if it was removed?
| self.assertRaises(AttributeError, load_on_instance) | ||
|
|
||
| def test_reset_from(self): | ||
| """Test if exception is raised when reset_from is used""" |
There was a problem hiding this comment.
is there some reason that exception should be raised when reset_from is used? correct behavious is no exception. please add checks that the model is actually reset.
| try: | ||
| model.reset_from(model2) | ||
| except AttributeError: | ||
| self.fail('AttributeError in reset_from()') |
There was a problem hiding this comment.
Because an uncaught error will result in a test failure, this catch-then-fail seems superfluous - it's enough to try the reset_from(), and do some afterwards checking that it's had the intended effect.
There was a problem hiding this comment.
I agree, it is not needed and I just updated the test.
|
Thanks for the fix! |
|
Happy I could help ;) |
3 participants
Fixed reset_from attribute bug in gensim 1.0.1 version.