Reverse order of prefixing & add changelog (#792)

* Reverse order of prefixing

* Simplify generated function (the non-generated path wasn't being used)

* Expand on submodel behaviour in changelog

Author: penelopeysm

HISTORY.md

@@ -0,0 +1,93 @@
+# DynamicPPL Changelog
+
+## 0.35.0
+
+**Breaking**
+
+- For submodels constructed using `to_submodel`, the order in which nested prefixes are applied has been changed.
+  Previously, the order was that outer prefixes were applied first, then inner ones.
+  This version reverses that.
+  To illustrate:
+
+  ```julia
+  using DynamicPPL, Distributions
+
+  @model function subsubmodel()
+    x ~ Normal()
+  end
+  
+  @model function submodel()
+    x ~ to_submodel(prefix(subsubmodel(), :c), false)
+    return x
+  end
+  
+  @model function parentmodel()
+    x1 ~ to_submodel(prefix(submodel(), :a), false)
+    x2 ~ to_submodel(prefix(submodel(), :b), false)
+  end
+  
+  keys(VarInfo(parentmodel()))
+  ```
+
+  Previously, the final line would return the variable names `c.a.x` and `c.b.x`.
+  With this version, it will return `a.c.x` and `b.c.x`, which is more intuitive.
+  (Note that this change brings `to_submodel`'s behaviour in line with the now-deprecated `@submodel` macro.)
+
+  This change also affects sampling in Turing.jl.
+
+
+## 0.34.2
+
+- Fixed bugs in ValuesAsInModelContext as well as DebugContext where underlying PrefixContexts were not being applied.
+  From a user-facing perspective, this means that for models which use manually prefixed submodels, e.g.
+
+  ```julia
+  using DynamicPPL, Distributions
+  
+  @model inner() = x ~ Normal()
+  
+  @model function outer()
+    x1 ~ to_submodel(prefix(inner(), :a), false)
+    x2 ~ to_submodel(prefix(inner(), :b), false)
+  end
+  ```
+
+  will: (1) no longer error when sampling due to `check_model_and_trace`; and (2) contain both submodel's variables in the resulting chain (the behaviour before this patch was that the second `x` would override the first `x`).
+
+- More broadly, implemented a general `prefix(ctx::AbstractContext, ::VarName)` which traverses the context tree in `ctx` to apply all necessary prefixes. This was a necessary step in fixing the above issues, but it also means that `prefix` is now capable of handling context trees with e.g. multiple prefixes at different levels of nesting.
+
+## 0.34.1
+
+- Fix an issue that prevented merging two VarInfos if they had different dimensions for a variable.
+
+- Upper bound the compat version of KernelAbstractions to work around an issue in determining the right VarInfo type to use.
+
+## 0.34.0
+
+**Breaking**
+
+- `rng` argument removed from `values_as_in_model`, and `varinfo` made non-optional. This means that the only signatures allowed are
+
+  ```
+  values_as_in_model(::Model, ::Bool, ::AbstractVarInfo)
+  values_as_in_model(::Model, ::Bool, ::AbstractVarInfo, ::AbstractContext)
+  ```
+
+  If you aren't using this function (it's probably only used in Turing.jl) then this won't affect you.
+
+## 0.33.1
+
+Reworked internals of `condition` and `decondition`.
+There are no changes to the public-facing API, but internally you can no longer use `condition` and `decondition` on an `AbstractContext`, you can only use it on a `DynamicPPL.Model`. If you want to modify a context, use `ConditionContext` and `decondition_context`.
+
+## 0.33.0
+
+**Breaking**
+
+- `values_as_in_model()` now requires an extra boolean parameter, specifying whether variables on the lhs of `:=` statements are to be included in the resulting `OrderedDict` of values.
+  The type signature is now `values_as_in_model([rng,] model, include_colon_eq::Bool [, varinfo, context])`
+
+**Other**
+
+- Moved the implementation of `predict` from Turing.jl to DynamicPPL.jl; the user-facing behaviour is otherwise the same
+- Improved error message when a user tries to initialise a model with parameters that don't correspond strictly to the underlying VarInfo used

src/contexts.jl

@@ -253,31 +253,26 @@ function PrefixContext{Prefix}(context::AbstractContext) where {Prefix}
     return PrefixContext{Prefix,typeof(context)}(context)
 end
 
-NodeTrait(context::PrefixContext) = IsParent()
+NodeTrait(::PrefixContext) = IsParent()
 childcontext(context::PrefixContext) = context.context
-function setchildcontext(parent::PrefixContext{Prefix}, child) where {Prefix}
+function setchildcontext(::PrefixContext{Prefix}, child) where {Prefix}
     return PrefixContext{Prefix}(child)
 end
 
 const PREFIX_SEPARATOR = Symbol(".")
 
-# TODO(penelopeysm): Prefixing arguably occurs the wrong way round here
-function PrefixContext{PrefixInner}(
-    context::PrefixContext{PrefixOuter}
-) where {PrefixInner,PrefixOuter}
-    if @generated
-        :(PrefixContext{$(QuoteNode(Symbol(PrefixOuter, PREFIX_SEPARATOR, PrefixInner)))}(
-            context.context
-        ))
-    else
-        PrefixContext{Symbol(PrefixOuter, PREFIX_SEPARATOR, PrefixInner)}(context.context)
-    end
+@generated function PrefixContext{PrefixOuter}(
+    context::PrefixContext{PrefixInner}
+) where {PrefixOuter,PrefixInner}
+    return :(PrefixContext{$(QuoteNode(Symbol(PrefixOuter, PREFIX_SEPARATOR, PrefixInner)))}(
+        context.context
+    ))
 end
 
-# TODO(penelopeysm): Prefixing arguably occurs the wrong way round here
 function prefix(ctx::PrefixContext{Prefix}, vn::VarName{Sym}) where {Prefix,Sym}
-    return prefix(
-        childcontext(ctx), VarName{Symbol(Prefix, PREFIX_SEPARATOR, Sym)}(getoptic(vn))
+    vn_prefixed_inner = prefix(childcontext(ctx), vn)
+    return VarName{Symbol(Prefix, PREFIX_SEPARATOR, getsym(vn_prefixed_inner))}(
+        getoptic(vn_prefixed_inner)
     )
 end
 prefix(ctx::AbstractContext, vn::VarName) = prefix(NodeTrait(ctx), ctx, vn)

test/contexts.jl

@@ -142,11 +142,11 @@ end
 
     @testset "PrefixContext" begin
         @testset "prefixing" begin
-            ctx = @inferred PrefixContext{:f}(
-                PrefixContext{:e}(
-                    PrefixContext{:d}(
-                        PrefixContext{:c}(
-                            PrefixContext{:b}(PrefixContext{:a}(DefaultContext()))
+            ctx = @inferred PrefixContext{:a}(
+                PrefixContext{:b}(
+                    PrefixContext{:c}(
+                        PrefixContext{:d}(
+                            PrefixContext{:e}(PrefixContext{:f}(DefaultContext()))
                         ),
                     ),
                 ),
@@ -174,8 +174,8 @@ end
             vn_prefixed4 = prefix(ctx4, vn)
             @test DynamicPPL.getsym(vn_prefixed1) == Symbol("a.x")
             @test DynamicPPL.getsym(vn_prefixed2) == Symbol("a.x")
-            @test DynamicPPL.getsym(vn_prefixed3) == Symbol("a.b.x")
-            @test DynamicPPL.getsym(vn_prefixed4) == Symbol("a.b.x")
+            @test DynamicPPL.getsym(vn_prefixed3) == Symbol("b.a.x")
+            @test DynamicPPL.getsym(vn_prefixed4) == Symbol("b.a.x")
             @test DynamicPPL.getoptic(vn_prefixed1) === DynamicPPL.getoptic(vn)
             @test DynamicPPL.getoptic(vn_prefixed2) === DynamicPPL.getoptic(vn)
             @test DynamicPPL.getoptic(vn_prefixed3) === DynamicPPL.getoptic(vn)