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

LDA eta parameter auto-learning #479

Merged
merged 5 commits into from
Nov 3, 2015
Merged

LDA eta parameter auto-learning #479

merged 5 commits into from
Nov 3, 2015

Conversation

cscorley
Copy link
Contributor

Adds auto-learning for the eta parameter on the LdaModel, a feature mentioned in Hoffman's Online LDA paper.

This PR is for a commit I cherry-picked from a fork by @joshua2ua. I've added some tests to ensure it works (=values actually change).

@tmylk
Copy link
Contributor

tmylk commented Oct 11, 2015

Cool, tests do give something asymmetric.
To avoid code duplication, could update_alpha and update_eta call the same dimension-agnostic update_dir_prior function?

@cscorley
Copy link
Contributor Author

@tmylk Not a bad idea, I've updated the PR with that.

@cscorley
Copy link
Contributor Author

@tmylk While we're here, would it be a bad idea to use the same setup procedures for eta that we do in alpha, i.e., enforce a non-scalar value during init by creating the vector if one is given?

@tmylk
Copy link
Contributor

tmylk commented Oct 11, 2015

Yes, that would make code much easier.
On 11 Oct 2015 16:43, "Christopher Corley" notifications@github.com wrote:

@tmylk https://github.com/tmylk While were here, would it be a bad idea
to use the same setup procedures for eta that we do in alpha, i.e., enforce
a non-scalar value during init?


Reply to this email directly or view it on GitHub
#479 (comment).

@cscorley
Copy link
Contributor Author

@tmylk, sorry for the delay! Got around to fixing this up. Added a bunch of tests for the init behaviors of both alpha and eta.

@tmylk
Copy link
Contributor

tmylk commented Oct 19, 2015

Looks pretty neat.

On a related note. Do you think that this should go on the wishlist: "support update_alpha in MultiCore mode"?
Requires changes to LdaModel to share alpha's numpy array between E-step worker threads. Multiprocessing does locks and shared memory.

@cscorley
Copy link
Contributor Author

I would defer that to @ziky90.

@tmylk
Copy link
Contributor

tmylk commented Nov 3, 2015

Regression test passed. (i.e. the tests that apply to both pre and post-PR functionality pass in both versions.)

tmylk added a commit that referenced this pull request Nov 3, 2015
LDA eta parameter auto-learning
@tmylk tmylk merged commit f4919ac into develop Nov 3, 2015
@piskvorky piskvorky deleted the eta_auto branch November 4, 2015 02:23
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.

2 participants