Include state and kwargs... to callback

[theogf]

Following #55 , state and kwargs... are passed to the callback function.

[codecov]

Codecov Report

Merging #56 (a2e51d8) into master (3263028) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   98.42%   98.39%   -0.04%     
==========================================
  Files           6        6              
  Lines         191      187       -4     
==========================================
- Hits          188      184       -4     
  Misses          3        3              

Impacted Files	Coverage Δ
src/sample.jl	99.21% <100.00%> (ø)
src/logging.jl	94.11% <0.00%> (-0.89%)	⬇️
src/AbstractMCMC.jl	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3263028...a2e51d8. Read the comment docs.

[devmotion]

I assume that state could be useful potentially, even though I haven't encountered an example myself and it is a sampler-specific internal object. I assume that it was just not included since there has not been a use case and in contrast to the samples it is not user-facing and its structure can be completely arbitrary.

I am a bit more reluctant with the keyword arguments though. They were not included on purpose and I think there are good reasons for keeping it this way. If we include them here, then every callback has to handle keyword arguments (most by just discarding kwargs..., I assume). It seems most (all?) cases could be handled by including them in the callback on the call site instead, so I am not sure if the increased flexibility here outweighs the fact that every callback implementation has to account for keyword arguments. Do you have any concrete application in mind that can't be handled with the current setup?

[theogf]

About state

Ok so here is my example which might makes things more clear.
I work on https://github.com/theogf/BayesianQuadrature.jl, which basically does things in two steps:

• Draw samples (this uses the AbstractMCMC interface)
• Compute the quadrature

Now of course it would be nice to have a callback during the sampling to estimate the quadrature after every draw. Except that one needs the whole collection of samples to do that (this actually makes me realize that passing samples would be more interesting than passing sample).
I did not think it through completely and added state because in some of my samplers, the collection of samples is anyway saved in state.

About the keyword argument:

You can indeed have something like:

my_array = []
my_callback(args...) = push!(my_array, i)

but if you don't work inside a closure it means you need to work with global variables, having a simple alternative would be nice.

outweighs the fact that every callback implementation has to account for keyword arguments

I don't buy this argument since you need to impose a structure for the callback function anyway...
One needs to (currently) write callback as callback(rng, model, sampler, sample, i) but with kwargs... the constraint is just a different one.

[devmotion]

Now of course it would be nice to have a callback during the sampling to estimate the quadrature after every draw.

Maybe it would make sense to use the iteration (or transducer) interface in this case. Of course, some of the defaults such as progress bars etc. would be lost (I don't know how important they are for you) but you would have direct access to all samples and could easily collect them, perform additional computations (also with the collection of samples), apply "callbacks" only in every n-th step etc.

but if you don't work inside a closure it means you need to work with global variables, having a simple alternative would be nice.

Hmm I'm not sure what exactly you mean - are you referring to the fact that one should avoid to clse over non-constant global variables? In this case one can apply e.g. the let block trick.

I don't buy this argument since you need to impose a structure for the callback function anyway...

True, it's just a slightly more complex structure. But why make it more complex if it is not needed? In other places we actually reduced the number of positional and keyword arguments since they were not used and made the interface more complex. IMO it is good to keep the interface as simple as possible (but not simpler). Though maybe this does not apply to keyword arguments to the same extent as positional arguments since they can be dropped by developers quite easily in the implementation with the catch-all kwargs... whereas positional arguments have to be specified.

[cpfiffer]

I think it kind of depends on how powerful we want the callbacks to be. The current format is rigorous and simple, but not as extensible as if keyword arguments were included.

I'm on the side of adding keyword arguments mostly because devs are going to have to make a change anyway to accommodate the state argument and can add ; kwargs... to their callbacks quite easily.

That said, I'm sympathetic to the argument that that is kind of an annoying thing to do if you are writing a quick-and-dirty callback function. Maybe we can catch this internally and allow both forms. Is there a robust way to check if the signature of the callback accepts keyword arguments, other than try/catch?

[theogf]

I find the callback problem generally so annoying!
That would be nice to have a standard way to do this with the least restrictions for both the devs and the users. Is there something like this already?

I was thinking: how about passing everything via keywords arguments?
Like any callback function should be of the form my_callback(;kwargs...) which can then be refined my_callback(;model, kwargs...) etc and internally the callback function is always callback(;model=model, state=state, i=i, etc..., kwargs...)
Is there anything wrong with this approach?

[EDIT cause writing faster than thinking] : This would not allow any multiple dispatch for the callback functions but this could be eventually done internally: my_callback(;kwargs...) = my_specialized_callback(kwargs.model)

[torfjelde]

Just to rejuvenate this discussion: I also want kwargs 🙃

Though maybe this does not apply to keyword arguments to the same extent as positional arguments since they can be dropped by developers quite easily in the implementation with the catch-all kwargs... whereas positional arguments have to be specified.

And this is why I'm pro kwargs despite slight increase in complexity. Current moving AHMC over to AMCMC interface, and I think we want to continue using the process tracking that already exists in AHMC (due to additional information provided). AFAIK it's easiest to implement this is as a callback, BUT the issue is that I want to know stuff like nadapts, if the user specified verbose, etc. so that I can inform the user that "hey, adaptation just completed!" in the callback.

Of course, all of this is easy to implement using the iterator or transducer interface, but IMO it's somewhat breaking with "user expectation": I very much expected kwargs... to be available (especially given that's being passed around to everything else in sample 😅 ).

EDIT: Ughh, I also need state 😞

[cpfiffer]

Okay, I think I'm perfectly happy to add keyword arguments.

EDIT: Ughh, I also need state disappointed

Yeah, I think this is a non-negotiable add -- state definitely needs to be passed on.

[cpfiffer]

@theogf could you bump the version to 3.0, since this will be very breaking?

[cpfiffer]

Nevermind, I changed the version myself.