Refactoring of compiler #513

trappmartin · 2018-09-10T14:03:13Z

This is a major refactoring of the compiler code. Currently, this PR is work in progress and should not be merged. The PR should be finished soon.

Todo:

fix issue Clean up the compiler code #501
fix issue Compiler: remove return vi::VarInfo. #483
fix issue Compiler: move model_f(;data=Dict()) from compiler to utilities #475
fix prior drawing described in Default sampling behaviour #446
make inplace updated of vi.logp and initialize vi.logp = zero(Real)
add more tests, e.g. ensure args and kwargs are always passed correctly
fix breaking tests
ensure code style according to guidelines

willtebbutt

This looks great, much cleaner than before. The thing that I feel most strongly about regards the use of MacroTools to make some code more robust and to avoid reinventing the wheel. Please see my specific comments on that.

willtebbutt · 2018-09-10T14:09:55Z

src/core/compiler.jl

+
+Extract function name, arguments and body.
+"""
+function extractcomponents(fexpr::Expr)


Please consider using MacroTools.jl here. It's splitdef function is designed precisely to do the job of this function.

src/core/compiler.jl

willtebbutt · 2018-09-10T14:10:33Z

src/core/compiler.jl


 function insdelim(c, deli=",")
-  reduce((e, res) -> append!(e, [res, deli]), c; init = [])[1:end-1]
+    reduce((e, res) -> append!(e, [res, deli]), c; init = [])[1:end-1]


return statement please. Also, this function appears to only be used in is_inside.jl, so perhaps we could move it to that file?

Good point, I'll move it into is_inside.jl

willtebbutt · 2018-09-10T14:10:45Z

src/core/compiler.jl

+    while isa(curr, Expr) && curr.head == :ref
+        curr = curr.args[1]
+    end
+    curr


willtebbutt · 2018-09-10T14:10:49Z

src/core/compiler.jl

+    else
+        map(translate!, ex.args)
+    end
+    ex


willtebbutt · 2018-09-10T14:28:39Z

src/core/compiler.jl

+    fname_inner = Symbol("$(fname)_model")
+    fname_inner_str = string(fname_inner)
+    fdefn_inner_func = constructfunc(
+                                     fname_inner,


Indentation (again, sorry)

src/core/compiler.jl

willtebbutt · 2018-09-10T14:47:46Z

src/core/compiler.jl

+    push!(fargs_outer[1].args, Expr(:kw, :compiler, compiler))
+
+    # turn f(x;..) into f(x=nothing;..)
+    for i = 2:length(fargs_outer)


(Personal preference) This is maybe a cleaner version:

fargs_outer = map(x -> x isa Symbol ? Expr(:kw, x, :nothing) : x, fargs_outer[2:end])

src/core/compiler.jl

trappmartin · 2018-09-10T15:44:56Z

Thanks for reviewing, I'll consider using MacroTools to improve the code.

willtebbutt · 2018-09-10T15:56:26Z

Regarding the @VarName macro, here's a sketch of the function that I would propose to replace it with:

make_varname(expr::Symbol) = (expr, (), gensym())
function make_varname(expr::Expr)
    @assert expr.head ==  :ref "VarName: Malformed variable name $(expr)"
    return _make_varname(expr, Vector{Union{Symbol, Expr}}())..., gensym()
end
function _make_varname(expr::Expr, indices::Vector{Union{Symbol, Expr}})
    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

(named make_varname to distinguish from the other varname function that we have.)

This doesn't quite work correctly, in particular the type of the second thing returned isn't quite the same as what @VarName currently returns, but it shouldn't be hard to refactor to make it return the right thing. eg. run make_varname(:(x[5][4])) and @VarName x[5][4] to see the difference.

yebai · 2018-09-10T16:05:16Z

Regarding the @varname macro, here's a sketch of the function that I would propose to replace it with:

There are cases where a function isn't enough, and a macro is required. Consider the following case

x = 1
y = 2
a = Array(undef, 4,4)

a[x,y] ~ Normal(0,1)

Here the indexing only becomes available at runtime. So we have to generate the variable name using a macro.

willtebbutt · 2018-09-10T17:01:14Z

@yebai I completely agree that we require a way to construct indices dynamically, but disagree that it necessitates the extra macro. I'll open a separate issue to discuss so that we can resolve this in a separate PR.

yebai · 2018-09-12T11:34:35Z

Hi @trappmartin, here is an updated compiler design after removing model_f(;data=Dict()) interface (#475). I found this very helpful when reasoning about closures. Hope this helps for refactoring the code.

    # Compiler design: sample(f_compiletime(x,y), sampler)
    #   f_compiletime(x, y; compiler=compiler) = begin
    #      ex = quote
    #          f_runtime(vi::VarInfo, sampler::Sampler; x = x, y = y) = begin
    #              # pour model definition `fbody`, e.g.
    #              x ~ Normal(0,1)
    #              y ~ Normal(x, 1)
    #          end
    #      end
    #      Main.eval(ex)
    #   end

…ing to guidelines

trappmartin · 2018-09-13T17:31:27Z

The current code breaks the ad.jl tests. I'm not sure why this is the case and I don't recall observing this before the last rebase.

ad1.jl: Error During Test at /home/travis/build/TuringLang/Turing.jl/test/utility.jl:73
  Got exception outside of a @test
  LoadError: DivideError: integer division error
  Stacktrace:
   [1] rem at ./int.jl:233 [inlined]
   [2] chunk_mode_gradient!(::Array{Float64,1}, ::getfield(Turing, Symbol("#f#84")){Turing.VarReplay.VarInfo,typeof(ad_test_model),Nothing}, ::Array{Float64,1}, ::ForwardDiff.GradientConfig{ForwardDiff.Tag{getfield(Turing, Symbol("#f#84")){Turing.VarReplay.VarInfo,typeof(ad_test_model),Nothing},Float64},Float64,0,Array{Dual{ForwardDiff.Tag{getfield(Turing, Symbol("#f#84")){Turing.VarReplay.VarInfo,typeof(ad_test_model),Nothing},Float64},Float64,0},1}}) at /home/travis/.julia/packages/ForwardDiff/hnKaN/src/gradient.jl:127
   [3] gradient!(::Array{Float64,1}, ::getfield(Turing, Symbol("#f#84")){Turing.VarReplay.VarInfo,typeof(ad_test_model),Nothing}, ::Array{Float64,1}, ::ForwardDiff.GradientConfig{ForwardDiff.Tag{getfield(Turing, Symbol("#f#84")){Turing.VarReplay.VarInfo,typeof(ad_test_model),Nothing},Float64},Float64,0,Array{Dual{ForwardDiff.Tag{getfield(Turing, Symbol("#f#84")){Turing.VarReplay.VarInfo,typeof(ad_test_model),Nothing},Float64},Float64,0},1}}) at /home/travis/.julia/packages/ForwardDiff/hnKaN/src/gradient.jl:37
   [4] gradient_forward(::Array{Float64,1}, ::Turing.VarReplay.VarInfo, ::Function, ::Nothing, ::Int64) at /home/travis/build/TuringLang/Turing.jl/src/core/ad.jl:32
   [5] gradient_forward(::Array{Float64,1}, ::Turing.VarReplay.VarInfo, ::Function) at /home/travis/build/TuringLang/Turing.jl/src/core/ad.jl:21

  ad.jl                                    |    6      2              8
    AD_compatibility_with_distributions.jl |                      No tests
    ad1.jl                                 |           1              1
    ad2.jl                                 |                      No tests
    ad3.jl                                 |           1              1
    pass_dual_to_dists.jl                  |    6                     6

Does anybody have an idea why the ad tests break?

cc: @yebai @xukai92

willtebbutt · 2018-09-13T17:46:34Z

Looks like CHUNK_SIZE isn't getting changed from zero for some reason. Just changing the initialisation for this constant to something other than zero (10?) sorts it out.

const CHUNKSIZE = Ref(10) # default chunksize used by AD

on line 51 of Turing.jl

trappmartin · 2018-09-14T10:53:21Z

Thanks, @yebai! The assume function calls look much better now.

…Turing.jl into ref/compiler_rebased

Refactoring of compiler

willtebbutt self-requested a review September 10, 2018 14:27

trappmartin changed the title ~~Refactoring of compiler~~ Refactoring of compiler (WIP) Sep 10, 2018

willtebbutt requested changes Sep 10, 2018

View reviewed changes

trappmartin added this to the 0.5 milestone Sep 10, 2018

This was referenced Sep 10, 2018

@VarName can be replaced with a function #514

Closed

Tidy up AD interface + remove cleandual! and realpart!. Do not merge. #515

Merged

trappmartin force-pushed the ref/compiler_rebased branch from 3fcdb47 to ee5c19c Compare September 12, 2018 15:30

yebai and others added 18 commits September 12, 2018 20:58

Small cleanup.

a234565

refactoring of compiler

3213f87

removed data macro

026d505

refactoring of tilde macro and fixed minor bug. Observer test broken.

66d0a5a

improved comment

8a5f8b5

changed IS to work with new model function scheme

431bfdb

changed hmc core to work with new model function, changed style accor…

9b5336d

…ing to guidelines

changed model function calls

d752e8d

fixing compiler tests

4ccd2f6

fixed varinfo test

3da1ba2

re-added kwarg checking

705b2b6

fixed missing data insertion problem

6fbd52d

minor fix

aca1334

lot of changes, wip

32e791d

code now uses MacroTools, with a few exceptions

03eec79

work in progress

0bc88e5

more refactoring, solving prior sampling in #446

d0b6e99

removed strange tabs

3afb276

willtebbutt mentioned this pull request Sep 13, 2018

Debug statements etc #526

Closed

trappmartin added 8 commits September 13, 2018 15:06

runmodel -> runmodel!

3902ec5

unifying initilistation for unconstrained distributions

ad8f35f

changing vi.logp inplace, added some tests

65181cb

added fix for #475 by implementing a data(Dict, Vector{Symbol}) function

8e55f59

typo

8effc9e

changed default assume behavior to sample from the prior

6293164

added more tests for the compiler

979b7b7

changed back to use initialisation instead of prior draws

b000580

trappmartin added 2 commits September 13, 2018 18:57

changed default value of CHUNKSIZE to 10

531f75c

Merge branch 'master' into ref/compiler_rebased

94b0566

trappmartin changed the title ~~Refactoring of compiler (WIP)~~ Refactoring of compiler Sep 13, 2018

yebai force-pushed the ref/compiler_rebased branch from 0c9402e to 2881f04 Compare September 13, 2018 20:42

Introduce a special algorithm for initializing HMC.

d3306d1

yebai force-pushed the ref/compiler_rebased branch from 2881f04 to d3306d1 Compare September 13, 2018 21:10

trappmartin added 3 commits September 14, 2018 12:33

clean up

1252169

minor fix

e8e7005

added MacroTools to REQUIRE

80ada41

trappmartin mentioned this pull request Sep 14, 2018

Compiler generated aliases #533

Closed

willtebbutt added 2 commits September 14, 2018 16:29

Minor fixes + style from meeting

3ee9408

Merge branch 'ref/compiler_rebased' of https://github.com/TuringLang/…

a8e8d5f

…Turing.jl into ref/compiler_rebased

willtebbutt approved these changes Sep 14, 2018

View reviewed changes

willtebbutt merged commit 8b7d753 into master Sep 14, 2018

yebai deleted the ref/compiler_rebased branch September 14, 2018 19:33

yebai mentioned this pull request Sep 14, 2018

Better error information from compiler #239

Closed

yebai pushed a commit that referenced this pull request Sep 18, 2018

Merge pull request #513 from TuringLang/ref/compiler_rebased

10de954

Refactoring of compiler

xukai92 mentioned this pull request Sep 19, 2018

Gradient error in forward AD #550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of compiler #513

Refactoring of compiler #513

trappmartin commented Sep 10, 2018 •

edited

Loading

willtebbutt left a comment

willtebbutt Sep 10, 2018

willtebbutt Sep 10, 2018

trappmartin Sep 10, 2018

willtebbutt Sep 10, 2018

willtebbutt Sep 10, 2018

willtebbutt Sep 10, 2018

willtebbutt Sep 10, 2018

trappmartin commented Sep 10, 2018

willtebbutt commented Sep 10, 2018

yebai commented Sep 10, 2018

willtebbutt commented Sep 10, 2018

yebai commented Sep 12, 2018 •

edited

Loading

trappmartin commented Sep 13, 2018 •

edited

Loading

willtebbutt commented Sep 13, 2018

trappmartin commented Sep 14, 2018

Refactoring of compiler #513

Refactoring of compiler #513

Conversation

trappmartin commented Sep 10, 2018 • edited Loading

willtebbutt left a comment

Choose a reason for hiding this comment

willtebbutt Sep 10, 2018

Choose a reason for hiding this comment

willtebbutt Sep 10, 2018

Choose a reason for hiding this comment

trappmartin Sep 10, 2018

Choose a reason for hiding this comment

willtebbutt Sep 10, 2018

Choose a reason for hiding this comment

willtebbutt Sep 10, 2018

Choose a reason for hiding this comment

willtebbutt Sep 10, 2018

Choose a reason for hiding this comment

willtebbutt Sep 10, 2018

Choose a reason for hiding this comment

trappmartin commented Sep 10, 2018

willtebbutt commented Sep 10, 2018

yebai commented Sep 10, 2018

willtebbutt commented Sep 10, 2018

yebai commented Sep 12, 2018 • edited Loading

trappmartin commented Sep 13, 2018 • edited Loading

willtebbutt commented Sep 13, 2018

trappmartin commented Sep 14, 2018

trappmartin commented Sep 10, 2018 •

edited

Loading

yebai commented Sep 12, 2018 •

edited

Loading

trappmartin commented Sep 13, 2018 •

edited

Loading