TuringLang / AbstractMCMC.jl

Abstract types and interfaces for Markov chain Monte Carlo methods
https://turinglang.org/AbstractMCMC.jl
MIT License
79 stars 18 forks source link

Provide generic implementation of `bundle_samples` for `Vector{T}` #46

Closed devmotion closed 4 years ago

devmotion commented 4 years ago

While updating AdvancedMH I noticed that we define a bunch of bundle_samples implementations there. I was wondering if we could generalize that in a way such that we don't have to repeat it in every package. In particular, it seems a bit strange to reimplement it in every package for Vector{NamedTuple} even though the only difference is how the samples are converted to a NamedTuple. But one could, e.g., also have a generic version for MCMCChains.Chains in MCMCChains with the basic structure + an interface for other packages to convert the samples, retrieve the names, the log-evidence + save the state, sampler, and model in a standardized way (and not different in every package).

This PR contains a very generic implementation for any desired vector-valued output.

devmotion commented 4 years ago

However, for AdvancedMH this would mean that the keyword argument param_names wouldn't be available anymore for Vector{NamedTuple} output. I'm wondering if that would be an issue? I guess users could still change the names afterwards (even for Chains) but requires an additional step?

devmotion commented 4 years ago

Ah wait, I guess one could just define convert(::Type{NamedTuple{names}}, ::Transition) in NamedTuples and then use Vector{NamedTuple{names}} as output type :+1:

Edit: But then one would have to include :lp in the names...

Edit 2: Could of course always assume that names does not include :lp - which is weird though since then the output wouldn't actually be of the stated type.

codecov[bot] commented 4 years ago

Codecov Report

Merging #46 into master will increase coverage by 0.03%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   97.61%   97.65%   +0.03%     
==========================================
  Files           6        6              
  Lines         126      128       +2     
==========================================
+ Hits          123      125       +2     
  Misses          3        3              
Impacted Files Coverage Δ
src/interface.jl 92.85% <100.00%> (+1.19%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 56a3b78...44c1725. Read the comment docs.

yebai commented 4 years ago

@devmotion I fixed a merge conflict. Not sure it is what you intended to do. Pls, take a look anyway.

devmotion commented 4 years ago

Thanks, looks good to me!