Open dustinvtran opened 9 years ago
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.
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.
Are you sure we can't break that refactoring into a lot of smaller pull requests?
Is this issue solved yet? I think so but I don't want to close it without confirmation.
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.)
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.
is michael's refactor comment (betanalpha commented on Jul 10, 2015) relevant?
or do we need to make some further change to write_iteration
?
This seems to be already done - lp is not written by ADVI (by my reading of the codebase) could @betanalpha verify and close?
Thanks, @martinmodrak --this kind of purposeful auditing of issues is really helpful.
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});
write_iteration_csv(...)
currently requires specifying the log probabilitylp
. 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-L317From stan-dev mailing list:
Example of printing
output.csv
with cmdstan's print script: