search

alashworth / test-issue-import

0 stars 0 forks source link

Refactor write_iteration* functions #71

Open alashworth opened 5 years ago

alashworth commented 5 years ago

Issue by dustinvtran Friday Jul 10, 2015 at 20:43 GMT Originally opened as https://github.com/stan-dev/stan/issues/1538

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,...
alashworth commented 5 years ago

Comment by syclik Friday Jul 10, 2015 at 20:56 GMT

Thanks for hunting this down. We should definitely generalize our functions. On Jul 10, 2015 4:43 PM, "Dustin Tran" notifications@github.com 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.

alashworth commented 5 years ago

Comment by betanalpha Friday Jul 10, 2015 at 20:58 GMT

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

On Jul 10, 2015, at 10:56 PM, Daniel Lee notifications@github.com wrote:

Thanks for hunting this down. We should definitely generalize our functions. On Jul 10, 2015 4:43 PM, "Dustin Tran" notifications@github.com 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.

alashworth commented 5 years ago

Comment by syclik Saturday Jul 11, 2015 at 15:09 GMT

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

alashworth commented 5 years ago

Comment by dustinvtran Sunday Feb 28, 2016 at 23:12 GMT

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

alashworth commented 5 years ago

Comment by syclik Monday Feb 29, 2016 at 02:45 GMT

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.)

alashworth commented 5 years ago

Comment by dustinvtran Monday Feb 29, 2016 at 02:48 GMT

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.

alashworth commented 5 years ago

Comment by akucukelbir Friday Mar 04, 2016 at 13:09 GMT

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

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

alashworth commented 5 years ago

Comment by martinmodrak Monday Jun 04, 2018 at 09:10 GMT

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

alashworth commented 5 years ago

Comment by bob-carpenter Tuesday Jun 05, 2018 at 03:08 GMT

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