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

KeyedVecs refactoring for word2vec #980

Merged
merged 43 commits into from
Nov 13, 2016
Merged

Conversation

anmolgulati
Copy link
Contributor

Adresses #833.

droudy and others added 30 commits September 12, 2016 19:06
Conflicts:
	CHANGELOG.md
	gensim/models/word2vec.py

@property
def vocab(self):
return self.wv.vocab
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a warning here?

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, there should be. I've added warnings now.


@property
def index2word(self):
return self.wv.index2word
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a warning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here. Added warnings now.

@tmylk
Copy link
Contributor

tmylk commented Oct 31, 2016

@anmol01gulati Please resolve the merge conflict by pulling in develop

@anmolgulati
Copy link
Contributor Author

I've made the changes as suggested. Please review once.

@@ -100,15 +101,16 @@
from types import GeneratorType

logger = logging.getLogger(__name__)

logging.basicConfig(format='%(asctime)s : %(levelname)s : %(message)s')
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should be present here, logging config is usually left up to either __main__ or someone using the module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @jayantj See #910

Copy link
Contributor

@tmylk tmylk Nov 11, 2016

Choose a reason for hiding this comment

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

See #910 to see why it was removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed logging config now.

except ImportError:
# failed... fall back to plain numpy (20-80x slower training than the above)
# Can't log here because logger is usually not configured at import time
logger.warning('Slow version of {0} is being used'.format(__name__))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work fine? (the removed comment seems to indicate otherwise, just making sure)

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed too @anmol01gulati

@@ -311,23 +364,39 @@ def testLocking(self):
model.build_vocab(corpus)

# remember two vectors
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

A few merge conflicts below too need to be resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

try:
from gensim.models.word2vec_inner import train_batch_sg, train_batch_cbow
from gensim.models.word2vec_inner import score_sentence_sg, score_sentence_cbow
from gensim.models.word2vec_inner import FAST_VERSION, MAX_WORDS_IN_BATCH
logger.debug('Fast version of {0} is being used'.format(__name__))
Copy link
Contributor

Choose a reason for hiding this comment

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

See #910 for a proper fix

@anmolgulati
Copy link
Contributor Author

@tmylk I've removed the log config and logging statements. Please review once, and I think we could merge it then.

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