Added argument to initialize Euclidean metric for samplers#576
Added argument to initialize Euclidean metric for samplers#576mitzimorris merged 5 commits intostan-dev:developfrom
Conversation
|
the challenge in adding this, as you've discovered - is the way that the command.hpp code sets up the cascade of dependent arguments. if you look at the current cmdstan manual (which needs to updated as part of the PR), the set of arguments for which passing in a metric file would be valid are the ones for "sample algorithm = hmc" given this, you could add argument "metric_file" as a subargument to argument "hmc" - you should be able to check that when the metric is "diag_e" the metric file is diagonal, and when the metric is "dense_e" the metric file is dense, and then you should be able to make this feature more general. |
Hmm, that does make sense.
Will do. Was just being lazy. |
|
This is ready for review |
|
I read through this but I'd like to do one more pass before calling it
good. I'll do that tomorrow.
…On Sat, Oct 21, 2017, 11:39 AM Ben Bales ***@***.***> wrote:
This is ready for review
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#576 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAfA6auNDxq8fOFs7Y2tt0xpVi4kZCETks5suhAhgaJpZM4P5jhC>
.
|
sakrejda
left a comment
There was a problem hiding this comment.
Only comment I had was maybe say in the doc that metric is used as a starting point for adaptation when both are specified (assuming this is what happens?). Looks good otherwise.
src/docs/cmdstan-guide/running.tex
Outdated
| metric, \code{inv\_metric} should be a positive-definite square matrix with | ||
| number of rows and columns equal to the number of parameters in the model. | ||
| The file pointed at by \code{metric\_file} should have the same format as | ||
| the input data. This option can be used with and without adaptation enabled. |
There was a problem hiding this comment.
It would be helpful to add what happens if it's used with adaptation enabled.
Good point. I ran methods with adaptation and they didn't fail, but I need to make sure the adaptation isn't simply ignoring the provided initial matrix and re-adapting a new one. I'll check that and update the doc to make it clear. |
|
now that we've got this all plumbed through, I'm still trying to make sense of the use cases for this feature:
running tests for case (2), I'm not sure adaptation is respecting stepsize. here's my test metric file - same as used for stan unit test: using model trying this command sequence: I set the stepsize smaller and smaller - if I run this with just one iteration, stepsize changes alot - should it? |
|
regarding my previous comment, fix for problems w/r/t stepsize and adaptation should go in at the stan services level - not a cmdstan problem. |
|
Use case (1) and (2), definitely. A third case is external algorithms---Aki needs that for something, I believe. |
|
Use case 3: 1) Fit the model to data A; 2) pretend like fitting the model to data A + B where B is much much smaller won't change posterior geometry much; 3) fit the model to data A + B without adaptation in parallel since now there's no adaptation to worry about (you could still do multiple chains with different starting points). |
|
w/r/t testing - do we have tests for use cases 2 and 3? |
|
Shouldn't tests be in stan::services? Or do you mean in general showing that they are worthwhile use cases? |
|
yes, tests should be in stan::services. |
I checked this. The way adaptation works, the provided metric is tossed if adaptation is enabled. So the way I have the docs written now is wrong. If someone provides a metric, adaptation of that metric should be disabled (otherwise it's misleading). I can make these options mutually exclusive. Is there a way to separately turn off timestep and metric adaptation? For my use case for this I was hoping I could just leave the timestep adaptation to Stan (and just provide the metric). Looking at the code it seems like either they both happen or neither happen: https://github.com/stan-dev/stan/blob/develop/src/stan/mcmc/hmc/nuts/adapt_diag_e_nuts.hpp#L31 Should we add this in? Or make it so that if someone provides a metric, they're liable for the timestep as well? Either way, don't merge this pull :P. |
|
hmm - if that's what the code is doing, that contradicts what Michael said here:
(https://groups.google.com/forum/#!topic/stan-users/O-PNZhzVjTI) |
|
OTOH, the current Stan manual says this:
|
|
@bbbales2 - you're misreading the code -
the variable named the stepsize can be set at initialization: |
|
this PR looks good, and I believe the stan code for the samplers will do the right thing. however, it would be good for the docs to spell out the 3 use cases and appropriate config: |
|
This is correct — unless you set adapt engaged=0 the step size and metric will
used only to initialize the adaptation routine. If they are good values then they
should not change much. This is the desired behavior and especially useful
if people want to use this feature to set informed guesses for the inv_metric
on new runs, as opposed to just restarting.
… On Oct 24, 2017, at 7:14 PM, Mitzi Morris ***@***.***> wrote:
this PR looks good, and I believe the stan code for the samplers will do the right thing. however, it would be good for the docs to spell out the 3 use cases and appropriate config:
(1) specify stepsize, metric, set "adapt engaged=0". the appropriate "num_samples" is determined by desired precision of your quantity of interest which in turn depends on "N_eff" for that QoI.
(2) specify stepsize, metric and any other non-default settings used in initial run
(3) depends on external algorithm
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#576 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdNlifo6AHbVAxu2Y9UpijS_FaTNbVSks5svm9LgaJpZM4P5jhC>.
|
…ation and specifying a metric_file
|
doc looks great! very clear. |
|
Np thanks for the review |
Submisison Checklist
./runCmdStanTests.py src/testSummary:
This is the CmdStan interface component of: stan-dev/stan#2260
This should allow initialization of Euclidean metric for samplers with an R dump file
Questions:
Where should the metric_file argument go? I think it makes sense under "init" but I didn't put it there cause that would mean breaking a bunch of other stuff (that depends on passing in init=whatever arguments). I just made it a high level argument for now.
I only added check-that-it's-working tests. Lemme know if I should add check-if-it's-failing tests. I figured the Stan tests should handle most of that though (I didn't really add any functionality beyond the interface)
Should the unit_e samplers allow initialization of their Euclidean metrics? The interface isn't there for them.
Nomenclature-wise, are we setting the metric? Or are we initializing the Euclidean metric? Or are we setting a Euclidean metric? Or what is the verbage equivalent to "setting the mass matrix".
How to Verify:
./runCmdStanTests.py src/test/interface/metric_test.cpp
Side Effects:
Documentation:
I still need to do it
Reviewer Suggestions:
@mitzimorris or @sakrejda, whoever feels inclined
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): University of California, Santa Barbara
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: