Turing.jl: @VarName can be replaced with a function

[willtebbutt]

This issue pertains to the @VarName macro in compiler.jl, which I don't believe needs to be a macro. This issue was previously discussed on #513, but it was felt that it should be dealt with in a separate PR.

In particular, replacing @VarName with a function make_varname, defined as follows:

make_varname(expr::Symbol) = (expr, (), gensym())
function make_varname(expr::Expr)
    @assert expr.head ==  :ref "VarName: Malformed variable name $(expr)"
    varname, indices = _make_varname(expr, Vector{Any}())
    index_expr = Expr(:tuple, [Expr(:vect, x) for x in indices]...)
    return varname, index_expr, gensym()
end
function _make_varname(expr::Expr, indices::Vector{Any})
    if expr.args[1] isa Symbol
        return expr.args[1], vcat(Symbol(expr.args[2]), indices)
    else
        return _make_varname(expr.args[1], vcat(Symbol(expr.args[2]), indices))
    end
end

and changing the @~ macro method that handles expressions on the lhs to

macro ~(left::Expr, right)
    vsym = getvsym(left)
    @assert isa(vsym, Symbol)

    if vsym in Turing._compiler_[:fargs]
        if ~(vsym in Turing._compiler_[:dvars])
            @info " Observe - `$(vsym)` is an observation"
            push!(Turing._compiler_[:dvars], vsym)
        end

        return generate_observe(left, right)
    else
        if ~(vsym in Turing._compiler_[:pvars])
            msg = " Assume - `$(vsym)` is a parameter"
            if isdefined(Main, vsym)
                msg  *= " (ignoring `$(vsym)` found in global scope)"
            end

            @info msg
            push!(Turing._compiler_[:pvars], vsym)
        end
        sym, indices, csym = make_varname(left)
        assume_ex = quote
            sym, idcs, csym = Symbol($(string(sym))), $indices, Symbol($(string(csym)))
            csym_str = string(Turing._compiler_[:fname])*string(csym)
            indexing = isempty(idcs) ? "" : mapreduce(idx -> string(idx), *, idcs)

            vn = Turing.VarName(vi, Symbol(csym_str), sym, indexing)

            $(left), __lp = Turing.assume(
                                          sampler,
                                          $right,   # dist
                                          vn,       # VarName
                                          vi        # VarInfo
                                         )
            _lp += __lp
        end
        return esc(assume_ex)
    end
end

appears to yield roughly the correct results. Only difference with code on #513 is about halfway down, the line

sym, indices, csym = make_varname(left)

is new, and the first line of the assume_ex has also changed.

For example, running this MWE:

using Turing, Distributions
using Turing: @model, @~

# Misc. code to instantiate `Turing._compiler_`.
@model foo() = begin
   x ~ Normal(0, 1)
end
foo()

@macroexpand @~ x[y][z] Normal(0, 1)

produces

quote
    #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:126 =#
    (sym, idcs, csym) = (Symbol("x"), ([y], [z]), Symbol("##371"))
    #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:127 =#
    csym_str = string(Turing._compiler_[:fname]) * string(csym)
    #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:128 =#
    indexing = if isempty(idcs)
            ""
        else
            mapreduce((idx->begin
                        #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:128 =#
                        string(idx)
                    end), *, idcs)
        end
    #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:130 =#
    vn = Turing.VarName(vi, Symbol(csym_str), sym, indexing)
    #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:132 =#
    ((x[y])[z], __lp) = Turing.assume(sampler, Normal(0, 1), vn, vi)
    #= /home/wct23/.julia/dev/Turing/src/core/compiler.jl:138 =#
    _lp += __lp
end
end

The point being that the first line is as required, but the rather unreadable @VarName code has been replaced with something more readable, and has removed the need to nest macro invocations. I would argue that this is a significant readability improvement.

[yebai]

With a combination of the ~ macro and make_varname, this might work. Feel free to create a PR, probably after #513 is in.

[willtebbutt]

getvsym is very closely related to this functionality. Possibly rewrite in terms of this to prevent duplicate code.

[yebai]

@mohamed82008 @willtebbutt Is this already fixed?

[willtebbutt]

@mohamed82008 have you sorted this in #660 ?

[mohamed82008]

No, VarName is still a macro. I don't personally see the appeal of changing it to a function. What that will do is basically unwrap @VarName as part of @model as opposed to letting Julia handle them hierarchically. The only bit I found confusing with @VarName is that the macro does not actually return a VarName instance, which you would expect from a macro with the same name as the type. Anyways, I can still do this in a quick PR over the weekend if you really insist, doesn't have to be part of #660.

[willtebbutt]

My main objection to nested macros is that they make the code harder to read. If there were some additional utility gained by keeping the macro (i.e. we used @VarName elsewhere in user-facing code) then I would probably be pro keeping it. As it stands we're using a macro when a function would suffice, and I can't see any particularly good reason for doing this.

[mohamed82008]

How about making @VarName take vi as a first input and make it return an instance of VarName directly? This can make it usable in user-facing code, e.g. in the advanced example http://turing.ml/docs/advanced/. So this part https://github.com/TuringLang/Turing.jl/blob/master/src/core/compiler.jl#L70 can be replaced by varname = @VarName vi var with proper interpolations.

Edit: the model_name can also be optionally passed.

[willtebbutt]

I would be pro this.

[mohamed82008]

So now after #965, we can do i = 1; @varname x[i] to get a VarName{:x}("[1]"). varname needs to be a macro because the symbol :x is extracted from the expression :(x[1]). I will close this for now. Please re-open it if you think VarName needs to be changed somehow.