Specialize on Types and use if-else instead of Val-dispatches

[devmotion]

The PR tries to improve a few things I noticed when skimming through the code, most noteably

• missing specializations on Types: Julia does not specialize on T::Type, one has to use ::Type{T}

• misuse of Val in type-unstable code involving Symbols: this does not recover type stability but leads to dynamic dispatches which are quite slow. Compare, e.g.,

julia> using Chairmarks

julia> f(x::Symbol) = f(Val(x))

julia> f(::Val{:a}) = 1

julia> f(::Val{:b}) = "test"

julia> f(::Val{:c}) = 2.4

julia> f(@nospecialize(x)) = error("$x is not supported.")

julia> function g(x::Symbol)
           if x === :a
               return 1
           elseif x === :b
               return "test"
           elseif x === :c
               return 2.4
           else
               error("$x is not supported.")
           end
       end

julia> @be rand([:a, :b, :c]) f
Benchmark: 2236 samples with 572 evaluations
 min    45.892 ns (<0.01 allocs: 0.196 bytes)
 median 50.407 ns (<0.01 allocs: 0.196 bytes)
 mean   51.162 ns (<0.01 allocs: 0.196 bytes)
 max    151.806 ns (<0.01 allocs: 0.196 bytes)

julia> @be rand([:a, :b, :c]) g
Benchmark: 3178 samples with 8368 evaluations
 min    2.594 ns (<0.01 allocs: 0.013 bytes)
 median 2.988 ns (<0.01 allocs: 0.013 bytes)
 mean   3.008 ns (<0.01 allocs: 0.013 bytes)
 max    5.492 ns (<0.01 allocs: 0.013 bytes)

[torfjelde]

This does mean that we can't easily extend the options to construct other samplers though 😕 A bit annoying if we have to mess around with if statements for this, no?

And do we really care that the constructors result in dynamic dispatch?

[devmotion]

A bit annoying if we have to mess around with if statements for this, no?

IMO the whole Symbol-based design is flawed but I didn't want to right away make any more fundamental changes.

And do we really care that the constructors result in dynamic dispatch?

I started to go through the code since we saw performance regressions when updating from AdvancedHMC 0.4 to 0.6 (and therefore had to revert to 0.4 for the time being). We are not dealing with these constructors though, it was just a by-product.

[torfjelde]

IMO the whole Symbol-based design is flawed but I didn't want to right away make any more fundamental changes.

What would you suggest instead?:)

I started to go through the code since we saw performance regressions when updating from AdvancedHMC 0.4 to 0.6 (and therefore had to revert to 0.4 for the time being). We are not dealing with these constructors though, it was just a by-product.

Hmm interesting. How bad?

[devmotion]

What would you suggest instead?:)

IMO Symbols would be OK-ish as long as it's not extended, even though it's conceptually a bit annoying to have to deal with both Symbols and integrator and metric types. I wonder if one could require users to always specify an integrator and a metric instance, but allow initialization to be deferred by e.g. making Leapfrog() construct a Leapfrog instance with nothings. Then make_integrator etc. would by default just return the pre-specified integrator, but for integrators with deferred initialization they would set epsilon etc. at this point.

Hmm interesting. How bad?

One of our tests regressed from

1.273538 seconds (9.12 M allocations: 3.126 GiB, 17.24% gc time)

to

1.998561 seconds (14.89 M allocations: 5.109 GiB, 18.50% gc time)

I wonder if the increase in number of allocations points to an increase in type inference issues.

[torfjelde]

I wonder if one could require users to always specify an integrator and a metric instance, but allow initialization to be deferred by e.g. making Leapfrog() construct a Leapfrog instance with nothings.

Hmm, maybe, maybe. I'm slightly worried that this is still a bit "too much" for some users. However, I do agree that the current symbol-based approach also isn't ideal 😕

[devmotion]

Isn't this the same as e.g. AutoForwardDiff() with nothing defaults for chunksizes?

[torfjelde]

Isn't this the same as e.g. AutoForwardDiff() with nothing defaults for chunksizes?

Sure, but that is a single configuration field; it does get a bit verbose if you have to do this for a several, no?

But maybe we can take this discussion elsewhere 👍

Regarding the PR, I'll approve:)