-
Notifications
You must be signed in to change notification settings - Fork 18
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
Basic rewrite of the package 2023 edition Part I: ADVI #49
Basic rewrite of the package 2023 edition Part I: ADVI #49
Conversation
This is to avoid having to reconstruct transformed distributions all the time. The direct use of bijectors also avoids going through lots of abstraction layers that could break. Instead, transformed distributions could be constructed only once when returing the VI result.
…into rewriting_advancedvi
This reverts commit 2a4514e.
- Full Monte Carlo ELBO estimation now works. I checked.
@torfjelde @Red-Portal This PR already looks good; let's try to get this PR merged in the next two weeks. |
@torfjelde @yebai As discussed, I've removed the parts directly interacting with Bijectors. I think it's ready for another review pass. (Hopeful that we're close to being done for this PR!) |
@torfjelde a reminder on this. |
Having a proper look now:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few more comments, most of which should be quick accepts.
There's one one argument ordering of reparam_with_entropy
that is potentially "discussable", but it should be a simple change if you accept it.
So. I think the time as come 👀
I am ready accept the PR 🙏
But this is really great work @Red-Portal 👏 And thank you so much for your persistence and just continuously chipping away ❤️ It must have been a pain, but I do think the PR is a much better state now than when we started, so it's been worth it:)
using ..ReverseDiff | ||
end | ||
|
||
# ReverseDiff without compiled tape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not handle compiled tape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it.
Co-authored-by: Tor Erlend Fjelde <[email protected]>
…/AdvancedVI.jl into rewriting_advancedvi_optimize
Hi, this is a partial pull request for #45.
The content of Part I is as follows:
Roadmap
LogDensityProblems
interface.ADTypes
interface.Functor.jl
for flattening/unflattening variational parameters.optimize
. (see Missing API method #32 )Optimisers.jl
.callback
option (Callback function during training #5)Notes
LocationScale
variational family (Part II) and the documentation (Part III).DistributionsAD.TuringDiagMvNormal
variational family for running the tests.