-
Notifications
You must be signed in to change notification settings - Fork 37
Implement DynamicPPL.rand_with_logpdf on LogDensityFunction
#1178
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
Open
penelopeysm
wants to merge
1
commit into
main
Choose a base branch
from
py/ldfinit
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DynamicPPL.rand_with_logpdfDynamicPPL.rand_with_logpdf on LogDensityFunction
Contributor
Benchmark Report
Computer InformationBenchmark Results |
2cf2f3f to
a35d0e2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1178 +/- ##
==========================================
+ Coverage 78.95% 78.97% +0.02%
==========================================
Files 41 41
Lines 3896 3938 +42
==========================================
+ Hits 3076 3110 +34
- Misses 820 828 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a35d0e2 to
7d4fe3c
Compare
Contributor
|
DynamicPPL.jl documentation for PR #1178 is available at: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, if you were performing MCMC sampling with a LogDensityFunction
ldfand you wanted to figure out a set of reasonable initial parameters you could do:You could then check, for example, whether logp was finite, etc. etc -- see e.g. SliceSampling.jl or Turing's HMC implementation.
Now that LogDensityFunction no longer stores a varinfo, this becomes impossible. Turing can currently handle it because it is responsible for generating the VarInfo and creating the LDF. However, external samplers cannot, because the external sampler is just given the LDF and that's it -- there's no way for it to regenerate the VarInfo.
This unfortunately blocks a lot of things. For example, in MH if you want to generate a new proposal from the prior, you can't do it with just a LogDensityFunction. That's partly why in Turing we are still stuck with carrying an old model + varinfo wrapper struct around. See https://github.com/TuringLang/Turing.jl/blob/4dc7ad096f92fd571de312e7751986484ac6cb50/src/mcmc/mh.jl#L181-L199
There is a cheap way to fix it, which is to just lump a varinfo into LogDensityFunction. I think that is a band-aid, and I don't like that. It also makes my life harder later on, as I REALLY want us to minimise usage of any varinfo that is not OnlyAccsVarInfo.
So, this PR introduces a new function that will do that correctly for LogDensityFunction using a custom accumulator. It's kind of like ValuesAsInModel, but it is a bit more 'batteries included' since it also calculates logjac.
While this does get things across the line in a way I'm generally happy with (see the docstring for a very nice invariant, which is also checked in the test suite), it is my belief that this
rand_with_logpdffunction will need to eventually live inside AbstractMCMC. I think it should be a method on AbstractMCMC.LogDensityModel, which would delegate to the innermodel.logdensityobject.The problem with that is, unfortuntaely, the strategy argument is currently very much DynamicPPL exclusive. So the
InitFrom...strategies also need to be upstreamed, which I feel less happy about doing. So I think this PR is a good middle ground for now.