Skip to content

Utils and Matutils changes#1062

Merged
tmylk merged 3 commits into
piskvorky:developfrom
bhargavvader:utils
Dec 28, 2016
Merged

Utils and Matutils changes#1062
tmylk merged 3 commits into
piskvorky:developfrom
bhargavvader:utils

Conversation

@bhargavvader
Copy link
Copy Markdown
Contributor

@bhargavvader bhargavvader commented Dec 27, 2016

  1. Moved dirichlet_expectation to matutils as the same code was being used in both ldamodel and hdpmodel.
  2. Changed all numpy in utils and matutils to np, keeping it uniform across the package.
  3. Fixed hanging indent.
  4. Changed import conventions in hdpmodel to make it consistent with ldamodel

@bhargavvader bhargavvader mentioned this pull request Dec 27, 2016
Comment thread gensim/utils.py Outdated
if len(doc1) == 0: # sparse documents must have a __len__ function (list, tuple...)
return True, obj # the first document is empty=>assume this is a corpus
id1, val1 = next(iter(doc1)) # if obj is a numpy array, it resolves to False here
id1, val1 = next(iter(doc1)) # if obj is a np array, it resolves to False here
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.

This replacement (and the one above) look unnecessary, the original was clearer.

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.

Cool, will fix it

Comment thread gensim/models/hdpmodel.py Outdated
def lda_e_step(doc_word_ids, doc_word_counts, alpha, beta, max_iter=100):
gamma = np.ones(len(alpha))
expElogtheta = np.exp(dirichlet_expectation(gamma))
expElogtheta = np.exp(matutils.dirichlet_expectation(gamma))
Copy link
Copy Markdown
Contributor

@tmylk tmylk Dec 28, 2016

Choose a reason for hiding this comment

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

better to add from matutils import dirichlet_expectation to avoid this change

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.

Okay, changed

@tmylk tmylk merged commit 49ede4d into piskvorky:develop Dec 28, 2016
@tmylk
Copy link
Copy Markdown
Contributor

tmylk commented Dec 28, 2016

Thanks for the improvement!

Comment thread gensim/utils.py
def run(self):
if self.as_numpy:
import numpy # don't clutter the global namespace with a dependency on numpy
import np # don't clutter the global namespace with a dependency on numpy
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.

Looks like the global module namespace already has numpy, so this line is unnecessary.

Comment thread gensim/utils.py

"""
import numpy
import numpy as np
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.

Already imported at module level, this line no longer unnecessary.

jayantj pushed a commit to jayantj/gensim that referenced this pull request Jan 4, 2017
* Utils, Matutils changes

* Changed comments

* changed matutils import
@bhargavvader bhargavvader deleted the utils branch February 23, 2017 10:34
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