search

TuringLang / AbstractMCMC.jl

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

Allow multiple callbacks #80

Open torfjelde opened 3 years ago

torfjelde commented 3 years ago

It would be nice if it was possible to pass in multiple callbacks rather than a single callable. Of course this can always be done manually, but still would be convenient, e.g. sample(..., callback=(cb1, cb2)) or something.

Easiest solutions:

  1. Add a MultiCallback struct that just takes a bunch of functions and its call is to iterate over them.
  2. Add a check using hasmethod (not inferrable at compile-time unfortunately).
  3. ???
devmotion commented 3 years ago

Hmm since it is already possible to use arbitrary callbacks and one can always combine them manually, I'd say it's not necessary to support tuples or arrays of callbacks. I think it would make AbstractMCMC more complex without any clear benefit. As a side note, also e.g. Flux does not support multiple callbacks explicitly. Do you have a convincing example?

torfjelde commented 3 years ago

Hmm since it is already possible to use arbitrary callbacks and one can always combine them manually, I'd say it's not necessary to support tuples or arrays of callbacks. I think it would make AbstractMCMC more complex without any clear benefit.

Not even adding a MultiCallback struct which is like 5 lines of code? I opened this issue because this is what I did myself and I was like "well, maybe this would actually be convenient to have in AMCMC itself".

As a side note, also e.g. Flux does not support multiple callbacks explicitly. Do you have a convincing example?

I'm using a custom progress-meter through a callback + logging to TensorBoard using another callback, which means that I do use this pattern. I also want to add some more specific callbacks on top of this. We also have Turkie.jl which one might want to use together with some custom callback.

IMO finding motivating examples is trivial, but one can argue that it's so easy to write this pattern by hand that it's not worth it, sure. I do want to maybe point out that some people that use Turing aren't the most proficient in Julia, and so being able to do something like ..., callbacks=MultiCallback(TuringCallbacks.TensorBoardCallback(...), ProgressCallback()) will likely be to their benefit.

devmotion commented 3 years ago

Not even adding a MultiCallback struct which is like 5 lines of code? I opened this issue because this is what I did myself and I was like "well, maybe this would actually be convenient to have in AMCMC itself".

Yes, in particular not adding a specific struct. I don't think this belongs into AbstractMCMC since it is something that is not specific to MCMC and could be dealt with much more generally than with a specific MultiCallback struct:

(args...; kwargs...) -> foreach(f -> f(args...; kwargs...), fs)

This seems to be a much more general pattern that arguably better fits into a separate package (I wonder if it already exists somewhere since it does not seem to be a completely uncommon use case).

cpfiffer commented 3 years ago

I don't think this belongs into AbstractMCMC since it is something that is not specific to MCMC and could be dealt with much more generally than with a specific MultiCallback struct

I concur. Cool thing, not a great fit for AbstractMCMC -- definitely a good candidate for a downstream bolt-on package, though.