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

LdaMallet.show_topics(formatted=False) fails #1150

Closed
frederik-elwert opened this issue Feb 17, 2017 · 4 comments
Closed

LdaMallet.show_topics(formatted=False) fails #1150

frederik-elwert opened this issue Feb 17, 2017 · 4 comments

Comments

@frederik-elwert
Copy link

Seems to be another side effect of the topn/num_topics issue:

In gensim/models/wrappers/ldamallet.py

            if formatted:
                topic = self.print_topic(i, topn=num_words)
            else:
                topic = self.show_topic(i, topn=num_words)

should be

            if formatted:
                topic = self.print_topic(i, topn=num_words)
            else:
                topic = self.show_topic(i, num_words=num_words)
@piskvorky
Copy link
Owner

@tmylk why the param name inconsistency?

@markroxor
Copy link
Contributor

@tmylk
Copy link
Contributor

tmylk commented Mar 8, 2017

Closing issue as fixed in #1066.

@piskvorky There is an inconsistency between show_topic-topn and show_topics-num_words in LdaModel which is replicated along other topic models in order to standardise the api.

show_topics and print_topics were standardised among topic models in #755 to match LdaModel to use num_words as the argument names were confusing.
Later #771 changed show_topic and print_topic to use num_words just in ldamallet. That was a way to standardise it with show_topics, but in retrospect it was not the best move.
Then print_topic was changed back to use topn like all the other models do when ldamallet inherited from BaseTopicModel in #995. Though show_topic has remained - that is the inconsistency you see above.

Raised an issue #1198

@tmylk tmylk closed this as completed Mar 8, 2017
@piskvorky
Copy link
Owner

Alright, makes sense, thanks. Let's make it consistent, definitely.

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

No branches or pull requests

4 participants