Keyword arguments

[cscherrer]

cc @sethaxen

[codecov]

Codecov Report

Merging #6 (7f85fa8) into master (649d93e) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   97.05%   97.22%   +0.16%     
==========================================
  Files           1        1              
  Lines          34       36       +2     
==========================================
+ Hits           33       35       +2     
  Misses          1        1              

Impacted Files	Coverage Δ
src/SampleChainsDynamicHMC.jl	97.22% <100.00%> (+0.16%)	⬆️

---

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 649d93e...7f85fa8. Read the comment docs.

[sethaxen]

My comments at this point are more high level, and they are opinionated. DynamicHMC is intended to serve as a sampling backend. As a result, it is highly customizable, but that also means that its API is meant to be more developer-friendly than user-friendly. While it is nice to have the customizability by essentially using DynamicHMC's API for configurable options, in my view it is better to identify the options users will most likely want to change and offer a convenient interface for changing those, while offering the rest of the options in a not-necessarily-as-user-friendly interface.

e.g. most users will not and should not change the warmup schedule, but they should change the total number of warmup draws, and the number of doublings should be set based on this number of draws. It is probably best that this package offer a warmup stage function that, if passed a number of warmup draws, auto chooses the warmup based on that.

Most users will want some feedback on how quickly their model is sampling. Defaulting to DynamicHMC.ProgressMeterReport() as the reporter seems then to be more useful. I've already checked that the progress meter works correctly on the REPL and jupyter notebooks (not on Pluto yet, it just goes from 0 to 100%, but it at least isn't a nuisance)
And perhaps reporter keyword that can take various Symbol values as aliases for DynamicHMC's reporters or the reporters themselves. e.g. reporter could take (nothing, :none, :logging, :progressmeter) in addition to DynamicHMC's types.

For init.q, would that be pre- or -post transform? That is, the user probably will want to provide an input in a NamedTuple with the parameter names they defined in their model, right?

In my view the most common warmup options users will want to (and should) change would be (roughly in decreasing order of priority):

• nwarmup: Int number of warmup draws (doublings chosen based on this)
• reporter: (:progressmeter, :logging, :none)
• adapt_delta: Real
• keep_warmup: Bool, warmup draws are returned with post-warmup draws
• metric: (:unit, :diag, or :dense)
• step_size_init: (::Union{Real,Nothing})
• init_point: NamedTuple of the type of an expected eltype of sample.

A simple interface for setting these directly without needing to know DynamicHMC's API and types would go a long way toward making it easy for the vast majority of users to do everything they need with this package.

What are your thoughts?

[cscherrer]

Thanks @sethaxen , this is a valuable discussion. Maybe it can help (or not hurt too much) to lay out some motivations that might be entirely at odds with each other. It's a sort of overdetermined system, and hopefully we can find the right metric for projecting onto the solution space. Sorry if this is stretching the analogy a little thin :)

1. It's good for commonly-used config parameters to be easily accessible by end-users
2. All available config parameters should be available without digging into the implementation, even if the code for uncommon cases might be a litttle longer
3. The DynamicHMC docs say, "The warmup interface below is not considered part of the exposed API, and may change with just minor version bump. It is intended for interactive use; the docstrings and the field names of results should be informative.", so there's some risk
4. AdvancedHMC implements something very similar. So if we deviate from the DynamicHMC interface, it would be good for the AdvancedHMC interface to be very similar (likely to be through SampleChainsAbstractMCMC).
5. In the long run, I'd like to support lots of different sampler back-ends, and keeping independent docs for each one will get to be a lot.

Based on all of this, I'm leaning toward something like

• Have a way to pass arguments through to DynamicHMC with as little processing as possible
• Possibly add some methods/keywords if very common config params are too awkward to access

[cscherrer]

I think you had suggested somewhere that if we allow keeping the warmup, we might have a Bool in the info indicating warmup vs "final sample". I like this. And a simple progress meter would be great, though I'm not sure how this works if we sample in parallel. But we can get to that later.

[sethaxen]

Okay, I think I see where you're coming from.

Based on all of this, I'm leaning toward something like

* Have a way to pass arguments through to DynamicHMC with as little processing as possible

Perhaps if we do this, it would be best to also re-export DynamicHMC from this package so that the user doesn't need to explicitly load DynamicHMC to access the API functions.

* Possibly add some methods/keywords if very common config params are too awkward to access

Yes, perhaps we can revisit this after using it for a while and maybe getting user input.

I think you had suggested somewhere that if we allow keeping the warmup, we might have a Bool in the info indicating warmup vs "final sample". I like this.

Yes, exactly. There could also be a "warmup_stage" field, since DynamicHMC gives us that information, but that seems unnecessary. Yet another alternative is having a warmup field in DynamicHMCChain where it can be stored, which avoids the user accidentally trying to draw inferences from warmup draws. I do think the simplest thing is prepending the wamup draws with the warmup draws and adding a flag to info.

And a simple progress meter would be great, though I'm not sure how this works if we sample in parallel. But we can get to that later.

ProgressMeter is friendly with Distributed and Threads, but we may need to create the progress bar outside of the threaded loop to have a single progress bar shared across threads (right now it seems to just show one bar at a time).

[sethaxen]

I made some suggestions, mostly documentation ones. In general this package also needs more tests. In particular now that the dynamicHMC function is correctly called and that user-specified parameters are effective, also that newchain creates a chain with the expected number of warmup stages and that sample! adds the expected number of samples.

[sethaxen]

What's the status of this PR?

[cscherrer]

Thanks for the ping @sethaxen . It looks like I had missed your earlier comments. Sorry about that, I'll have a look now. Also, it looks like we'll need to update for TransformVariables 0.4. Shouldn't be much to that, I think the biggest change is that transforms are no longer callable, but need to use transform

[sethaxen]

What do you think about doing @reexport using DynamicHMC?