Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple callbacks #80

Open
torfjelde opened this issue Jul 1, 2021 · 4 comments
Open

Allow multiple callbacks #80

torfjelde opened this issue Jul 1, 2021 · 4 comments

Comments

@torfjelde
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

cpfiffer commented Jul 1, 2021

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.

@torfjelde torfjelde mentioned this issue Dec 5, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants