Chains shouldn't (really) be indexed by Symbols

[penelopeysm]

I know this is a contrived example, but I don't think that this should be a problem if somebody wanted to do it:

julia> using Turing

julia> @model function f()
          var"x.a" ~ Normal()
          x = (a = 1.0,)
          x.a ~ Normal()
       end
f (generic function with 2 methods)

julia> sample(f(), MH(), 100)
Sampling 100%|███████████████████████████████████████████████████████████████████| Time: 0:00:02
Chains MCMC chain (100×3×1 Array{Float64, 3}):

Iterations        = 1:1:100
Number of chains  = 1
Samples per chain = 100
Wall duration     = 3.61 seconds
Compute duration  = 3.61 seconds
parameters        = x.a
internals         = lp
Error showing value of type Chains{Float64, AxisArrays.AxisArray{Float64, 3, Array{Float64, 3}, Tuple{AxisArrays.Axis{:iter, StepRange{Int64, Int64}}, AxisArrays.Axis{:var, Vector{Symbol}}, AxisArrays.Axis{:chain, UnitRange{Int64}}}}, Missing, @NamedTuple{parameters::Vector{Symbol}, internals::Vector{Symbol}}, @NamedTuple{varname_to_symbol::OrderedDict{AbstractPPL.VarName, Symbol}, start_time::Float64, stop_time::Float64}}:
ERROR: ArgumentError: index Symbol[] not found
[...]

IMO the use of Symbols in chains should be reserved for printing / indexing convenience. The underlying data type should, IMO, be a type parameter (in our case, VarName) that is convertable to a Symbol.

This would also eliminate the need for the varname to symbol dict that we stick inside our chains (to be precise, it would make it an inherent part of the Chains object, rather than it being manually tacked on as an extra).

[penelopeysm]

To be clear, I'm aware that both #469 and this issue are 'edge cases' that could be trivially avoided by renaming a model parameter, but I think that they both stem from an imperfect underlying data structure which could imo be revisited.

[penelopeysm]

After thinking about this for quite a while, this definitely feels like a Herculean task, because we use NamedTuples in so many parts of the ecosystem and we would have to (1) rework these to take OrderedDict{T}, and (2) set T to be VarName but only when sampling from DynamicPPL models..

That said - maybe it's worth doing (?). I suppose all of this would go into AbstractMCMC? We would thus have an interface that consists of

abstract type AbstractState end

abstract type AbstractNamedParamState end

getparams(::AbstractState) = (returns a Vector{<:Real})
getinfo(::AbstractState) = (returns a Dict{Symbol, Any}, and the sampler can chuck whatever info it wants inside)
getnamedparams(::AbstractNamedParamState) = (returns an OrderedDict of params => scalar values)

(Actually, on second thoughts, note that "instances" for these "typeclasses" can be derived from each other because getnamedparams(state) can be flattened to a vector, and a vector can be trivially converted to an OrderedDict where the keys are just parameters[i] for i in 1:length(getparams(state)))

The Chains parameter type would then be something like eltype(getparameters(state)).

Note that we intend to remove transitions from AbstractMCMC, which implies that MCMCChains should be constructed from a Vector{SamplerState}. It seems to me that getnamedparams and getinfo would provide enough functionality to do that.

The broad strategy would be this:

1. The individual package samplers e.g. AdvancedMH.MH have no guarantee on the model that they are given, so the state returned by that sampler (let's call it MHState) can only subtype AbstractState.
2. Can we force DynamicPPL.Sampler{AdvancedMH.MH} to return something that conforms to AbstractNamedParamState, say, a tuple of (VarInfo, MHState) from which we could implement getnamedparams (using e.g. values_as_in_model)? Note that Turing.externalsampler already does something very similar to this.