You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
The point was that AbstractMCMC.step(rng, ::Model, ::DynamicPPL.Sampler) would do a few things:
It would create a default_varinfo for the model (which meant typed varinfo, unless you specifically overrode it)
It would then initialise the parameters in the varinfo
Finally, it would call DynamicPPL.initialstep(...) with that new varinfo
Because I'm now changing sample and step to work with LogDensityFunctions rather than Model, this means that the work to create the varinfo has to be done before calling step. See:
Moves the parameter initialisation into default_varinfo, and adds an argument to link the varinfo (which Turing samplers can then make use of, by declaring that they need unconstrained space)
Since default_varinfo is called higher up, it no longer needs to be part of AbstractMCMC.step, and thus initialstep is no longer necessary.
Samplers that implemented DynamicPPL.initialstep(rng, model, spl, vi, ...) should just implement AbstractMCMC.step(rng, ldf, spl) in exactly the same way.
Note that all of these are merely interface changes -- they don't actually affect any code in DynamicPPL as none of the actual sampling is implemented here.
I'm not sure if this should be a breaking change. Technically, none of these functions were exported, so I have erred on the side of danger and marked it as a patch.
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.
Other changes in service of TuringLang/Turing.jl#2555 --- but I think these also make life easier.
The point was that
AbstractMCMC.step(rng, ::Model, ::DynamicPPL.Sampler)
would do a few things:default_varinfo
for the model (which meant typed varinfo, unless you specifically overrode it)DynamicPPL.initialstep(...)
with that new varinfoBecause I'm now changing
sample
andstep
to work withLogDensityFunction
s rather thanModel
, this means that the work to create the varinfo has to be done before callingstep
. See:https://github.com/TuringLang/Turing.jl/blob/9054b0af3f6cd625c1e981b7b96c800ade63ca3f/src/mcmc/sample.jl#L186-L200
So, this PR:
default_varinfo
, and adds an argument to link the varinfo (which Turing samplers can then make use of, by declaring that they need unconstrained space)default_varinfo
is called higher up, it no longer needs to be part ofAbstractMCMC.step
, and thusinitialstep
is no longer necessary.Samplers that implemented
DynamicPPL.initialstep(rng, model, spl, vi, ...)
should just implementAbstractMCMC.step(rng, ldf, spl)
in exactly the same way.Note that all of these are merely interface changes -- they don't actually affect any code in DynamicPPL as none of the actual sampling is implemented here.
I'm not sure if this should be a breaking change. Technically, none of these functions were exported, so I have erred on the side of danger and marked it as a patch.