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

Allows training if model is not modified by "_minimize_model". Adds deprecation warning. #1207

Merged
merged 4 commits into from
Mar 12, 2017

Conversation

chinmayapancholi13
Copy link
Contributor

@chinmayapancholi13 chinmayapancholi13 commented Mar 12, 2017

This PR modifies the function _minimize_model in word2vec.py by checking if model was modified at all before setting model_trimmed_post_training. Also, a deprecation warning has been added for the function _minimize_model. The function would be deprecated in the future as using KeyedVectors is the way to go for word2vec.

Maling list discussion.

self.model_trimmed_post_training = True
if not save_syn1 or not save_syn1neg or not save_syn0_lockf:
self.model_trimmed_post_training = True
logger.warning("This method would be deprecated in the future. Use KeyedVectors to retain the previously trained word vectors instead.")
Copy link
Owner

Choose a reason for hiding this comment

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

warning.warn better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. Thanks for your suggestion. I am making this change in the PR.

Copy link
Contributor

@tmylk tmylk left a comment

Choose a reason for hiding this comment

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

Please fix the logic

@@ -1251,7 +1251,10 @@ def _minimize_model(self, save_syn1 = False, save_syn1neg = False, save_syn0_loc
del self.syn1neg
if hasattr(self, 'syn0_lockf') and not save_syn0_lockf:
del self.syn0_lockf
self.model_trimmed_post_training = True
if not save_syn1 or not save_syn1neg or not save_syn0_lockf:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean when all 3 are true? Then use if (save_syn1 and save_syn1neg and save_syn0_lockf) and just return. Please move this check to the top of the function.

self.model_trimmed_post_training = True
if not save_syn1 or not save_syn1neg or not save_syn0_lockf:
self.model_trimmed_post_training = True
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move imports to the file header

if not save_syn1 or not save_syn1neg or not save_syn0_lockf:
self.model_trimmed_post_training = True
import warnings
warnings.warn("This method would be deprecated in the future. Use KeyedVectors to retain the previously trained word vectors instead.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move to the top of the function. Better text is "This method would be deprecated in the future. Keep just_word_vectors = model.wv to retain just the KeyedVectors instance for read-only querying of word vectors."

@chinmayapancholi13
Copy link
Contributor Author

@tmylk Thanks for the suggestions. I have updated the code to reflect these changes.

@tmylk tmylk merged commit dd396e3 into piskvorky:develop Mar 12, 2017
@tmylk
Copy link
Contributor

tmylk commented Mar 12, 2017

Thanks for the pr

pranaydeeps pushed a commit to pranaydeeps/gensim that referenced this pull request Mar 21, 2017
@chinmayapancholi13 chinmayapancholi13 deleted the minimize_model branch May 23, 2017 17:53
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.

3 participants