search

TuringLang / Turing.jl

Bayesian inference with probabilistic programming.
https://turinglang.org
MIT License
2.04k stars 219 forks source link

Do we need `resume_from` now that we have `initial_state`? #2171

Open torfjelde opened 9 months ago

torfjelde commented 9 months ago

AbstractMCMC@5 has initial_state as a mechanism to "resume" a chain.

This then begs the question: do we even need resume_from that is present in many sampler impls in Turing.jl?

https://github.com/TuringLang/Turing.jl/blob/39f5d5bb7fbf1607e073025a36688f1ed336e9d2/src/mcmc/hmc.jl#L92-L93

AFAIK the only place resume_from is actually overloaded is for Chains:

https://github.com/TuringLang/DynamicPPL.jl/blob/ff68206d4b34e230c7e444e91fc60297dd5a5bd0/ext/DynamicPPLMCMCChainsExt.jl#L18-L27

IMO we should replace this with a simple getstate or something from Chains and then just use initial_state everywhere to stay consistent with the AbstractMCMC interface.

torfjelde commented 9 months ago

@devmotion @yebai thoughts?

yebai commented 8 months ago

I don't think we need resume_from anymore.

devmotion commented 8 months ago

Yes, I think it's redundant and confusing, now that initial_state is an official AbstractMCMC keyword argument.

torfjelde commented 7 months ago

I see #2115 did something on this, but it doesn't seem to be "complete", e.g.

https://github.com/TuringLang/Turing.jl/blob/c29d36e20f1d61aed2bff118aa4c21694bc9c97c/src/mcmc/hmc.jl#L100-L115

is hit when resume_from === nothing which will be the case even if the user specifies initial_state.

penelopeysm commented 2 weeks ago

I was looking at the DynamicPPL codebase for unrelated reasons and thought to myself that this resume_from seemed very unnecessary.

I'll work on a PR to get rid of it.