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

Using numpy.random.RandomState instead of numpy.random in LDA #748

Merged
merged 35 commits into from
Jun 25, 2016
Merged

Using numpy.random.RandomState instead of numpy.random in LDA #748

merged 35 commits into from
Jun 25, 2016

Conversation

droudy
Copy link
Contributor

@droudy droudy commented Jun 20, 2016

In reference to #113 , a random_state parameter has been added to the LdaState initializer and a check_random_seed method as well to check the seed. The check_random_state method was taken from maciejkula/glove-python@95a04af , should I rewrite the method?

@droudy droudy changed the title Implementing numpy.random.RandomState instead of numpy.random in LDA Using numpy.random.RandomState instead of numpy.random in LDA Jun 20, 2016
@tmylk
Copy link
Contributor

tmylk commented Jun 21, 2016

Please add tests for this particular function(checking both initializaitons work).

Also need to update existing tests that fix random seed to use this new API, for example https://github.com/RaRe-Technologies/gensim/blob/develop/gensim/test/test_ldamodel.py#L253

@tmylk
Copy link
Contributor

tmylk commented Jun 22, 2016

Just need to add random_state to MultiCore

@@ -586,6 +586,7 @@ def show_topics(self, num_topics=10, num_words=10, log=False, formatted=True):
if num_topics < 0:
num_topics = len(self.data)


Copy link
Contributor

Choose a reason for hiding this comment

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

remove the line

def testRandomState():
testcases = [numpy.random.seed(0), None, numpy.random.RandomState(0), 0]
for testcase in testcases:
assert(isinstance(ldamodel.check_random_state(testcase), numpy.random.RandomState))
Copy link
Contributor

Choose a reason for hiding this comment

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

add a test assertEqual(ldamodel.check_random_state(testcase), numpy.random.RandomState(0))

@tmylk tmylk merged commit 2e0ed26 into piskvorky:develop Jun 25, 2016
@@ -97,7 +98,7 @@ def testAlpha(self):
expected_shape = (2,)

# should not raise anything
self.class_(**kwargs)
self.class_(**kwarghs)
Copy link
Owner

@piskvorky piskvorky Jun 26, 2016

Choose a reason for hiding this comment

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

@droudy @tmylk what is this change? If on purpose, needs a clear comment.

@droudy
Copy link
Contributor Author

droudy commented Jun 26, 2016

@piskvorky it was an accident, it was fixed in 8ea4d58

@droudy droudy deleted the droudy-patch-1 branch June 27, 2016 15:42
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