Skip to content

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

Merged
tmylk merged 4 commits into
piskvorky:developfrom
chinmayapancholi13:minimize_model
Mar 12, 2017
Merged

Allows training if model is not modified by "_minimize_model". Adds deprecation warning.#1207
tmylk merged 4 commits into
piskvorky:developfrom
chinmayapancholi13:minimize_model

Conversation

@chinmayapancholi13
Copy link
Copy Markdown
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.

Comment thread gensim/models/word2vec.py Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread gensim/models/word2vec.py Outdated
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
Copy Markdown
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.

Comment thread gensim/models/word2vec.py Outdated
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
Copy Markdown
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

Comment thread gensim/models/word2vec.py Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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