Update for newer AbstractMCMC/Turing interface #49
Conversation
penelopeysm
force-pushed
the
py/turing
branch
from
17934e1 to
03943e3
Compare
penelopeysm
force-pushed
the
py/turing
branch
from
03943e3 to
18fb8ec
Compare
| # Required for using the slice samplers as `externalsampler`s in Turing | ||
| # begin | ||
| function Turing.Inference.getparams( | ||
| ::Turing.DynamicPPL.Model, sample::SliceSampling.Transition | ||
| ) | ||
| return sample.params | ||
| end | ||
| # end |
There was a problem hiding this comment.
The changes to the external sampler interface are described in https://github.com/TuringLang/Turing.jl/releases/tag/v0.42.0 -- the general aim is that you should not need to overload Turing internal functions (getparams was actually not exported afaik) and shifting this to AbstractMCMC means that it's easier for other packages to make use of this info.
Turing.Inference.getparams is gone now, it's replaced with AbstractMCMC.getparams (but called on the state instead of the transition).
| Turing.Inference.isgibbscomponent(::SliceSampling.RandPermGibbs) = true | ||
| Turing.Inference.isgibbscomponent(::SliceSampling.HitAndRun) = true | ||
| Turing.Inference.isgibbscomponent(::SliceSampling.Slice) = true | ||
| Turing.Inference.isgibbscomponent(::SliceSampling.SliceSteppingOut) = true | ||
| Turing.Inference.isgibbscomponent(::SliceSampling.SliceDoublingOut) = true |
There was a problem hiding this comment.
isgibbscomponent now defaults to true so it no longer needs to be overridden
| abstract type AbstractStateWithTransition end | ||
| AbstractMCMC.getparams(state::AbstractStateWithTransition) = state.transition.params | ||
| AbstractMCMC.getstats(state::AbstractStateWithTransition) = state.transition.info | ||
| function AbstractMCMC.setparams!!( | ||
| model::AbstractMCMC.LogDensityModel, state::AbstractStateWithTransition, params | ||
| ) | ||
| new_lp = LogDensityProblems.logdensity(model.logdensity, params) | ||
| new_transition = Transition(params, new_lp, NamedTuple()) | ||
| return Accessors.@set state.transition = new_transition | ||
| end |
There was a problem hiding this comment.
Since the definitions of these functions are the same for all states in this package, I thought it would be cleaner to just define the behaviour on an abstract type. It does necessitate an extra dep on Accessors, but that's fairly lightweight.
|
The failing tests are to do with SliceSampling.jl/ext/SliceSamplingTuringExt.jl Lines 38 to 41 in 2dd6716 The fact is that when SliceSampling is used via Turing's externalsampler mechanism, that code path will never be hit, because externalsampler makes sure to provide a vector of initial params: So this code path will only be hit if somebody actually manually constructs a LogDensityFunction and passes it to SliceSampling without specifying initial params. One option is therefore to delete the function and the tests. Even though this seems like a bit of a niche use case, I also don't see any justifiable reason to not support it, and deleting it seems overkill. If we don't go down that route, we need a patch to make it work again, since DynamicPPL 0.39 removed some functionality that was needed for this. That patch is here: TuringLang/DynamicPPL.jl#1178. Thus, we need to wait for that to be merged, and I can revisit this after that. Once that is done, I think that the Turing ext can be changed into a DynamicPPL ext, which will make life easier for everyone (more accurate compat bounds, no need to update when there are unrelated changes in Turing, etc.) |
I think it is fine to remove the custom initialization. After all, the purpose of that
Yes, sounds good to me. Let me handle that once this PR is merged. |
Comments in PR!