Refactor write_iteration* functions

[dustinvtran]

write_iteration_csv(...) currently requires specifying the log probability lp. This is unneeded for variational inference, and as a hack currently writes 0.0 there: https://github.com/stan-dev/stan/blob/develop/src/stan/variational/advi.hpp#L315-L317

From stan-dev mailing list:

Let's look into refactoring that into less of a hack.  We shouldn't be requiring
lp__ in anything downstream.   Feel free to bring these things up during
design, and we can look into redoing them as we go so we don't wind up with
a hack release that we want to fix later by getting rid of lp__.

- Bob

Example of printing output.csv with cmdstan's print script:

# stan_version_major = 2
# stan_version_minor = 6
# stan_version_patch = 3
# model = bernoulli_model
# method = experimental
#   experimental
#     variational
#       algorithm = meanfield (Default)
# meanfield
#       iter = 10000 (Default)
#       grad_samples = 1 (Default)
#       elbo_samples = 100 (Default)
#       eta_adagrad = 0.10000000000000001 (Default)
#       tol_rel_obj = 0.01 (Default)
#       eval_elbo = 100 (Default)
#       output_samples = 1000 (Default)
# id = 0 (Default)
# data
#   file = bernoulli.data.R
# init = 2 (Default)
# random
#   seed = 3959575278
# output
#   file = output.csv (Default)
#   diagnostic_file =  (Default)
#   refresh = 100 (Default)
lp,theta
0,0.221171
0,0.166458
0,0.297987
0,...
0,...
0,...

[syclik]

Thanks for hunting this down. We should definitely generalize our functions. On Jul 10, 2015 4:43 PM, "Dustin Tran" [email protected] wrote:

write_iteration_csv(...) https://github.com/stan-dev/stan/blob/develop/src/stan/services/io/write_iteration_csv.hpp currently requires specifying the log probability lp. This is unneeded for variational inference, and as a hack currently writes 0.0 there: https://github.com/stan-dev/stan/blob/develop/src/stan/variational/advi.hpp#L315-L317

From stan-users mailing list https://groups.google.com/forum/#!topic/stan-dev/Bo5fWHI12iA:

Let's look into refactoring that into less of a hack. We shouldn't be requiring lp__ in anything downstream. Feel free to bring these things up during design, and we can look into redoing them as we go so we don't wind up with a hack release that we want to fix later by getting rid of lp__.

• Bob

— Reply to this email directly or view it on GitHub https://github.com/stan-dev/stan/issues/1538.

[betanalpha]

Cough, cough — already done in the writer refactor — cough, cough

On Jul 10, 2015, at 10:56 PM, Daniel Lee [email protected] wrote:

Thanks for hunting this down. We should definitely generalize our functions. On Jul 10, 2015 4:43 PM, "Dustin Tran" [email protected] wrote:

write_iteration_csv(...) https://github.com/stan-dev/stan/blob/develop/src/stan/services/io/write_iteration_csv.hpp currently requires specifying the log probability lp. This is unneeded for variational inference, and as a hack currently writes 0.0 there: https://github.com/stan-dev/stan/blob/develop/src/stan/variational/advi.hpp#L315-L317

From stan-users mailing list https://groups.google.com/forum/#!topic/stan-dev/Bo5fWHI12iA:

Let's look into refactoring that into less of a hack. We shouldn't be requiring lp__ in anything downstream. Feel free to bring these things up during design, and we can look into redoing them as we go so we don't wind up with a hack release that we want to fix later by getting rid of lp__.

• Bob

— Reply to this email directly or view it on GitHub https://github.com/stan-dev/stan/issues/1538.

— Reply to this email directly or view it on GitHub.

[syclik]

Are you sure we can't break that refactoring into a lot of smaller pull requests?

[dustinvtran]

Is this issue solved yet? I think so but I don't want to close it without confirmation.

[syclik]

I don't think so... it's not updated on develop, right? (Normally, I'd check first, but hopefully it's easy for you to confirm.)

[dustinvtran]

Oh, you're right. I mistook my above issue with something else about streams. Yes we still have to do a hack by inputting 0 for the log prob's.

[akucukelbir]

is michael's refactor comment (betanalpha commented on Jul 10, 2015) relevant?

or do we need to make some further change to write_iteration?

[martinmodrak]

This seems to be already done - lp is not written by ADVI (by my reading of the codebase) could @betanalpha verify and close?

[bob-carpenter]

Thanks, @martinmodrak --this kind of purposeful auditing of issues is really helpful.

[mcol]

I think it's not done, as src/stan/variational/advi.hpp still contains these lines:

// Write lp__, log_p, and log_g.
values.insert(values.begin(), {0, log_p, log_g});