TuringLang / AbstractMCMC.jl

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

Error if `init_params` has not been specified for all chains #125

Closed torfjelde closed 10 months ago

torfjelde commented 11 months ago

It seems we don't error correctly for certain multi-chain sample calls; in particular, Serial and MCMCDistributed seem to have this issue

https://github.com/TuringLang/AbstractMCMC.jl/blob/d7c549fe41a80c1f164423c7ac458425535f624b/src/sample.jl#L522-L526

https://github.com/TuringLang/AbstractMCMC.jl/blob/d7c549fe41a80c1f164423c7ac458425535f624b/src/sample.jl#L469-L473

while MCMCThreads does not, as it makes of _first_or_nothing:

https://github.com/TuringLang/AbstractMCMC.jl/blob/d7c549fe41a80c1f164423c7ac458425535f624b/src/sample.jl#L315C21-L316

https://github.com/TuringLang/AbstractMCMC.jl/blob/d7c549fe41a80c1f164423c7ac458425535f624b/src/sample.jl#L542-L549

Doesn't seem like there's a reason why we don't use _first_or_nothing for MCMCSerial and MCMCDistributed; guessing it's just an oversight?

Ref: https://github.com/TuringLang/Turing.jl/issues/2079

torfjelde commented 11 months ago

Note that the check in _first_or_nothing is not sufficient to catch scenarios where length(y) == n just happens to hold because the number of chains is the same as the dimension of the parameters :grimacing: But we can't really catch this here given that elements of init_params could be anything (even if init_params[1] is a Float64, it could be that the inner sample call expects Float64 as the init_params argument)

devmotion commented 11 months ago

Doesn't seem like there's a reason why we don't use _first_or_nothing for MCMCSerial and MCMCDistributed

Can you explain why we should use it? In those two cases we call map/pmap which require argument of matching length already, so it should not be possible to use init_params of incorrect length. We also can't call these functions with nothing, so we have to check for nothing explicitly anyway and then call the method without additional argument.

torfjelde commented 11 months ago

we call map/pmap which require argument of matching length already

It doesn't :grimacing: map will simply just use the shortest inputs:

julia> map(+, 1:2, 3:5)
2-element Vector{Int64}:
 4
 6

Alternatively we can use zip or something, which does require same length inputs

devmotion commented 11 months ago

Oops, I'm not sure why but I was convinced that map and pmap demand arguments of the same length but you're right. (BTW zip does also not require the arguments to be of the same length - I was aware of this but I always assumed this was an exception.)

torfjelde commented 11 months ago

(BTW zip does also not require the arguments to be of the same length - I was aware of this but I always assumed this was an exception.)

Oh true, haha. I had the exact opposite impression of what you did: I knew map didn't demand this, but I thought zip did :facepalm: