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

Ldaresults #46

Merged
merged 7 commits into from Jun 21, 2015
Merged

Ldaresults #46

merged 7 commits into from Jun 21, 2015

Conversation

dkrasner
Copy link
Contributor

Some cleanup:

  • removed redundant probability data frame in LDAResults
    * cleaned up and made more uniform
  • removed catdoc from list of unix file converter utils since it's no longer supported and fails on OSX; replaced it with antiword utility
  • test cleanup to reflect above

# tokenized_text = ['newtoken', 'newtoken']
# with self.assertRaises(TokenError) as cm:
# results = lda.predict(tokenized_text, raise_on_unknown=True)
def test_predict_6(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point these tests should be renamed to something meaningful...or doc added. In this case, I don't know why something was supposed to be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's I commented them out earlier; they were not really informative or testing much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@langmore
Copy link
Contributor

I had a few comments. Use what you like. LGTM.

@dkrasner
Copy link
Contributor Author

@langmore ok let me know if this looks ok to merge

@langmore
Copy link
Contributor

LGTM (= "Looks good to me", so go ahead and merge after making/not making the suggested changes).

@dkrasner
Copy link
Contributor Author

merging

@langmore - maybe we are due for a micro/small release?

dkrasner pushed a commit that referenced this pull request Jun 21, 2015
@dkrasner dkrasner merged commit f850274 into master Jun 21, 2015
@dkrasner dkrasner deleted the ldaresults branch June 21, 2015 11:26
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.

None yet

2 participants