-
Notifications
You must be signed in to change notification settings - Fork 19
Description
This issue is intended to keep track of what (I think) needs to be done for a new major version of AbstractMCMC. Please feel free to comment as necessary :)
Remove Transitions
Described further in #149.
I contemplated keeping a skeletal Transition
type which contains e.g. a namedtuple of params + a logp, however, I don't think AbstractMCMC is the right place to do that. If downstream packages want to have such functionality then the onus is on them to implement it.
However, imo, AbstractMCMC should provide an interface on an AbstractState
type that will allow for downstream users to extract the necessary information.
Q: Should AbstractState
be a type parameter of AbstractSampler
?
- the problem with this is that the sampler state may also depend on the model. e.g. for DynamicPPL models we may want to use some bundle of VarInfo + sampler-specific state. One workaround for this could be to give
SubSampler
its own state, and thenDynamicPPL.Sampler{::SubSampler}
can have that tuple of varinfo + sampler-specific state. Potentially ugly ugly - Update: I actually think it makes more sense for the model to be a type parameter of the state
AbstractState{<:AbstractModel}
, that way one can define specific behaviour for a specific model
Q: For params we now have getparams
and setparams!!
; note that we don't have the equivalent for logp. should we have an interface where:
getlogp(model::AbstractModel, params::AbstractVector{<:Real})
should evaluate the model at the given set of params and return the value of logp. Of course, we can't provide a concrete implementation here.getlogp([model::AbstractModel, ]state)
should have a default implementation ofgetlogp(model, getparams(state))
, but users can override this for performance if e.g. the logp value is stored in the state.
and is it possible to have another type for params
that carries more information?
We don't need setlogp
because logp should be uniquely determined by (model, params)
.
Names bikesheddable.
Q: Should we also have an interface function for obtaining the first derivative of logp?
Move default_chain_type(::AbstractSampler)
from DynamicPPL to here
I think it makes much more sense for that to be here, and then it can be overloaded later.
This would also allow us to pass default_chain_type(spl)
as the default value to the chain_type
parameter of sample
, bundle_samples
, and mcmcsample
Define a clear interface for the various abstract types
Including tests.
Docs
Once the dust has settled from the above it will be a good time to write new docs imo