Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement vb and adapt interface #74

Merged
merged 16 commits into from
Sep 29, 2020
Merged

Implement vb and adapt interface #74

merged 16 commits into from
Sep 29, 2020

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Sep 28, 2020

This PR combines updating the package interface (based on #73 and feedback from @pearsonca) with added support for variational inference (#72). It contains breaking changes that will impact all downstream users but substantially simplify the end-user experience whilst adding the ability for future extensions. See the updated readme and examples for new implementation details.

Changes

  • stan arguments now passed using stan_args with defaults from create_stan_args
  • generation times and incubation periods now stored in a generalised package data table
  • Added access functions for the generation time and incubation period to smooth access
  • Removed spurious arguments in examples
  • Removed testing arguments and embedded in stan_args as undocumented features (i.e stuck_chains).
  • Add support for variational bayes (method = "approximate").

Relevant to: @kathsherratt @pearsonca @joeHickson @jhellewell14

@seabbs
Copy link
Contributor Author

seabbs commented Sep 28, 2020

Example using variational inference with standard defaults (but using 10 trials in case of algorithm failure). It is likely that there are better defaults (i.e slower but more conservative) but no info on what these are at the moment.

Screenshot 2020-09-28 at 16 27 47

Via NUTs (run time 100 ~ 200 times longer).

Screenshot 2020-09-28 at 16 58 06

@joeHickson
Copy link
Collaborator

joeHickson commented Sep 28, 2020

I'm starting a review on this one but it seems to be a lot of tickets joined into one PR. @seabbs please can you try and keep them to separate branches for future changes? 48 files, +1018 lines, -101,232 lines is not a good sign for me being able to hold it all in my head (even if -100,001 is one file being removed) ;)

edit - I can see they may be intertwined so hard to split

@seabbs
Copy link
Contributor Author

seabbs commented Sep 28, 2020

Unfortunately, supporting VB required a large-scale interface change so this is a minimal PR.

@seabbs seabbs marked this pull request as ready for review September 28, 2020 16:45
@seabbs seabbs changed the title Implement vb and adapt interfacee Implement vb and adapt interface Sep 28, 2020
@seabbs seabbs added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 28, 2020
@seabbs seabbs self-assigned this Sep 28, 2020
@seabbs
Copy link
Contributor Author

seabbs commented Sep 29, 2020

Any issues with this @joeHickson or happy for me to merge in?

@joeHickson
Copy link
Collaborator

still reviewing!

@joeHickson
Copy link
Collaborator

(and yes, I have left comments but because it's mid-review I don't think it's exposed them to you yet)

Copy link
Collaborator

@joeHickson joeHickson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a chat about code styling standards on the call later - there's a few bits in here that change from one style to another with regards line breaks and it slows down the review a bit.

one or two minor bits and a couple of larger ones, notably:

The stan_args generation method is a bit wierd in the way some properties are native arguments and others are a list that is passed in (as stan_args), and then the calling places treat both the original stub list and the returning object as stan_args. I can see this is to do with how you pass the list of user defined options in to where it's generated. I have suggested some R6 objects but there are other ways around this one (including other structures of R6 objects), but if the current structure is kept some renaming should be done to clarify when the list is a stan_args stub and when it's a full stan_args list.

The use of magic strings is something that I spotted whilst reviewing - this task adds a few more. Suggestions as to how can be found on incubation-period.R but again there are multiple ways to crack this nut.

tests/testthat/test-regional_epinow.R Show resolved Hide resolved
tests/testthat/test-regional_epinow.R Show resolved Hide resolved
R/create.R Show resolved Hide resolved
R/estimate_infections.R Outdated Show resolved Hide resolved
NEWS.md Show resolved Hide resolved
R/estimate_infections.R Show resolved Hide resolved
R/estimate_infections.R Outdated Show resolved Hide resolved
data-raw/incubation-period.R Show resolved Hide resolved
R/get.R Show resolved Hide resolved
src/stanExports_estimate_infections.h Show resolved Hide resolved
@seabbs
Copy link
Contributor Author

seabbs commented Sep 29, 2020

Thanks for the review @joeHickson.

Addressed the minor issue with stan_args and opened a few issues based on review comments that I think warrant further discussion/there own PR. Happy to see an issue on the magic strings comment as well.

Merging this now as I think the main features are in place and will bring some solid user benefits.

@seabbs seabbs merged commit 6fca0c8 into master Sep 29, 2020
@seabbs seabbs deleted the implement_vb branch September 29, 2020 16:11
sbfnk pushed a commit that referenced this pull request May 3, 2024
Implement vb and adapt interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce arguments across functions Implement variational Bayes as a supported option.
3 participants