Skip to content

Does model.f really need to take a context?Β #951

@penelopeysm

Description

@penelopeysm

Here,

DynamicPPL.jl/src/model.jl

Lines 942 to 944 in 3e54c2d

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:

  1. Remove model.context field. This is more awkward because things like condition and fix rely on modifying model.context
  2. 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.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions