Preserve context in LogDensityFunction
#1943
Conversation
Pull Request Test Coverage Report for Build 4740709796
💛 - Coveralls |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #1943 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 21 21
Lines 1422 1422
======================================
Misses 1422 1422
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
devmotion
left a comment
devmotion
left a comment
There was a problem hiding this comment.
Looks good to me, I would just suggest adding a few additional tests to cover all changes. Feel free to merge afterwards.
| # Get the initial log pdf and gradient functions. | ||
| ∂logπ∂θ = gen_∂logπ∂θ(vi, spl, model) | ||
| logπ = Turing.LogDensityFunction(vi, model, spl, DynamicPPL.DefaultContext()) | ||
| logπ = Turing.LogDensityFunction( |
There was a problem hiding this comment.
Seems this is not tested (codecov complains)?
There was a problem hiding this comment.
Tests have been added 👍
| # Make a new transition. | ||
| densitymodel = AMH.DensityModel( | ||
| Base.Fix1(LogDensityProblems.logdensity, Turing.LogDensityFunction(vi, model, DynamicPPL.SamplingContext(rng, spl))) | ||
| Base.Fix1( |
There was a problem hiding this comment.
It seems MH sampler should be tested as well?
There was a problem hiding this comment.
Done! Though it seems to really struggle with here, even with the prior (despite sampling from the prior; am really confused about this..)
…gdensityfunction' into torfjelde/preserve-context-in-logdensityfunction
delete-merged-branch
bot
deleted the
torfjelde/preserve-context-in-logdensityfunction
branch
After TuringLang/DynamicPPL.jl#452 we can properly preserve the context related to the model.
For example, at the moment something like
won't work because
LogDensityFunctionis constructed withDefaultContext, which then replaces the leaf-context inmodelwhen we callevaluate!!.After this PR, stuff like the above works as intended.