Closed cscherrer closed 3 years ago
Merging #6 (7f85fa8) into master (649d93e) will increase coverage by
0.16%
. The diff coverage is100.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%) |
:arrow_up: |
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.
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 drawsmetric
: (: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?
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 :)
Based on all of this, I'm leaning toward something like
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.
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 theinfo
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).
What's the status of this PR?
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
cc @sethaxen