tpapp / DynamicHMC.jl

Implementation of robust dynamic Hamiltonian Monte Carlo methods (NUTS) in Julia.
Other
244 stars 21 forks source link

experimental iterator interface #94

Closed tpapp closed 5 years ago

tpapp commented 5 years ago

@cscherrer, please kindly review this and tell me if I can improve anything.

codecov[bot] commented 5 years ago

Codecov Report

Merging #94 into master will increase coverage by 0.18%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   89.62%   89.81%   +0.18%     
==========================================
  Files           8        8              
  Lines         376      383       +7     
==========================================
+ Hits          337      344       +7     
  Misses         39       39
Impacted Files Coverage Δ
src/mcmc.jl 95.5% <100%> (+0.38%) :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 22002b4...5f71e3b. Read the comment docs.

cscherrer commented 5 years ago

Nice! I really like that ℓq and ∇ℓq are available as well, think we can do some interesting things with that.

It would be great to have an iterate method as well. I think it would be something like

function iterate(steps::DynamicHMC.MCMCSteps, state=Q) 
    Q, tree_stats = mcmc_next_step(steps, Q)
    ((Q, tree_stats), Q)
end

But the initial Q will need to come from somewhere. Maybe a closure will get us there?

cscherrer commented 5 years ago

Got it working, using ResumableFunctions.jl: https://github.com/cscherrer/Soss.jl/blob/dev/src/dynamicHMC.jl

tpapp commented 5 years ago

Great to see that this is working for you.

I am reluctant to make an interface for iterate, since I don't really think of simulation as iteration (though I recognize the analogues).

Is there anything else you need here? Otherwise I would just merge this and tag a release.

cscherrer commented 5 years ago

Sounds good, thank you!