TS 2: introduce TypedVarInfo and fix spl.info[:cache_updated] #742

mohamed82008 · 2019-03-31T06:36:57Z

This is the second wave of #660. In this PR:

The TypedVarInfo type is introduced with all its utility functions mapping that of UntypedVarInfo.
Unit tests mapping those of UntypedVarInfo are also defined.
One side change that was made in this PR is to make sure to invalidate the sampler's cache every time push! is called on VarInfo. This seemed like a latent bug that was "worked around" by invalidating the cache at the call site after calling push!.
getvals(vi, spl) was removed as it seemed to be doing the same thing as vi[spl] and it was unused in the rest of Turing.

Your feedback is appreciated.

mohamed82008 · 2019-03-31T06:54:11Z

~~TODO: replace most of the generated functions with recursive functions. Example:~~

@generated function _getranges(vi::TypedVarInfo{Tvis}, idcs) where Tvis
    args = []
    for f in fieldnames(Tvis)
        push!(args, :($f = _map(vi, $(QuoteNode(f)), idcs.$f)))
    end
    if length(args) == 0
        nt = :(NamedTuple())
    else
        nt = :(($(args...),))
    end
    return nt
end

can be replaced with:

_getranges(vi::TypedVarInfo, idcs) = _getranges(vi.vis, vi, idcs)
@inline function _getranges(vis::NamedTuple{names}, vi::TypedVarInfo, idcs) where names
	length(names) === 0 && return NamedTuple()
	f = names[1]
	v = _map(vi, f, getfield(idcs, f))
	nt = NamedTuple{(f,), Tuple{typeof(v)}}(v)
	return merge(nt, _getranges(Base.tail(vis), vi, idcs))
end

yebai · 2019-04-03T16:23:46Z

@mohamed82008 is this ready for a review?

mohamed82008 · 2019-04-03T20:24:02Z

Yes if you don't mind the generated functions. The semantics will not change by making them into normal functions.

xukai92

I left some comments. Most of them are things I didn't quite understand and ask for explanations. Also, I didn't check the set and get functions carefully but I'm happy with them given the tests are covered and pass.

xukai92 · 2019-04-06T13:42:57Z

src/inference/is.jl

@@ -58,7 +58,7 @@ end

 function assume(spl::Sampler{<:IS}, dist::Distribution, vn::VarName, vi::VarInfo)
    r = rand(dist)
-    push!(vi, vn, r, dist, spl.selector)
+    push!(vi, vn, r, dist, spl)


It seems that we are storing spl directly instead of selector inside vi now? Why do we do this?

No we are still pushing the same way. The reason why I am passing spl here is to reset the :cache_updated field inside the function.

In you opinion, do you think this cache thing is worth doing here at all. I feel it might be over-optimisation here...

I think it is important in cases where the VarInfo is "filled" most of the time so the cache is valid most of the time. This is the case for most samplers except the particle ones I believe.

src/core/RandomVariables.jl

xukai92 · 2019-04-06T14:24:42Z

test/core/RandomVariables.jl

        dists = [Normal(0, 1), MvNormal([0; 0], [1.0 0; 0 1.0]), Wishart(7, [1 0.5; 0.5 1])]
+        function test_varinfo!(vi)


Would it be nice to improve the tests here a bit, e.g. adding more comments on what's testing and expected, and improving the code (variable names, code reuse, etc).

I will add more comments to the src too.

xukai92 · 2019-04-06T14:25:28Z

test/core/RandomVariables.jl

@@ -1,9 +1,9 @@
 using Turing, Random
 using Turing: Selector, reconstruct, invlink, CACHERESET, SampleFromPrior
 using Turing.RandomVariables
-using Turing.RandomVariables: uid, cuid, getvals, getidcs,
+using Turing.RandomVariables: uid, cuid, getidcs,


Are we aware of the test coverage of the TypedVarInfo?

I made sure the test coverage of TypedVarInfo is no less than that of UntypedVarInfo. But I will see if I can improve it.

src/core/RandomVariables.jl

xukai92 · 2019-04-06T14:36:28Z

src/core/RandomVariables.jl

@@ -118,32 +130,224 @@ mutable struct UntypedVarInfo <: AbstractVarInfo
 end
 VarInfo() = UntypedVarInfo()

+###########################
+# Single variable VarInfo #


Does SingleVarInfo only contains a single random variable? If so why does ranges required for SingleVarInfo and fields like dists, gids are still vectors?

In case we have a vector variable where each element has a different prior distribution, so I think dists still needs to be a vector. I think gids can be shared however. Not sure about ranges, in case we have a matrix variable whose columns are multivariate variables, we probably still need it.

Hmmm, I see your point, though I guess we should discourage users to do that. And I'm not sure whether it makes or not for a single multivariate random variable whose different dimension follows different distributions. But I'm OK with it for now.

xukai92 · 2019-04-06T14:41:15Z

src/core/RandomVariables.jl

+end
+@inline function _link!(vis::NamedTuple{names}, vi, vns, space) where {names}
+    length(names) === 0 && return nothing
+    f = names[1]


A bit confused by the variables here, what is f? Also we are calling getfield(vns, f) multiple times later, can we assign it to some local variable with a name?

f is the first variable's symbol.

But using recursion, this "first" variable will be a different variable in each level of the recursion.

But for the loop below we are calling getfield(vns, f) with same vns and f all the time do we?

Yes, so we are iterating over all the vns of the first symbol, then all the vns of the second, and so on. Each symbol has a vns vector because it can consist of multiple random variables, e.g. multiple univariate variables arranged in vector form or multiple multivariate variables arranged in matrix form, etc. The last line is where recursion happens.

xukai92 · 2019-04-06T14:41:53Z

src/core/RandomVariables.jl

+end
+@inline function _invlink!(vis::NamedTuple{names}, vi, vns, space) where {names}
+    length(names) === 0 && return nothing
+    f = names[1]


Similar here.

mohamed82008 · 2019-04-07T05:09:07Z

I am working on docstrings for all the major functions and types in the module RandomVariables.

mohamed82008 · 2019-04-07T13:45:33Z

To do:

More unit tests for TypedVarInfo and UntypedVarInfo
More comments and docstrings
Make gids for TypedVarInfo a resizeable FillArray type

yebai

Review in progress - I'll review this PR in several steps in the next 2-3 days.

yebai · 2019-04-25T19:39:58Z

src/core/RandomVariables.jl

+
+Examples:
+
+- `x[2] ~ Normal()` will generate a `VarName` with `sym == :x` and `indexing == "[1]"`


indexing == "[1]" ==> indexing == "[2]"?

yebai · 2019-04-25T19:42:25Z

src/core/RandomVariables.jl

+end
+```
+
+A variable identifier. Every variable has a symbol `sym`, indices `indexing`, and internal fields: `csym` and `counter`. The Julia variable in the model corresponding to `sym` can refer to a single value or to a hierarchical array structure of univariate, multivariate or matrix variables. `indexing` stores the indices that can access the random variable from the Julia variable. 


Perhaps breaking this line into smaller lines (i.e. <80 chars)?

Or 92 which is the number from our guide.

yebai · 2019-04-25T19:48:08Z

src/core/RandomVariables.jl

@@ -60,20 +108,57 @@ isequal(x::VarName, y::VarName) = hash(uid(x)) == hash(uid(y))
 Base.string(vn::VarName) = "{$(vn.csym),$(vn.sym)$(vn.indexing)}:$(vn.counter)"
 Base.string(vns::Vector{<:VarName}) = replace(string(map(vn -> string(vn), vns)), "String" => "")

+"""
+`sym_idx(vn::VarName)`


Is this equivalent to the function mentioned in #721?

It's the inverse of it.

yebai · 2019-04-25T20:07:46Z

src/core/RandomVariables.jl


+A light wrapper over one or more instances of `Metadata`. Let `vi` be an instance of `Metadata`. If `vi isa VarInfo{<:Metadata}`, then only `Metadata` instance is used for all the sybmols. `VarInfo{<:Metadata}` is aliased `UntypedVarInfo`. If `vi isa VarInfo{<:NamedTuple}`, then `vi.metadata` is a `NamedTuple` that maps each symbol used on the LHS of `~` in the model to its `Metadata` instance. The latter allows for the type specialization of `vi` after the first sampling iteration when all the symbols have been observed. `VarInfo{<:NamedTuple}` is aliased `TypedVarInfo`.
+
+Note: It is the user's responsibility to ensure that each symbol is visited at least once whenever the model is called, regardless of any stochastic branching.


presumably, it's ok if a variable is visited multiple times since they can have different counter? If so, can we say this explicitly in the comment to reduce potential confusion?

Each random variable can only be visited once in a model call. But the symbol of the Julia variable can be visited more than once, e.g. x[1] ~ ... and x[2] ~ .... This line refers to the symbol x not the random variables x[1] and x[2]. The requirement here is that each symbol, e.g. x, is visited at least once. I will try to make this clearer.

xukai92 · 2019-04-26T23:35:20Z

src/core/RandomVariables.jl

+end
+```
+
+A variable identifier. Every variable has a symbol `sym`, indices `indexing`, and internal fields: `csym` and `counter`. The Julia variable in the model corresponding to `sym` can refer to a single value or to a hierarchical array structure of univariate, multivariate or matrix variables. `indexing` stores the indices that can access the random variable from the Julia variable. 


Or 92 which is the number from our guide.

xukai92 · 2019-04-26T23:46:57Z

src/core/RandomVariables.jl

+- `md.vals[md.ranges[md.idcs[vn]]]` is the vector of values of corresponding to `vn`.
+- `md.flags` is a dictionary of true/false flags. `md.flags[flag][md.idcs[vn]]` is the value of `flag` corresponding to `vn`. 
+
+To make `md::Metadata` type stable, all the `md.vns` must have the same symbol and distribution type. However, one can have a Julia variable, say `x`, that is a matrix or a hierarchical array sampled in partitions, e.g. `x[1][:] ~ MvNormal(zeros(2), 1.0); x[2][:] ~ MvNormal(ones(2), 1.0)` and is managed by a single `md::Metadata` so long as all the distributions on the RHS of `~` are of the same type. Type unstable `Metadata` will still work but will have inferior performance. When sampling, the first iteration uses a type unstable `Metadata` for all the variables then a specialized `Metadata` is used for each symbol along with a function barrier to make the rest of the sampling type stable.


xukai92 · 2019-04-26T23:50:36Z

src/core/RandomVariables.jl


+A light wrapper over one or more instances of `Metadata`. Let `vi` be an instance of `Metadata`. If `vi isa VarInfo{<:Metadata}`, then only `Metadata` instance is used for all the sybmols. `VarInfo{<:Metadata}` is aliased `UntypedVarInfo`. If `vi isa VarInfo{<:NamedTuple}`, then `vi.metadata` is a `NamedTuple` that maps each symbol used on the LHS of `~` in the model to its `Metadata` instance. The latter allows for the type specialization of `vi` after the first sampling iteration when all the symbols have been observed. `VarInfo{<:NamedTuple}` is aliased `TypedVarInfo`.


Let vi be an instance of Metadata. -> Let vi be an instance of VarInfo ?

xukai92 · 2019-04-26T23:53:32Z

src/core/RandomVariables.jl

+
+To make `md::Metadata` type stable, all the `md.vns` must have the same symbol and distribution type. However, one can have a Julia variable, say `x`, that is a matrix or a hierarchical array sampled in partitions, e.g. `x[1][:] ~ MvNormal(zeros(2), 1.0); x[2][:] ~ MvNormal(ones(2), 1.0)` and is managed by a single `md::Metadata` so long as all the distributions on the RHS of `~` are of the same type. Type unstable `Metadata` will still work but will have inferior performance. When sampling, the first iteration uses a type unstable `Metadata` for all the variables then a specialized `Metadata` is used for each symbol along with a function barrier to make the rest of the sampling type stable.
+"""
+struct Metadata{TIdcs <: Dict{<:VarName,Int}, TDists <: AbstractVector{<:Distribution}, TVN <: AbstractVector{<:VarName}, TVal <: AbstractVector{<:Real}, TGIds <: AbstractVector{Set{Selector}}}


In fact, I guess in the future, we could have two types of Metadata which are NativeMetadata and FlatMetadata. The first one being not flatten the native Julia variable at all, as in SMC and PG they are not required to be flatten. I think this would give us some performance back from flattening and reconstruction.

Hmm, avoiding the flattening is an interesting idea. I am not sure what the best way to handle this is. Should probably open an issue to discuss it.

xukai92 · 2019-04-27T01:16:06Z

src/core/RandomVariables.jl

+
+This function finds all the unique `sym`s from the instances of `VarName{sym}` found in `vi.metadata.vns`. It then extracts the metadata associated with each symbol from the global `vi.metadata` field. Finally, a new `VarInfo` is created with a new `metadata` as a `NamedTuple` mapping from symbols to type-stable `Metadata` instances, one for each symbol.
+"""
+function TypedVarInfo(vi::UntypedVarInfo)


I didn't check line by line but I'm happy as long as it has some test cases.

xukai92 · 2019-04-27T01:27:07Z

src/core/RandomVariables.jl

 getlogp(vi::AbstractVarInfo) = vi.logp
+
+"""
+`setlogp!(vi::VarInfo, logp::Real)`


Just for discussion. I guess there is no way in Julia to make a filed "private" and force people using setlogp! instead of vi.log = . Do you think it's actually a good idea for us to have setlogp! at all?

FYI, this set of get/set functions were originally introduced by me at a time I was new to Julia and only know the practise of OO programming.

We can overload setproperty! for :log and default that to an error, but that's probably overkill unless there is a good reason to stop people from doing this.

I'm happy with removing those setter and getter methods if we don't need them. I don't see much benefit.

xukai92 · 2019-04-27T01:27:40Z

src/core/RandomVariables.jl

 function link!(vi::UntypedVarInfo, spl::Sampler)
-    vns = getvns(vi, spl)
+    # TODO: Change to a lazy iterator over `vns`


What does it mean?

As in we don't actually need to materialize vns, we can replace it with a lazy iterator that loops over the relevant vns.

xukai92 · 2019-04-27T02:08:49Z

src/core/RandomVariables.jl

+Base.getindex(vi::UntypedVarInfo, spl::Sampler) = copy(getval(vi, _getranges(vi, spl)))
+function Base.getindex(vi::TypedVarInfo, spl::Sampler)
+    # Gets the ranges as a NamedTuple
+    # getfield(ranges, f) is all the indices in `vals` of the `vn`s with symbol `f` sampled by `spl` in `vi`


There is no getfield(ranges, f) any more.

ranges refers to the output variable from _getranges. Calling getfield(ranges, f) is what this line refers to. I will try to make it clearer.

xukai92 · 2019-04-27T02:10:18Z

src/core/RandomVariables.jl

+    return vcat(_get(vi.metadata, ranges)...)
+end
+# Recursively builds a tuple of the `vals` of all the symbols
+@inline function _get(metadata::NamedTuple{names}, ranges) where {names}


Maybe name it as _getindex as it's only used by Base.getindex.

xukai92 · 2019-04-27T02:13:32Z

src/core/RandomVariables.jl


+"""


Bookmark for Kai.

trappmartin

Look good to me! Awesome work.

trappmartin · 2019-05-01T12:11:44Z

src/core/RandomVariables.jl

 VarName(csym, sym, indexing, counter) = VarName{sym}(csym, indexing, counter)
+function VarName(csym::Symbol, sym::Symbol, indexing::String)
+    # TODO: update this method when implementing the sanity check
+    VarName{sym}(csym, indexing, 1)


Is it correct that the counter is always 1 in the constructor?

I haven't changed this behavior. Maybe @xukai92 knows better.

trappmartin · 2019-05-01T12:19:55Z

src/core/RandomVariables.jl

 getlogp(vi::AbstractVarInfo) = vi.logp
+
+"""
+`setlogp!(vi::VarInfo, logp::Real)`


I'm happy with removing those setter and getter methods if we don't need them. I don't see much benefit.

yebai · 2019-05-03T13:38:38Z

Current exported API

Turing.jl/src/core/RandomVariables.jl

Lines 13 to 36 in 06e0dbb

    
           export  VarName,  
        
                   AbstractVarInfo, 
        
                   VarInfo, 
        
                   UntypedVarInfo, 
        
                   uid,  
        
                   sym,  
        
                   getlogp,  
        
                   set_retained_vns_del_by_spl!,  
        
                   resetlogp!,  
        
                   is_flagged,  
        
                   unset_flag!,  
        
                   setgid!,  
        
                   copybyindex,  
        
                   setorder!,  
        
                   updategid!,  
        
                   acclogp!,  
        
                   istrans,  
        
                   link!,  
        
                   invlink!,  
        
                   setlogp!,  
        
                   getranges,  
        
                   getrange,  
        
                   getvns,  
        
                   getval

logp

Turing.jl/src/core/RandomVariables.jl

Lines 169 to 176 in 06e0dbb

    
           function Turing.runmodel!(model::Model, vi::AbstractVarInfo, spl::AbstractSampler = SampleFromPrior()) 
        
               setlogp!(vi, zero(Float64)) 
        
               if spl isa Sampler && isdefined(spl.info, :eval_num) 
        
                   spl.info.eval_num += 1 
        
               end 
        
               model(vi, spl) 
        
               return vi 
        
           end

Key data types

VarName

Turing.jl/src/core/RandomVariables.jl

Lines 43 to 47 in 06e0dbb

    
           struct VarName{sym} 
        
               csym      ::    Symbol 
        
               indexing  ::    String 
        
               counter   ::    Int 
        
           end

Metadata

Turing.jl/src/core/RandomVariables.jl

Line 212 in 06e0dbb

    
           struct Metadata{TIdcs <: Dict{<:VarName,Int}, TDists <: AbstractVector{<:Distribution}, TVN <: AbstractVector{<:VarName}, TVal <: AbstractVector{<:Real}, TGIds <: AbstractVector{Set{Selector}}}

VarInfo, TypedVarInfo, and UntypedVarInfo

Turing.jl/src/core/RandomVariables.jl

Lines 310 to 316 in 06e0dbb

    
           struct VarInfo{Tmeta, Tlogp} <: AbstractVarInfo 
        
               metadata::Tmeta 
        
               logp::Base.RefValue{Tlogp} 
        
               num_produce::Base.RefValue{Int} 
        
           end 
        
           const UntypedVarInfo = VarInfo{<:Metadata} 
        
           const TypedVarInfo = VarInfo{<:NamedTuple}

View

Turing.jl/src/core/RandomVariables.jl

Line 431 in 06e0dbb

const VarView = Union{Int, UnitRange, Vector{Int}}

Internal / utility functions

Turing.jl/src/core/RandomVariables.jl

Lines 428 to 429 in 06e0dbb

vns(vi::UntypedVarInfo) = Set(keys(vi.idcs)) # get all vns

Base.keys(vi::UntypedVarInfo) = keys(vi.idcs)

getval and setval!:

Turing.jl/src/core/RandomVariables.jl

Line 438 in 06e0dbb

getval(vi::UntypedVarInfo, vview::VarView) = view(vi.vals, vview)

Turing.jl/src/core/RandomVariables.jl

Lines 445 to 446 in 06e0dbb

    
           setval!(vi::UntypedVarInfo, val, vview::VarView) = vi.vals[vview] = val 
        
           function setval!(vi::UntypedVarInfo, val, vview::Vector{UnitRange})

Turing.jl/src/core/RandomVariables.jl

Lines 499 to 500 in 06e0dbb

    
           getval(vi::UntypedVarInfo, vn::VarName) = view(vi.vals, getrange(vi, vn)) 
        
           function getval(vi::TypedVarInfo, vn::VarName{sym}) where sym

Turing.jl/src/core/RandomVariables.jl

Lines 510 to 511 in 06e0dbb

    
           setval!(vi::UntypedVarInfo, val, vn::VarName) = vi.vals[getrange(vi, vn)] = val 
        
           function setval!(vi::TypedVarInfo, val, vn::VarName{sym}) where sym

Turing.jl/src/core/RandomVariables.jl

Lines 521 to 522 in 06e0dbb

    
           getval(vi::UntypedVarInfo, vns::Vector{<:VarName}) = view(vi.vals, getranges(vi, vns)) 
        
           function getval(vi::TypedVarInfo, vns::Vector{VarName{sym}}) where sym

Turing.jl/src/core/RandomVariables.jl

Lines 532 to 533 in 06e0dbb

getall(vi::UntypedVarInfo) = vi.vals

getall(vi::TypedVarInfo) = vcat(_getall(vi.metadata)...)

Turing.jl/src/core/RandomVariables.jl

Lines 549 to 551 in 06e0dbb

    
           setall!(vi::UntypedVarInfo, val) = vi.vals .= val 
        
           setall!(vi::TypedVarInfo, val) = _setall!(vi.metadata, val) 
        
           @inline function _setall!(metadata::NamedTuple{names}, val, start = 0) where {names}

getidx:

Turing.jl/src/core/RandomVariables.jl

Line 458 in 06e0dbb

getidx(vi::UntypedVarInfo, vn::VarName) = vi.idcs[vn]
Turing.jl/src/core/RandomVariables.jl

Line 465 in 06e0dbb

function getidx(vi::TypedVarInfo, vn::VarName{sym}) where sym
Turing.jl/src/core/RandomVariables.jl

Line 784 in 06e0dbb

function Base.getindex(vi::AbstractVarInfo, vn::VarName)
Turing.jl/src/core/RandomVariables.jl

Line 832 in 06e0dbb

Base.setindex!(vi::AbstractVarInfo, val::Any, vn::VarName) = setval!(vi, val, vn)

getrange

Turing.jl/src/core/RandomVariables.jl

Line 474 in 06e0dbb

getrange(vi::UntypedVarInfo, vn::VarName) = vi.ranges[getidx(vi, vn)]
Turing.jl/src/core/RandomVariables.jl

Line 481 in 06e0dbb

function getrange(vi::TypedVarInfo, vn::VarName{sym}) where sym
Turing.jl/src/core/RandomVariables.jl

Line 490 in 06e0dbb

function getranges(vi::AbstractVarInfo, vns::Vector{<:VarName})

Turing.jl/src/core/RandomVariables.jl

Lines 988 to 991 in 06e0dbb

    
           # Get all indices of variables belonging to SampleFromPrior: 
        
           #   if the gid/selector of a var is an empty Set, then that var is assumed to be assigned to 
        
           #   the SampleFromPrior sampler 
        
           function _getidcs(vi::UntypedVarInfo, ::SampleFromPrior)

Turing.jl/src/core/RandomVariables.jl

Lines 1052 to 1054 in 06e0dbb

    
           # Get all vns of variables belonging to spl 
        
           _getvns(vi::UntypedVarInfo, spl::AbstractSampler) = view(vi.vns, _getidcs(vi, spl)) 
        
           function _getvns(vi::TypedVarInfo, spl::AbstractSampler)

Turing.jl/src/core/RandomVariables.jl

Line 1074 in 06e0dbb

function _getranges(vi::AbstractVarInfo, spl::Sampler)

setorder!

Turing.jl/src/core/RandomVariables.jl

Lines 969 to 974 in 06e0dbb

    
           `setorder!(vi::VarInfo, vn::VarName, index::Int)` 
        
           Sets the `order` of `vn` in `vi` to `index`, where `order` is the number of `observe  
        
           statements run before sampling `vn`. 
        
           """ 
        
           function setorder!(vi::UntypedVarInfo, vn::VarName, index::Int)

Turing.jl/src/core/RandomVariables.jl

Line 980 in 06e0dbb

function setorder!(mvi::TypedVarInfo, vn::VarName{sym}, index::Int) where {sym}

flagging and gid functions:

Turing.jl/src/core/RandomVariables.jl

Line 1113 in 06e0dbb

function is_flagged(vi::UntypedVarInfo, vn::VarName, flag::String)
Turing.jl/src/core/RandomVariables.jl

Line 1125 in 06e0dbb

function set_flag!(vi::UntypedVarInfo, vn::VarName, flag::String)
Turing.jl/src/core/RandomVariables.jl

Line 1137 in 06e0dbb

function unset_flag!(vi::UntypedVarInfo, vn::VarName, flag::String)
Turing.jl/src/core/RandomVariables.jl

Line 1149 in 06e0dbb

function set_retained_vns_del_by_spl!(vi::UntypedVarInfo, spl::Sampler)
Turing.jl/src/core/RandomVariables.jl

Line 1164 in 06e0dbb

function set_retained_vns_del_by_spl!(vi::TypedVarInfo, spl::Sampler)

Turing.jl/src/core/RandomVariables.jl

Lines 1195 to 1200 in 06e0dbb

    
           `updategid!(vi::VarInfo, vn::VarName, spl::Sampler)` 
        
           If `vn` doesn't have a sampler selector linked and `vn`'s symbol is in the space of  
        
           `spl`, this function will set `vn`'s `gid` to `Set([spl.selector])`. 
        
           """ 
        
           function updategid!(vi::AbstractVarInfo, vn::VarName, spl::Sampler)

mohamed82008 · 2019-05-03T14:53:37Z

I think we need to do the following in a separate PR:

Find which functions we need outside of RandomVariables to define the "necessary" API.
For internal functions, we can use any names we like.
For functions needed outside, we can try to find Base functions with similar enough semantics and overload those instead to minimize the exported names and make it easier to remember.

yebai · 2019-05-03T14:59:09Z

I think we need to do the following in a separate PR:

Sounds good to do the API redesign in a separate PR.

…gLang/Turing.jl into mt/ts_wave2_untypedvarinfo

mohamed82008 · 2019-05-11T15:40:33Z

Rebased on master

xukai92 · 2019-05-13T05:36:24Z

Looks great to me.

The @code_warntype check doesn't pass on my local, althought it's not the issue from this PR.

 @model gdemo_d() = begin
    s ~ InverseGamma(2, 3)
    m ~ Normal(0, sqrt(s))
    1.5 ~ Normal(m, sqrt(s))
    2.0 ~ Normal(m, sqrt(s))
    return s, m
end

mf = gdemo_d()

@code_warntype sample(mf, NUTS(2000, 0.8))
Body::Any
1 ─ %1 = invoke Turing.Inference.AHMCAdaptor(_3::NUTS{Turing.Core.ForwardDiffAD{40},Any})::AdvancedHMC.Adaptation.StanNUTSAdaptor
│   %2 = (Turing.Inference.:(#sample#19))(false, Turing.Inference.nothing, 0, %1, Turing.Inference.nothing, Turing.Inference.GLOBAL_RNG, $(QuoteNode(Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}())), #self#, model, alg)::Any
└──      return %2

xukai92 · 2019-05-13T05:38:20Z

OK it seems only the return type is not stable but not during sampling?

mohamed82008 · 2019-05-13T09:15:56Z

No, this PR doesn't hook into sample yet, this will be the next PR. And even if I hook them, sample won't be type stable; we need to address the info field of Sampler and the value field of Sample to get complete type stability from the second iteration onwards. Currently, spl.info is Dict{Symbol, Any} so when we retrieve values from it during sampling, the compiler can't infer its type and this muddies the rest of the sampling function. So 2 more PRs are needed to get complete type stability from the second iteration onwards for all algorithms:

Hooking TypedVarInfo to sample, and
Making spl.info and sample.value type stable.

mohamed82008 · 2019-05-13T09:21:14Z

Something weird happened when I merged, I will fix the error.

yebai · 2019-05-13T10:27:16Z

Making spl.info and sample.value type stable.

This issue should go away after #746 is fixed.

yebai requested review from KDr2 and xukai92 April 5, 2019 18:32

xukai92 reviewed Apr 6, 2019

View reviewed changes

yebai mentioned this pull request Apr 19, 2019

Plan for release 0.20 #689

Closed

56 tasks

yebai reviewed Apr 25, 2019

View reviewed changes

xukai92 reviewed Apr 27, 2019

View reviewed changes

yebai mentioned this pull request Apr 29, 2019

RFC Sampler type #771

Closed

trappmartin reviewed May 1, 2019

View reviewed changes

mohamed82008 force-pushed the mt/ts_wave2_untypedvarinfo branch from 06e0dbb to 7dc8e05 Compare May 5, 2019 15:42

mohamed82008 added 13 commits May 9, 2019 22:16

add typed varinfo

a10379e

fix tests and cache_updated

ee0353e

remove generated functions

4174442

reorganization and some docstrings and comments

50ebcae

cleanup and docstrings

1b8ce6b

some more comments and docstrings

a831afa

more comments and docstrings

5eba502

more comments, fixes and docstrings

a33ea92

some test fixes

bb44ae8

add final comments and docstrings in RandomVariables src

74e1a1e

respond to review comments

e98ce91

RandomVariables reorganization

e3a22ce

fix eval_num

7b7e714

mohamed82008 added 20 commits May 11, 2019 21:42

cleanup and docstrings

2c2f24a

some more comments and docstrings

7486f34

more comments and docstrings

7abbfb0

more comments, fixes and docstrings

133729c

some test fixes

7df0bbb

add final comments and docstrings in RandomVariables src

0408e26

respond to review comments

ec06961

RandomVariables reorganization

fab7960

fix eval_num

879b211

reorg and add some tests

c8c36b6

logp tests

9a775fa

add link! tests

2843d83

fix link! and invlink! for TypedVarInfo

8be7dc1

add setgid! test

8311a9f

pass spl to vi when pushing

1b6d158

add TypedVarInfo constructor test

a8486a3

turing_test -> turing_testset

f28ee9e

merge RandomVariables files into one

c29393b

fix tests

9a78993

Merge branch 'mt/ts_wave2_untypedvarinfo' of https://github.com/Turin…

bd7cf3e

…gLang/Turing.jl into mt/ts_wave2_untypedvarinfo

fix merge typo

928a2f6

yebai merged commit 51b7880 into master May 13, 2019

yebai deleted the mt/ts_wave2_untypedvarinfo branch June 8, 2019 22:34

mohamed82008 mentioned this pull request Jun 9, 2019

VarInfo performance and model types #620

Closed

		dists = [Normal(0, 1), MvNormal([0; 0], [1.0 0; 0 1.0]), Wishart(7, [1 0.5; 0.5 1])]
		function test_varinfo!(vi)


		Examples:

		- `x[2] ~ Normal()` will generate a `VarName` with `sym == :x` and `indexing == "[1]"`


		A light wrapper over one or more instances of `Metadata`. Let `vi` be an instance of `Metadata`. If `vi isa VarInfo{<:Metadata}`, then only `Metadata` instance is used for all the sybmols. `VarInfo{<:Metadata}` is aliased `UntypedVarInfo`. If `vi isa VarInfo{<:NamedTuple}`, then `vi.metadata` is a `NamedTuple` that maps each symbol used on the LHS of `~` in the model to its `Metadata` instance. The latter allows for the type specialization of `vi` after the first sampling iteration when all the symbols have been observed. `VarInfo{<:NamedTuple}` is aliased `TypedVarInfo`.

		Note: It is the user's responsibility to ensure that each symbol is visited at least once whenever the model is called, regardless of any stochastic branching.

TS 2: introduce TypedVarInfo and fix spl.info[:cache_updated] #742

TS 2: introduce TypedVarInfo and fix spl.info[:cache_updated] #742

Conversation

mohamed82008 commented Mar 31, 2019 • edited Loading

mohamed82008 commented Mar 31, 2019 • edited Loading

yebai commented Apr 3, 2019

mohamed82008 commented Apr 3, 2019

xukai92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamed82008 commented Apr 7, 2019

mohamed82008 commented Apr 7, 2019 • edited Loading

yebai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamed82008 Apr 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trappmartin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebai commented May 3, 2019 • edited Loading

mohamed82008 commented May 3, 2019

yebai commented May 3, 2019

mohamed82008 commented May 11, 2019

xukai92 commented May 13, 2019

xukai92 commented May 13, 2019

mohamed82008 commented May 13, 2019

mohamed82008 commented May 13, 2019

yebai commented May 13, 2019

mohamed82008 commented Mar 31, 2019 •

edited

Loading

mohamed82008 commented Mar 31, 2019 •

edited

Loading

mohamed82008 commented Apr 7, 2019 •

edited

Loading

mohamed82008 Apr 27, 2019 •

edited

Loading

yebai commented May 3, 2019 •

edited

Loading