comments on feature/bbvb branch

[Daniel Lee]

Alp, this email is for you. GitHub's interface won't let me annotate
directly on a branch, so I'm just going to dump it as an email.

1. why have README.txt and TO-DO.txt been removed? Revert commit hash
a3012ca for those files. (if you want to remove them or update them,
put them on a separate branch)

2. src/stan/vb/bbvb.hpp: line 69: elbo_n_monte_carlo shouldn't be set
to 100, but rather to the number in the member variable
elbo_n_monte_carlo_.

3. src/stan/vb/bbvb.hpp: line 136 -- FIXME. Yes, that's correct.

4. src/stan/services/command.hpp: starting at line 662. Instead of the
whole thing inside command, we should break this out into a function.
So, it'll look like:
if (parser.arg("method")->arg("variational")) {
variational_inference(...);
}
[note: we haven't done that with command.hpp for the rest, but we need
to. I'd rather not add 100 lines to command.hpp without an ability to
test the entry points independently.]

5. the test models should move. I'll help with that.

6. take out the direct uses of std::cout. Use a pointer to the stream instead.

7. bbvb: calc_ELBO could probably be written as a signal function
instead of two.

8. you'll need to add latent_vars.hpp. It's not in the repo.

9. I think you'll want to add some more tests for the entry points.
I'm thinking along the lines of tests for
- bbvb: calc_ELBO
- bbvb: calc_combined_grad
- bbvb: do_robins_monro_adagrad
- bbvb: run_fullrank
- bbvb: run_meanfield

maybe a handful of tests for vb param classes -- just to make sure it
can't be created with bad values.


It looks pretty good, though. We'll talk Monday.


Daniel

[Michael Betancourt]

4. src/stan/services/command.hpp: starting at line 662. Instead of the
whole thing inside command, we should break this out into a function.
So, it'll look like:
   if (parser.arg("method")->arg("variational")) {
      variational_inference(...);
   }
[note: we haven't done that with command.hpp for the rest, but we need
to. I'd rather not add 100 lines to command.hpp without an ability to
test the entry points independently.]

These are getting split off in the current Writer refactor,

although it turns out the refactor is going to be much 

more extensive than thought.

6. take out the direct uses of std::cout. Use a pointer to the stream instead.

This will have to be updated to use the Writers anyways,

so I wouldn’t worry about it too much.

[Alp Kucukelbir]

Thanks daniel!

> 1. why have README.txt and TO-DO.txt been removed? Revert commit hash
> a3012ca for those files. (if you want to remove them or update them,
> put them on a separate branch)

i have no idea. sorry about that. i've added them back. how do i use
these files? are they actively used within the dev team?

> 2. src/stan/vb/bbvb.hpp: line 69: elbo_n_monte_carlo shouldn't be set
> to 100, but rather to the number in the member variable
> elbo_n_monte_carlo_.

i made the modification. the user can now change it. (but it's not
recommended... i don't know how to make that clear in the arguments.)

> 3. src/stan/vb/bbvb.hpp: line 136 -- FIXME. Yes, that's correct.

great. FIXME comments removed.

> 4. src/stan/services/command.hpp: starting at line 662. Instead of the
> whole thing inside command, we should break this out into a function.
> So, it'll look like:
> if (parser.arg("method")->arg("variational")) {
> variational_inference(...);
> }
> [note: we haven't done that with command.hpp for the rest, but we need
> to. I'd rather not add 100 lines to command.hpp without an ability to
> test the entry points independently.]

ok, let's discuss this. michael mentioned something below, so i think i'm
a bit out of the loop on this end. i'm happy to adapt.

> 5. the test models should move. I'll help with that.

great! thanks.

> 6. take out the direct uses of std::cout. Use a pointer to the stream instead.

yeah... those are temporary. i didn't know which stream made the most sense
for us.

> 7. bbvb: calc_ELBO could probably be written as a signal function
> instead of two.

the entropy calculation at the end is a bit different. i guess i could
push that into the params classes and then template the calc_ELBO function.
i'll look into it.

> 8. you'll need to add latent_vars.hpp. It's not in the repo.

damn. `latent_vars.hpp` became `vb_params_fullrank.hpp` . that points to
how outdated my tests are. i need to rewrite the tests to point to
`vb_params_fullrank.hpp`. i also need to write new tests for
`vb_params_meanfield.hpp`.

> 9. I think you'll want to add some more tests for the entry points.
> I'm thinking along the lines of tests for
> - bbvb: calc_ELBO
> - bbvb: calc_combined_grad
> - bbvb: do_robins_monro_adagrad
> - bbvb: run_fullrank
> - bbvb: run_meanfield
>
> maybe a handful of tests for vb param classes -- just to make sure it
> can't be created with bad values.

i'm on board. i have ELBO tests already in the outdated files. perhaps they
should be separated into a different file?

the trick with writing a test for the actual algorithm is that it's stochastic.
so we'd have to really make sure that the tolerance we set for the tests
don't occasionally trip jenkins/etc just due to randomness.

> It looks pretty good, though. We'll talk Monday.

looking forwrd. thanks again!

cheers
alp

[Bob Carpenter]

Alp --- please don't remove them in your branch. I'll
create an issue to deal with them.

- Bob

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Daniel Lee]

Thanks, Bob. I'll figure out a way to deal with the conflicts.