Remove usage of DynamicPPL.tonamedtuple

[torfjelde]

Now that we have DynamicPPL.varname_and_value_leaves we no longer need to make use of tonamedtuple.

EDIT: I should also point out that with this PR we can also properly put non-array types, e.g. Cholesky, into chains:

julia> using Turing

julia> @model demo_lkj() = x ~ LKJCholesky(2, 1.0)
demo_lkj (generic function with 2 methods)

julia> model = demo_lkj();

julia> chain = sample(model, Prior(), 10)
Sampling 100%|█████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:00
Chains MCMC chain (10×4×1 Array{Float64, 3}):

Iterations        = 1:1:10
Number of chains  = 1
Samples per chain = 10
Wall duration     = 0.02 seconds
Compute duration  = 0.02 seconds
parameters        = x[1,1], x[2,1], x[2,2]
internals         = lp

Summary Statistics
  parameters      mean       std      mcse   ess_bulk   ess_tail      rhat   ess_per_sec 
      Symbol   Float64   Float64   Float64    Float64    Float64   Float64       Float64 

      x[1,1]    1.0000    0.0000       NaN        NaN        NaN       NaN           NaN
      x[2,1]   -0.1403    0.6866    0.2171    10.0000    10.0000    0.9145      416.6667
      x[2,2]    0.7043    0.2582    0.0817    10.0000    10.0000    1.7894      416.6667

Quantiles
  parameters      2.5%     25.0%     50.0%     75.0%     97.5% 
      Symbol   Float64   Float64   Float64   Float64   Float64 

      x[1,1]    1.0000    1.0000    1.0000    1.0000    1.0000
      x[2,1]   -0.9725   -0.6705   -0.3638    0.5753    0.7537
      x[2,2]    0.2169    0.6330    0.7611    0.9191    0.9345


julia> chain = sample(model, NUTS(), 10)
┌ Info: Found initial step size
└   ϵ = 1.8
Sampling 100%|█████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:00
Chains MCMC chain (10×15×1 Array{Float64, 3}):

Iterations        = 6:1:15
Number of chains  = 1
Samples per chain = 10
Wall duration     = 0.98 seconds
Compute duration  = 0.98 seconds
parameters        = x[1,1], x[2,1], x[2,2]
internals         = lp, n_steps, is_accept, acceptance_rate, log_density, hamiltonian_energy, hamiltonian_energy_error, max_hamiltonian_energy_error, tree_depth, numerical_error, step_size, nom_step_size

Summary Statistics
  parameters      mean       std      mcse   ess_bulk   ess_tail      rhat   ess_per_sec 
      Symbol   Float64   Float64   Float64    Float64    Float64   Float64       Float64 

      x[1,1]    1.0000    0.0000       NaN        NaN        NaN       NaN           NaN
      x[2,1]    0.0275    0.5981    0.1912    10.0000     5.1546    1.4926       10.1523
      x[2,2]    0.8068    0.1712    0.0762     6.5463     5.1546    1.5393        6.6460

Quantiles
  parameters      2.5%     25.0%     50.0%     75.0%     97.5% 
      Symbol   Float64   Float64   Float64   Float64   Float64 

      x[1,1]    1.0000    1.0000    1.0000    1.0000    1.0000
      x[2,1]   -0.8471   -0.5180    0.3527    0.3527    0.6650
      x[2,2]    0.5314    0.7380    0.8597    0.9357    0.9834

and it's easy to add support for further types by overloading DynamicPPL.varname_and_value_leaves_inner 👍

[torfjelde]

Btw, @sethaxen do you have any thoughts about what we should return for the Cholesky factor? Currently we return C.UL but an potentially more natural alternative is maybe PDMat?

See https://github.com/TuringLang/DynamicPPL.jl/blob/ba16e3bc91e293c58b03ad287637472d6e11f52f/src/utils.jl#L1020-L1043 for where this is decided.

[sethaxen]

Btw, @sethaxen do you have any thoughts about what we should return for the Cholesky factor? Currently we return C.UL

How does the user encounter this return value? If it's just for storage in a Chains, then I tend to think that either one of the strict triangles of the correlation matrix or UL should be returned. I lean towards a strict triangle of the correlation matrix, since a Factorization in general behaves like the matrix it factorizes, not like its factors.

But if currently what is being returned is .UL, then I think it is misleading to label those entries as x[i, j], etc. I would definitely interpret those as entries of the correlation matrix, not of the factor. If you go that route, I recommend replacing x with x.U or x.L, whichever applies.

but an potentially more natural alternative is maybe PDMat?

Here's where my assumptions about flattening don't seem to follow. If the goal is flattening, then an iterator is returned, and I'm not sure how it would be relevant if the Cholesky was first wrapped as a PDMat before iterating.

[torfjelde]

How does the user encounter this return value?

Just in how it will be represented in the chain. Currently we just flatten x.UL, which means that you end up with x[1,1], x[1,2], and x[2,2] and their values in the case of x.UL being x.U, etc.

Here's where my assumptions about flattening don't seem to follow. If the goal is flattening, then an iterator is returned, and I'm not sure how it would be relevant if the Cholesky was first wrapped as a PDMat before iterating.

In the sense of, we return the flattened PDMat instead of x.UL i.e. the correlation matrix represented by this cholesky decomp flattened rather than the cholesky flattened.

EDIT:

I lean towards a strict triangle of the correlation matrix, since a Factorization in general behaves like the matrix it factorizes, not like its factors.

Btw, what do you mean when you say "strict triangle" here? You mean excluding the diagonal?

[torfjelde]

If you go that route, I recommend replacing x with x.U or x.L, whichever applies.

I like this suggestion, if we indeed go down that route:)

[sethaxen]

In the sense of, we return the flattened PDMat instead of x.UL i.e. the correlation matrix represented by this cholesky decomp flattened rather than the cholesky flattened.

Ah okay, then yes, this is also my preference. The only thing I don't like about it is that getindex isn't actually defined on the Cholesky object, so there's still room for confusion. The x.U or x.L approach doesn't have this problem. I leave the final decision up to you!

Btw, what do you mean when you say "strict triangle" here? You mean excluding the diagonal?

Yes, because, the diagonal entries are constrained to be 1s.

[torfjelde]

Yeah, unfortunately it's a bit "annoying" to show the strict triangle with the current implementation since it requires knowledge of the distribution (the current flattening-approach just takes into account the value itself, not the distribution, etc.), hence why I didn't do that.

I think for now, we do x.UL or PDMat(x); I'm leaning towards the x.UL to be completely transparent to the user.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (9423562) 0.00% compared to head (8dbd613) 0.00%.

❗ Current head 8dbd613 differs from pull request most recent head 4aeea42. Consider uploading reports for the commit 4aeea42 to get more accurate results

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #2071   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          22      21    -1     
  Lines        1492    1447   -45     
======================================
+ Misses       1492    1447   -45     

Files Changed	Coverage Δ
ext/TuringDynamicHMCExt.jl	0.00% <0.00%> (ø)
ext/TuringOptimExt.jl	0.00% <0.00%> (ø)
src/Turing.jl	0.00% <ø> (ø)
src/contrib/inference/abstractmcmc.jl	0.00% <0.00%> (ø)
src/contrib/inference/sghmc.jl	0.00% <0.00%> (ø)
src/essential/ad.jl	0.00% <ø> (ø)
src/inference/AdvancedSMC.jl	0.00% <0.00%> (ø)
src/inference/Inference.jl	0.00% <0.00%> (ø)
src/inference/emcee.jl	0.00% <0.00%> (ø)
src/inference/ess.jl	0.00% <0.00%> (ø)
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[torfjelde]

This finally looks ready to be reviewed @yebai ! This is technically a non-breaking release given how we don't export this functionality, but I'm in favour of making it breaking as there are bound to be some downstream issues with this.

[yebai]

Thanks @torfjelde , it looks good to me!