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

DTM wrapper bug fixes. #770

Merged
merged 3 commits into from
Jul 4, 2016
Merged

DTM wrapper bug fixes. #770

merged 3 commits into from
Jul 4, 2016

Conversation

bhargavvader
Copy link
Contributor

@bhargavvader bhargavvader commented Jul 2, 2016

Changed topn for num_words, wrt #769 .

While the same problem as in LdaMallet wrapper (PR #767 ) for it not being backward compatible is there, the PR #755 already made the non-backward compatible changes so I don't think this should be a problem. (@piskvorky , please have a look at this)

At any rate, either only topn or num_words should be there (not half and half), the print_topics function will not work otherwise.

I've also removed six.u with respect to #768 as well.

Edit: Made changes to the DTM notebook to reflect this as well.

@bhargavvader
Copy link
Contributor Author

@piskvorky , @tmylk , could you review?

@tmylk tmylk merged commit 012877a into piskvorky:develop Jul 4, 2016
@piskvorky
Copy link
Owner

I like topn (or perhaps top_n) a little better, because it's not dependent on what the features represent (words, phrases, entities, characters...).

But it's not a big deal either way. We should just be consistent throughout, whichever terminology we use.

@bhargavvader bhargavvader deleted the DTM_bugfix branch July 19, 2016 09:35
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