Turing.jl: MethodError when initializing NUTS with default values

[mkarikom]

Pass the default values for the NUTS constructor explicitly:

using Test
# Import Turing and Distributions.
using Turing, Distributions

# Import RDatasets.
using RDatasets

# Import MCMCChains, Plots, and StatsPlots for visualizations and diagnostics.
using MCMCChains, Plots, StatsPlots

# We need a logistic function, which is provided by StatsFuns.
using StatsFuns: logistic

# Functionality for splitting and normalizing the data
using MLDataUtils: shuffleobs, stratifiedobs, rescale!

# Set a seed for reproducibility.
using Random
Random.seed!(0);

# Turn off progress monitor.
Turing.turnprogress(false)

Sd = Dict(:iter=>200,:n_adapt=>100,:δ=>0.65,:max_depth=>5,:Δ_max=>1000.0,:ϵ=>0.0)
@testset "nutsparam" begin
    @test typeof(NUTS(Sd[:n_adapt],Sd[:δ])) <: NUTS
    @test typeof(NUTS(Sd[:n_adapt],Sd[:δ],Sd[:max_depth],Sd[:Δ_max],Sd[:ϵ])) <: NUTS
end

Output:

┌ Warning: `turnprogress(progress::Bool)` is deprecated, use `setprogress!(progress)` instead.
│   caller = top-level scope at nutsparam.jl:22
└ @ Core ~/code/scratch/local/turing/nutsparam.jl:22
[ Info: [Turing]: progress logging is disabled globally
[ Info: [AdvancedVI]: global PROGRESS is set as false
nutsparam: Error During Test at /home/me/code/scratch/local/turing/nutsparam.jl:27
  Test threw exception
  Expression: typeof(NUTS(Sd[:n_adapt], Sd[:δ], Sd[:max_depth], Sd[:Δ_max], Sd[:ϵ])) <: NUTS
  MethodError: Cannot `convert` an object of type Int64 to an object of type Symbol
  Closest candidates are:
    convert(::Type{S}, !Matched::T) where {S, T<:CategoricalValue} at /home/me/.julia/packages/CategoricalArrays/0ZAbp/src/value.jl:68
    convert(::Type{T}, !Matched::RCall.RObject{S}) where {T, S<:RCall.Sxp} at /home/me/.julia/packages/RCall/AEOQ7/src/convert/base.jl:1
    convert(::Type{T}, !Matched::T) where T at essentials.jl:171
    ...
  Stacktrace:
   [1] merge(::NamedTuple{,Tuple{}}, ::Tuple{Int64,Float64,Int64,Float64,Float64}) at ./namedtuple.jl:273
   [2] NUTS{Turing.Core.ForwardDiffAD{40},space,metricT} where metricT<:AdvancedHMC.AbstractMetric where space(::Int64, ::Vararg{Any,N} where N) at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:359
   [3] NUTS(::Int64, ::Vararg{Any,N} where N; kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{,Tuple{}}}) at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:313
   [4] NUTS(::Int64, ::Vararg{Any,N} where N) at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:313
   [5] top-level scope at /home/me/code/scratch/local/turing/nutsparam.jl:27
   [6] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
   [7] top-level scope at /home/me/code/scratch/local/turing/nutsparam.jl:26

Test Summary: | Pass  Error  Total
nutsparam     |    1      1      2
ERROR: Some tests did not pass: 1 passed, 0 failed, 1 errored, 0 broken.

[devmotion]

According to the documentation

the constructor takes two positional arguments (at most; the constructor without any arguments is listed as well) and all other arguments are passed as keyword arguments.

Does it work if you pass only n_adapts and δ as positional arguments?

[mkarikom]

Oops sorry, I misread that.
Switching to keyword args the following seems to have an issue
For clarity I added a third case where numbers are passed:

NUTS(100,0.65;max_depth=5,Δ_max=1000.0,ϵ=0.0)

MWE:

using Test
# Import Turing and Distributions.
using Turing, Distributions

# Import RDatasets.
using RDatasets

# Import MCMCChains, Plots, and StatsPlots for visualizations and diagnostics.
using MCMCChains, Plots, StatsPlots

# We need a logistic function, which is provided by StatsFuns.
using StatsFuns: logistic

# Functionality for splitting and normalizing the data
using MLDataUtils: shuffleobs, stratifiedobs, rescale!

# Set a seed for reproducibility.
using Random
Random.seed!(0);

# Turn off progress monitor.
Turing.turnprogress(false)

Sd = Dict(:iter=>200,:n_adapt=>100,:δ=>0.65,:max_depth=>5,:Δ_max=>1000.0,:ϵ=>0.0)
@testset "nutsparam" begin
    @test typeof(NUTS(Sd[:n_adapt],Sd[:δ])) <: NUTS
    @test typeof(NUTS(Sd[:n_adapt],Sd[:δ]; max_depth=Sd[:max_depth],Δ_max=Sd[:Δ_max],ϵ=Sd[:ϵ])) <: NUTS
    @test typeof(NUTS(100,0.65;max_depth=5,Δ_max=1000.0,ϵ=0.0)) <: NUTS
end

Output:

nutsparam: Error During Test at /home/me/code/scratch/local/turing/nutsparam.jl:28
  Test threw exception
  Expression: typeof(NUTS(100, 0.65; max_depth = 5, Δ_max = 1000.0, ϵ = 0.0)) <: NUTS
  MethodError: no method matching NUTS{Turing.Core.ForwardDiffAD{40},space,metricT} where metricT<:AdvancedHMC.AbstractMetric where space(::Int64, ::Float64; max_depth=5, Δ_max=1000.0, ϵ=0.0)
  Closest candidates are:
    NUTS{Turing.Core.ForwardDiffAD{40},space,metricT} where metricT<:AdvancedHMC.AbstractMetric where space(::Int64, ::Float64, !Matched::Int64, !Matched::Float64, !Matched::Float64, !Matched::Type{metricT}, !Matched::Tuple) where {AD, metricT} at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:315 got unsupported keyword arguments "max_depth", "Δ_max", "ϵ"
    NUTS{Turing.Core.ForwardDiffAD{40},space,metricT} where metricT<:AdvancedHMC.AbstractMetric where space(::Int64, ::Float64, !Matched::Tuple{}; kwargs...) where AD at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:327
    NUTS{Turing.Core.ForwardDiffAD{40},space,metricT} where metricT<:AdvancedHMC.AbstractMetric where space(::Int64, ::Float64, !Matched::Symbol...; max_depth, Δ_max, init_ϵ, metricT) where AD at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:336 got unsupported keyword argument "ϵ"
    ...
  Stacktrace:
   [1] kwerr(::NamedTuple{(:max_depth, :Δ_max, :ϵ),Tuple{Int64,Float64,Float64}}, ::Type{T} where T, ::Int64, ::Float64) at ./error.jl:157
   [2] NUTS(::Int64, ::Vararg{Any,N} where N; kwargs::Base.Iterators.Pairs{Symbol,Real,Tuple{Symbol,Symbol,Symbol},NamedTuple{(:max_depth, :Δ_max, :ϵ),Tuple{Int64,Float64,Float64}}}) at /home/me/.julia/packages/Turing/yvxUH/src/inference/hmc.jl:313
   [3] top-level scope at /home/me/code/scratch/local/turing/nutsparam.jl:28
   [4] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
   [5] top-level scope at /home/me/code/scratch/local/turing/nutsparam.jl:26

Test Summary: | Pass  Error  Total
nutsparam     |    1      2      3
ERROR: LoadError: Some tests did not pass: 1 passed, 0 failed, 2 errored, 0 broken.
in expression starting at /home/me/code/scratch/local/turing/nutsparam.jl:25

[devmotion]

It seems the keyword argument is called init_ϵ, not ϵ: https://github.com/TuringLang/Turing.jl/blob/beb440593b7a61f299a875abb84f3521fdbf3ada/src/inference/hmc.jl#L342

[mkarikom]

oh I see thanks, I have created a pr for this here

actually there is something strange about the following lines:

• the type declaration lists ϵ::Float64
• constructor 1 sets ϵ::Float64
• but 3 and 4 both set init_ϵ::Float64

Are these typos? If so I could prob do a pr, looks like all that might be missing is a unit test for constructor 1

[yebai]

Are these typos? If so I could prob do a pr, looks like all that might be missing is a unit test for constructor 1

It looks like a typo. @xukai92 can you clarify?

[xukai92]

I took a pass over the types and constructors and here's what I think now:

Although the variable names are different, I don't think there is any inconsistency ported to the user, since only init_ϵ is a keyword argument. So I suppose it is the documentation is misleading, or outdated after some refactoring being done? So the minimal thing we should do here is to fix the documentation.

With this being said, maybe we should have unified names to make things easier but here are some constraints

1. One reason ϵ is used internally is becasue this is a field all HMC types have, and for static HMC, using init_ϵ doesn't really make sense.
2. There is a keyword argument called init_theta to set the starting point, and init_ϵ was named to be consistent to it.

So it seems that there are two options we could do

1. Renaming the keryword argument init_ϵ to ϵ, in the same time also renaming keyword argument init_theta to theta.
2. Renaming the internal fileds name ϵ from init_ϵ for HMCDA and NUTS.

I personally perfor option 2, even though we might need to deal with alg.ϵ and alg.init_ϵ seperately internally.
I like it because there would be no interface change to users, and alg.init_ϵ is actually a more descriptive name for adaptive methods, as we actually comment in our type declarations (https://github.com/TuringLang/Turing.jl/blob/beb440593b7a61f299a875abb84f3521fdbf3ada/src/inference/hmc.jl#L246 and https://github.com/TuringLang/Turing.jl/blob/beb440593b7a61f299a875abb84f3521fdbf3ada/src/inference/hmc.jl#L310)

How do people think?

[devmotion]

IIRC DynamicPPL (and hence the sampling algorithms in Turing, but the main implementation is in DynamicPPL) uses init_params and not init_theta.

[xukai92]

I think we shall unify them.
Maybe supporting init_params in Turing first while keeping init_theta and throwing a deprecation message?

[devmotion]

We had a discussion a while ago in DynamicPPL when updating it to AbstractMCMC 2 that the init_params (or init_theta) keyword arguments and their implementation is a bit weird and maybe should be changed. The initial parameters correspond to an initial state of the sampler, and therefore maybe it would even make sense to add some standardized way for starting with an initial state to AbstractMCMC, similar to the iterator and transducer implementations. E.g., one could add an additional sample method with the state as an additional argument. This could be used for resuming sampling as well, which is currently implemented in Turing also in a bit special and slightly strange way.

[yebai]

therefore maybe it would even make sense to add some standardized way for starting with an initial state to AbstractMCMC

I agree - AbstractMCMC sounds like a natural place for such a mechanism of seeding initial MCMC states.