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

Fixed wrapper tests #1323 #1359

Merged
merged 3 commits into from
May 25, 2017
Merged

Conversation

shubhamdotjain
Copy link
Contributor

No description provided.

@menshikh-iv
Copy link
Contributor

Thank you @shubhamjain74, It's a nice start 👍
I check this PR with full test run on 2.7 and 3.5

@menshikh-iv menshikh-iv merged commit b2f15ff into piskvorky:develop May 25, 2017
@menshikh-iv menshikh-iv mentioned this pull request May 25, 2017
@@ -281,6 +281,7 @@ def finalize_options(self):
'scipy >= 0.7.0',
'six >= 1.5.0',
'smart_open >= 1.2.1',
'morfessor==2.0.2alpha4',
Copy link
Owner

Choose a reason for hiding this comment

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

-1: why should this be a hard dependency??

Has this been thoroughly vetted, by @tmylk and @gojomo ? Is morfessor really a core, indispensable part of gensim now, with all that it entails (we'll have to support its installation problems etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's dependency for VarEmbed wrapper. Maybe need to create several mode for install.

Copy link
Owner

Choose a reason for hiding this comment

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

Definitely. See the optional extras_require setup param.

@@ -190,7 +190,7 @@ def load_word_topics(self):
if hasattr(self.id2word, 'token2id'):
word2id = self.id2word.token2id
else:
word2id = dict((v, k) for k, v in iteritems(self.id2word))
word2id = dict((v, k) for k, v in iteritems(dict(self.id2word)))
Copy link
Owner

@piskvorky piskvorky May 27, 2017

Choose a reason for hiding this comment

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

What is this dict casting for?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's for python3 compatibility (if self.id2word is FakeDict object, iteritems can't work correctly)

Copy link
Owner

Choose a reason for hiding this comment

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

Let's update utils.revdict then, and use that consistently. There are probably more places like this in the code.

@@ -93,7 +93,7 @@ def __init__(
lencorpus = sum(1 for _ in corpus)
if lencorpus == 0:
raise ValueError("cannot compute DTM over an empty corpus")
if model == "fixed" and any([i == 0 for i in [len(text) for text in corpus.get_texts()]]):
if model == "fixed" and any([i == 0 for i in [len(text) for text in corpus]]):
Copy link
Owner

Choose a reason for hiding this comment

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

No need for the outer brackets [ and ] (generator fine).

But the whole construction feels unpythonic and strange -- simple any(not text for text in corpus) better?

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