Update convergence sampling signature to use `state`

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

Update convergence sampling signature to use `state` #67

Closed mileslucas closed 3 years ago

mileslucas commented 3 years ago

https://github.com/TuringLang/AbstractMCMC.jl/blob/d961513b0ad3dc53d414da6581b332b66c4d0ef3/src/sample.jl#L209

I think this should be updated to be

isdone(rng, model, sampler, samples, state, i; progress=progress, kwargs...)

to coincide with the immutable samplers (#42, #56), which would be breaking (unfortunately).

Thoughts on this? I'm happy to PR, just let me know if this has downstream effects that I'll need to address, too.

devmotion commented 3 years ago

I would be fine with adding it, it seems reasonable (even though I don't think that the linked PRs imply that one has to do it). As long as we follow semver and make a breaking release we should not worry about downstream packages here I think (additionally, I believe it's not used in many downstream packages).