Here,
|
context_new = setleafcontext( |
|
context, setleafcontext(model.context, leafcontext(context)) |
|
) |
evaluate!!(model, varinfo, context) takes model.context and context, smushes them together to make a new_context, and then runs model.f(model, varinfo, new_context, ...).
In principle, there are two contexts here, because model.context still exists, and there's also new_context. However, I don't think that model.context is ever accessed again, essentially because its information content has been lumped into new_context.
It appears to me that we should only retain one context, i.e., don't pass both model.context and new_context to model.f. There are a couple of ways to do this:
- Remove
model.context field. This is more awkward because things like condition and fix rely on modifying model.context
- Or, remove the
context argument to model.f. Then in evaluate!!, after constructing new_context, we set that as the new context of the model. This seems to have more potential but I'm mildly concerned about it messing with other places where we manually set the context, e.g. submodels, or Gibbs.
One benefit of removing this duplication is to simplify the method dispatch for evaluate!!: #720
In other words, the 'main' method would be evaluate!!(model, varinfo). We could keep the functionality evaluate!!(model, varinfo, context) with the same semantics but deprecate it.
But I think the bigger benefit is to remove the context argument from LogDensityFunction.
Here,
DynamicPPL.jl/src/model.jl
Lines 942 to 944 in 3e54c2d
evaluate!!(model, varinfo, context)takesmodel.contextandcontext, smushes them together to make anew_context, and then runsmodel.f(model, varinfo, new_context, ...).In principle, there are two contexts here, because
model.contextstill exists, and there's alsonew_context. However, I don't think thatmodel.contextis ever accessed again, essentially because its information content has been lumped intonew_context.It appears to me that we should only retain one context, i.e., don't pass both
model.contextandnew_contexttomodel.f. There are a couple of ways to do this:model.contextfield. This is more awkward because things likeconditionandfixrely on modifyingmodel.contextcontextargument tomodel.f. Then inevaluate!!, after constructingnew_context, we set that as the new context of the model. This seems to have more potential but I'm mildly concerned about it messing with other places where we manually set the context, e.g. submodels, or Gibbs.One benefit of removing this duplication is to simplify the method dispatch for evaluate!!: #720
In other words, the 'main' method would be
evaluate!!(model, varinfo). We could keep the functionalityevaluate!!(model, varinfo, context)with the same semantics but deprecate it.But I think the bigger benefit is to remove the context argument from
LogDensityFunction.