Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix save/load_word2vec_format methods for FastText model. Fix #1743 #1755

Merged
merged 8 commits into from
Dec 6, 2017

Conversation

manneshiva
Copy link
Contributor

This PR addresses #1743 by disabling load_word2vec_format call for FastText models.

@manneshiva manneshiva changed the title Fixes save/load_word2vec_format for FastText models [MRG] Fixes save/load_word2vec_format for FastText models Dec 4, 2017
@@ -243,6 +243,14 @@ def word_vec(self, word, use_norm=False):
def load_fasttext_format(cls, *args, **kwargs):
return Ft_Wrapper.load_fasttext_format(*args, **kwargs)

@classmethod
def load_word2vec_format(cls, fname, fvocab=None, binary=False, encoding='utf8', unicode_errors='strict',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary? Personally, I would leave this out. I don't see any problem with getting a "deprecation error" instead of "not implemented error". Plus, it's weird to call a method that you know will fail. This is likely to cause more confusion than good. @menshikh-iv, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Removing this function from here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janpom agree with you

@menshikh-iv
Copy link
Contributor

@manneshiva please resolve merge conflict

@@ -137,6 +137,12 @@ def __contains__(self, word):
char_ngrams = compute_ngrams(word, self.min_n, self.max_n)
return any(ng in self.ngrams for ng in char_ngrams)

@classmethod
def load_word2vec_format(cls, fname, fvocab=None, binary=False, encoding='utf8', unicode_errors='strict',
Copy link
Contributor

@jayantj jayantj Dec 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny change I might suggest here would be to use *args and **kwargs instead. I like following this practice in situations where the subclass doesn't make use of the function params itself. It is useful because if any params in the parent class change in the future, the subclass function signature doesn't have to be modified.

@menshikh-iv menshikh-iv changed the title [MRG] Fixes save/load_word2vec_format for FastText models Fix save/load_word2vec_format methods for FastText model. Fix #1743 Dec 6, 2017
@menshikh-iv menshikh-iv merged commit 09a16d1 into piskvorky:develop Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants