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

modify comment in LdaModel.inference() to be more explicit #3057

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

yocen
Copy link
Contributor

@yocen yocen commented Mar 1, 2021

One comment in LdaModel.inference() confused me:
# The optimal phi_{dwk} is proportional to expElogthetad_k * expElogbetad_w.
And I think it should modified to this so the comment would be more accurate:
# The optimal phi_{dwk} is proportional to expElogthetad_k * expElogbetad_kw.

Reasons:
1.
According to Algorithm 1 on paper:
Online Learning for Latent Dirichlet Allocation, NIPS 2010 http://www.cs.princeton.edu/~mdhoffma,
phi_dwk is proportional to exp(Elogtheta_dk + Elogbeta_kw).
2.
phi_{dwk} is a scalar, expElogthetad_k is a scalar, and expElogbetad_w is a vector with K elements, so expElogthetad_k * expElogbetad_w will return a vector with K elements, which does not match the type of phi_{dwk}. If we use expElogthetad_k * expElogbetad_kw instead, in which expElogbetad_kw is a scalar, it matches.

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

Thank you for pointing this out to us!

@mpenkov mpenkov merged commit 9f2a4ac into piskvorky:develop Apr 13, 2021
@jonaschn
Copy link
Contributor

@yocen Thanks for pointing that out.
@olavurmortensen implemented an efficient version of computing the phinorm for the Author-Topic Model.
I am not sure if the phinorm is the same for LDA and the author-topic model.
If this is the case it would make sense to "backport" @olavurmortensen idea to the LDAModel.

https://github.com/RaRe-Technologies/gensim/blob/2feef89a24c222e4e0fc6e32ac7c6added752c26/gensim/models/atmodel.py#L377

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