Refactor model for the AbstractPPL way #244

phipsgabler · 2021-05-17T15:45:43Z

Now, here I have begun refactorings that go in hand with introducing the AbstractPPL abstractions. For now it is mostly changing what goes in Model, and how the compiler macro constructs it.

This is not in a usable state right now; but it's less boilerplate, unless I forgot something important in the stuff I removed.

Since the structural part AbstractPPL is (in the currently proposed idea) based on the condition and decondition operations, I decided to try a different approach than the current combination of "arguments" and "missings". Construction of this stuff should, I think, work; it remains to adapt the model macro and make evaluation work.

Couple of questions:

Putting parameters and observations in the model macro requires distinguishing them somehow. As a first option, the easiest way to do that would be Soss-style: make @model always construct a GenerativeModel, and use condition later. Which is totally breaking what people expect from the previous interface, though.

Second alternative would be to mark either parameters or observations syntactically. I kind of like this, but have heard voices against it; also, it changes from one non-standard evaluation semantics to another.

Third option, most advanced but perhaps intuitive one, would be to do this via the same kind of process that TypedVarInfo uses, by running one evaluation.
The parameters/observations split also requires them to be separated in the evaluator function. Is it a problem to just pass two named tuples instead of the original argument form? Or was that designed for some reasons I don't know?

Also, how does this interact with the ::Type{T} = Float64 hack?

src/compiler.jl

devmotion · 2021-05-17T15:55:30Z

src/model.jl

+    struct Model{F, parameternames, observationnames, Tparams, Tobs} <: AbstractProbabilisticProgram
        name::Symbol
-        f::F
-        args::NamedTuple{argnames,Targs}
-        defaults::NamedTuple{defaultnames,Tdefaults}
+        evaluator::F
+        parameters::NamedTuple{parameternames,Tparams}
+        observations::NamedTuple{observationnames,Tobs}
    end


These are breaking changes. Is it possible and worth the effort to introduce the new supertype (which is not breaking itself) without changing the type parameters and fields of Model?

I'm also a bit skeptical about encoding this in Model. Would it maybe be better to just have a ContextualModel where we couple a AbstractContext to a Model instance, e.g. we make a ConditionContext, and condition(model, ...) just returns the a ContextualModel{<:ConditionContext, <:Model} or w/e. I'm guessing the motivation behind stuff like condition being "structural" (in the sense that it "changes" the model itself rather than just wrapping it) is described somewhere else, so I'll raise this more "general" comment there.

But for this PR I agree with the above: an abstract Model so that we can experiment without making breaking changes is a good idea:)

EDIT: The motivation behind the above is that observations should never have to be specified at construction of the model IMO. Arguments of the model can be observations or just general "hyperparameters" of a model. condition should then specify what's observed and what's not.

The idea was that the @model macro builds a model constructor function that will then automatically construct the conditioned model, based on the same principles that we have now with inargnames and missings. And it should do this in a way that behaves like condition(generative_model, observations), where the generative model is what the evaluator function basically expresses.

Now, I have thought about separating those two cases, so that the evaluator function goes into a GenerativeModel, and the model constructor then applies condition, but I realized that one type should suffice, since a generative model is just a model conditioned on an empty set of observations.

devmotion · 2021-05-17T15:56:25Z

src/model.jl

-
-julia> Model{(:y,)}(f, (x = 1.0, y = 2.0), (x = 42,)) # with special definition of missings
-Model{typeof(f),(:x, :y),(:x,),(:y,),Tuple{Float64,Float64},Tuple{Int64}}(f, (x = 1.0, y = 2.0), (x = 42,))
+TODO


Is there a reason to shorten the docstring and remove the example?

Yeah, it will look differently and I haven't yet updated it :P

devmotion · 2021-05-17T15:57:23Z

src/model.jl

-    """
-        Model{missings}(name::Symbol, f, args::NamedTuple, defaults::NamedTuple)
-
-    Create a model of name `name` with evaluation function `f` and missing arguments
-    overwritten by `missings`.
-    """


This docstring should not be removed. It was added on purpose due to a bug in Julia (otherwise it won't show up in ? Model IIRC). I can look up the relevant PR.

devmotion · 2021-05-17T15:59:22Z

src/model.jl

+Macro with more convenient syntax for declaring `Model` types with observations (similar to the
+`Base.@NamedTuple` macro).  The observations to the parameters part of the braces:
+`@ConditionedModel{; x::Int, y}`.  Type annotations can be omitted, in which case the type is
+defaulted to `Any`.


Do you have an example where this macro is needed? Currently users never explicitly construct a Model instance.

devmotion · 2021-05-17T16:00:30Z

src/model.jl

+
+Type alias for models without observations.
+"""
+const GenerativeModel{F, parameternames, Tparams} = @ConditionedModel{F, parameternames, Tparams;}


Same here, do you have an example where it is needed?

devmotion · 2021-05-17T16:01:35Z

src/model.jl

-@generated function _evaluate(rng, model::Model{_F,argnames}, varinfo, sampler, context) where {_F,argnames}
-    unwrap_args = [:($matchingvalue(sampler, varinfo, model.args.$var)) for var in argnames]
-    return :(model.f(rng, model, varinfo, sampler, context, $(unwrap_args...)))
+function _evaluate(rng, model::Model, varinfo, sampler, context)


I guess there was a reason for why this was a generated function before. It would be good to benchmark it before making it a regular function.

devmotion · 2021-05-17T16:03:24Z

src/model.jl

 """
-getargnames(model::Model{_F,argnames}) where {argnames,_F} = argnames
-
+getparameternames(model::Model{_F,parameternames}) where {_F,parameternames} = parameternames


Depending on whether we plan a breaking release or not, these should be deprecated (if exported). In any case it might be good to deprecate them and make a non-breaking release such that it is easier to update Turing and other downstream packages (if there are any 😄).

devmotion · 2021-05-17T16:04:31Z

src/model.jl

+
+# See also [`logjoint`](@ref) and [`loglikelihood`](@ref).
+# """
+# function logprior(model::Model, varinfo::AbstractVarInfo)


It would be good to still be able to evaluate log prior and log likelihoods.

devmotion · 2021-05-17T16:06:00Z

src/compiler.jl


    if left isa Symbol || left isa Expr
-        @gensym out vn inds isassumption
+        @gensym out vn inds isobservation


We should benchmark all changes here, similar to @torfjelde's PR a while ago. There were some surprising performance regressions that had to be fixed.

devmotion · 2021-05-17T16:20:04Z

The parameters/observations split also requires them to be separated in the evaluator function. Is it a problem to just pass two named tuples instead of the original argument form? Or was that designed for some reasons I don't know?

Yes, this seems problematic. If one would define a specialized evaluation function that distinguishes parameters and observations already when constructing the Model, I assume this would break the prob and logprob macros and the submodel functionality that rely on the ability to use it to run the model with different combinations of observations and parameters. @model gdemo(; x, y) = ... just generates the regular gdemo function that itself returns a Model with the general evaluation function. Currently we only define one evaluation function in the @model macro that does not distinguish between observations and parameters such that the user can define them dynamically when calling gdemo(; x=missing, y=[3.0, missing, 4.0]) and during model evaluation.

github-actions · 2021-06-14T12:40:43Z

src/compiler.jl

@@ -1,44 +1,12 @@
+const DISTMSG = "Right-hand side of a ~ must be subtype of Distribution or a vector of " *


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

const DISTMSG = "Right-hand side of a ~ must be subtype of Distribution or a vector of " *

const DISTMSG =

"Right-hand side of a ~ must be subtype of Distribution or a vector of " *

github-actions · 2021-06-14T12:40:44Z

src/compiler.jl

-
-# failsafe: a literal is never an assumption
-isassumption(expr) = :(false)



[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2021-06-14T12:40:44Z

src/compiler.jl

    return unwrap_right_left_vns(right, left, vns)
 end

+


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2021-06-14T12:40:44Z

src/compiler.jl

-    # Generate main body
-    modelinfo[:body] = generate_mainbody(mod, modelinfo[:modeldef][:body], warn)
+    # Generate main body and find all variable symbols
+    modelinfo[:body], modelinfo[:varnames] = generate_mainbody(mod, modelinfo[:modeldef][:body], warn)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

modelinfo[:body], modelinfo[:varnames] = generate_mainbody(mod, modelinfo[:modeldef][:body], warn)

modelinfo[:body], modelinfo[:varnames] = generate_mainbody(

mod, modelinfo[:modeldef][:body], warn

)

github-actions · 2021-06-14T12:40:44Z

src/compiler.jl

+    # extract parameters and observations from that
+    modelinfo[:paramnames] = filter(x -> x ∉ modelinfo[:varnames], modelinfo[:allargs_syms])
+    modelinfo[:obsnames] = setdiff(modelinfo[:allargs_syms], modelinfo[:paramnames])
+


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2021-06-14T12:40:46Z

src/model.jl

+@generated function getobservationnames(model::Model{_F,argnames,Targs}) where {_F,argnames,Targs}
+    obs_indices = filter(i -> Targs.parameters[i] <: Observation, eachindex(Targs.parameters))


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

@generated function getobservationnames(model::Model{_F,argnames,Targs}) where {_F,argnames,Targs}

obs_indices = filter(i -> Targs.parameters[i] <: Observation, eachindex(Targs.parameters))

@generated function getobservationnames(

model::Model{_F,argnames,Targs}

) where {_F,argnames,Targs}

obs_indices = filter(

i -> Targs.parameters[i] <: Observation, eachindex(Targs.parameters)

)

github-actions · 2021-06-14T12:40:46Z

src/model.jl

-    logprior(model::Model, varinfo::AbstractVarInfo)
-
-Return the log prior probability of variables `varinfo` for the probabilistic `model`.
+function AbstractPPL.decondition(model::Model, name = Symbol(model.name, "_joint"))


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

function AbstractPPL.decondition(model::Model, name = Symbol(model.name, "_joint"))

function AbstractPPL.decondition(model::Model, name=Symbol(model.name, "_joint"))

github-actions · 2021-06-14T12:40:47Z

src/model.jl

+function AbstractPPL.condition(model::Model, observations, name = Symbol(model.name, "_cond"))
+    return Model(name, model.evaluator, model.parameters, merge(model.observations, observations))


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

function AbstractPPL.condition(model::Model, observations, name = Symbol(model.name, "_cond"))

return Model(name, model.evaluator, model.parameters, merge(model.observations, observations))

function AbstractPPL.condition(model::Model, observations, name=Symbol(model.name, "_cond"))

return Model(

name, model.evaluator, model.parameters, merge(model.observations, observations)

)

github-actions · 2021-06-14T12:40:47Z

src/model.jl


-"""
-    loglikelihood(model::Model, varinfo::AbstractVarInfo)



[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2021-06-14T12:40:47Z

src/model.jl

-    model(varinfo, LikelihoodContext())
-    return getlogp(varinfo)
-end



[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2021-06-21T10:53:29Z

src/model.jl

+#     return :(Modelngs}(name, f, args, defaults))
+# end
+
+


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2021-06-21T10:53:29Z

src/model.jl

+    vn::VarName{s},
+    model::Model{_F, argnames}
+) where {s, _F, argnames}


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

vn::VarName{s},

model::Model{_F, argnames}

) where {s, _F, argnames}

vn::VarName{s}, model::Model{_F,argnames}

) where {s,_F,argnames}

github-actions · 2021-06-21T10:53:30Z

src/model.jl

+isobservation(vn::VarName, obs::Observation) = !ismissing(_getindex(obs, vn.indexing))
+isobservation(vn::VarName, obs::Observation{Missing}) = false
+
+


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

phipsgabler · 2021-06-24T16:10:20Z

Closed in favour of #268 for now, which will allow for a nicer way of implementing condition/decondition. (The rest is mostly renaming.)

Start rebuilding Model

df45a10

devmotion reviewed May 17, 2021

View reviewed changes

phipsgabler added 3 commits May 22, 2021 17:46

Some minor adaptions

9da4c5b

Update some new syntax

ac9ed3e

Merge master and try out stuff

8f62596

github-actions bot reviewed Jun 14, 2021

View reviewed changes

More model refactorings

74ad6e2

github-actions bot reviewed Jun 21, 2021

View reviewed changes

phipsgabler closed this Jun 24, 2021

yebai deleted the phg/abstractppl_interface branch January 31, 2022 20:01

		@@ -1,44 +1,12 @@
		const DISTMSG = "Right-hand side of a ~ must be subtype of Distribution or a vector of " *

	const DISTMSG = "Right-hand side of a ~ must be subtype of Distribution or a vector of " *
	const DISTMSG =
	"Right-hand side of a ~ must be subtype of Distribution or a vector of " *


		# failsafe: a literal is never an assumption
		isassumption(expr) = :(false)

		@generated function getobservationnames(model::Model{_F,argnames,Targs}) where {_F,argnames,Targs}
		obs_indices = filter(i -> Targs.parameters[i] <: Observation, eachindex(Targs.parameters))

	function AbstractPPL.decondition(model::Model, name = Symbol(model.name, "_joint"))
	function AbstractPPL.decondition(model::Model, name=Symbol(model.name, "_joint"))

		function AbstractPPL.condition(model::Model, observations, name = Symbol(model.name, "_cond"))
		return Model(name, model.evaluator, model.parameters, merge(model.observations, observations))

		isobservation(vn::VarName, obs::Observation) = !ismissing(_getindex(obs, vn.indexing))
		isobservation(vn::VarName, obs::Observation{Missing}) = false

Refactor model for the AbstractPPL way #244

Refactor model for the AbstractPPL way #244

Uh oh!

Conversation

phipsgabler commented May 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phipsgabler May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmotion commented May 17, 2021

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

phipsgabler commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

torfjelde May 17, 2021 •

edited

Loading

phipsgabler May 22, 2021 •

edited

Loading