-
Notifications
You must be signed in to change notification settings - Fork 33
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 dist_spec
interface
#363
Conversation
Remaining error is the one that would be fixed by #366 but this didn't occur before so it may point to an inadverted change to the model. |
Now all the explained things are resolved I'll have a proper look through + see if I can identify what is leading to the change in behaviour. |
Should be all fixed now. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0043b32 is merged into develop:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f6a0334 is merged into develop:
|
Looks like performance is worsening with this - will need to investigate. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f6a0334 is merged into develop:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f6a0334 is merged into develop:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f6a0334 is merged into develop:
|
I like
Yes I think this is fine to add but it needs to be very clearly communicated that this change has happened or it will break lots of code. As I have said many times I think the default should be to use the actual parameters of the distribution as the default as this will limit the information loss in a standard modelling pipeline. This should also be the case for the gamma distribution as well really and perhaps we can make that transition in this release as well.
This is all nice. I need to go through the code in detail and kick some tires (so far I have just sight read) but in principle I am a big fan of the proposed changes. As I mention in the comments below we could go further in unifying distributions across delays, generation times, and truncations etc and if we did this now it could save us pain in the future. I also really think we want to be combining fixed PMFs in R vs in stan now we have code to generate them as this will reduce model complexity and make it easier to test everything is working as expected (is this already happening? I think not but may have missed). |
ea44603
to
254d9b4
Compare
@sbfnk waiting on you now I think. I have had to crank adapt delta on some of the unit tests which isn't a great sign in terms of numerical stability. Did you see that in your testing? |
Not really - does this cause an error or just a warning somewhere? I've looked through this again and it looks good to me, apart of course from the performance hit. |
I agree it's not ideal - though I think could also be down to the specific scenario I've used for testing this. Definitely worth exploring further. |
Co-authored-by: Sebastian Funk <[email protected]>
Co-authored-by: Sebastian Funk <[email protected]>
Co-authored-by: Sebastian Funk <[email protected]>
Co-authored-by: Sebastian Funk <[email protected]>
The issue I was seeing is failure to recover the mean which appeared to be due to low effective sample size. Right I think this is ready to merge if we are happy with the potential performance impact (which I am - hoping it gets resolved). |
LGTM - merging! |
* implement `dist_spec` interface * Add skipping of stan tests in the expected places * fix checks * fix epinow example * update syntax in more places * update another example * move parenthesis to the right place * update uncertainty in estimate_infections example * fix use of generation_time_opts as resource to call get_generation_time() * break line to make R CMD CHECK happy * fix uses of `trunc_opts` * fix updating of `cur_len` in ragged convolution * remove bounds on mean parameters * use dist_spec syntax in `estimate_delays` * add print function for `dist_spec` * add names to printing if given * clarify printouts * reduce unnecessary function calls * Revert "reduce unnecessary function calls" This reverts commit 4d7e5666ddf054652de5c18a060aabf161be0214. * fix typo * add default option for generation time * simplify delay inits * update pmf doc * dist -> distribution * fix max of np dist logic * simplify pmf truncation syntax * fix typos and use `is` * extract function for stan code conversion * fix variable name * fix function name * do truncnorm with appropriate lengths * fix initial condition sampling * update `to_stan` documentation * fix typo * stan model with unified delays * update R access to unified dist interface * update tests * ensure arrays are arrays * simplify stan seq (and avoid conflict with R) * fix test * fix simulation models * fix final tests * update usage of c -> + * Automatic readme update * update examples/doc and re-doc * linting * update docs * final requested lint * update return type of bootstrapped_dist_fit * redoc * update estimate_delay to reflect changes * dot product for all convolutions * report gt mean and var * bug fix in calculation of max delays * Automatic readme update * update tests * clean whitespace * reduce number of calculations by precomputing len * optional head/tail * Revert "optional head/tail" This reverts commit 8c59db1. * don't convolve first pmf * reduce vector copying * fix reversing * fix printing of combined distributions * add exampples, export, and add basic dist plotting * Automatic readme update * add some tests for dist_spec * add tests for +.dist_spec * add tests for mean.dist_spec * add some basic additional tests and docs * linting * fix linting * export c * fix plotting to work with c() method for dist_spec * more linting fixes * remove extract line in generation_time.stan * add a check in convolve_rev_pmf when len >= xlen + ylen and update tests * be more efficient when calc discrete pmfs * catch missing indexes in omf calc * clarify comments * add tolerance for +.dist_spec * don't load testthat * trigger benchmarking * remove benchmark trigger * linting * Automatic readme update * trigger benchmarking * remove benchmark trigger * refine tolerance checks for convolution * fix example * add an internal function * trigger benchmarking * benchmarking off * add back in missing tolerance docs * fix edge case check for length 1 pmfs * whitespace linting * test more carefully * use commas like a smart boy * crank that adapt delta handle * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/get.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/opts.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/dist.R Co-authored-by: Sebastian Funk <[email protected]> * fixed @internal and brackets + fcase * don't export c.dist_spec * drop c() examle from plot --------- Co-authored-by: Sam Abbott <[email protected]> Co-authored-by: GitHub Action <[email protected]>
* implement `dist_spec` interface * Add skipping of stan tests in the expected places * fix checks * fix epinow example * update syntax in more places * update another example * move parenthesis to the right place * update uncertainty in estimate_infections example * fix use of generation_time_opts as resource to call get_generation_time() * break line to make R CMD CHECK happy * fix uses of `trunc_opts` * fix updating of `cur_len` in ragged convolution * remove bounds on mean parameters * use dist_spec syntax in `estimate_delays` * add print function for `dist_spec` * add names to printing if given * clarify printouts * reduce unnecessary function calls * Revert "reduce unnecessary function calls" This reverts commit 9037e21. * fix typo * add default option for generation time * simplify delay inits * update pmf doc * dist -> distribution * fix max of np dist logic * simplify pmf truncation syntax * fix typos and use `is` * extract function for stan code conversion * fix variable name * fix function name * do truncnorm with appropriate lengths * fix initial condition sampling * update `to_stan` documentation * fix typo * stan model with unified delays * update R access to unified dist interface * update tests * ensure arrays are arrays * simplify stan seq (and avoid conflict with R) * fix test * fix simulation models * fix final tests * update usage of c -> + * Automatic readme update * update examples/doc and re-doc * linting * update docs * final requested lint * update return type of bootstrapped_dist_fit * redoc * update estimate_delay to reflect changes * dot product for all convolutions * report gt mean and var * bug fix in calculation of max delays * Automatic readme update * update tests * clean whitespace * reduce number of calculations by precomputing len * optional head/tail * Revert "optional head/tail" This reverts commit 8c59db1. * don't convolve first pmf * reduce vector copying * fix reversing * fix printing of combined distributions * add exampples, export, and add basic dist plotting * Automatic readme update * add some tests for dist_spec * add tests for +.dist_spec * add tests for mean.dist_spec * add some basic additional tests and docs * linting * fix linting * export c * fix plotting to work with c() method for dist_spec * more linting fixes * remove extract line in generation_time.stan * add a check in convolve_rev_pmf when len >= xlen + ylen and update tests * be more efficient when calc discrete pmfs * catch missing indexes in omf calc * clarify comments * add tolerance for +.dist_spec * don't load testthat * trigger benchmarking * remove benchmark trigger * linting * Automatic readme update * trigger benchmarking * remove benchmark trigger * refine tolerance checks for convolution * fix example * add an internal function * trigger benchmarking * benchmarking off * add back in missing tolerance docs * fix edge case check for length 1 pmfs * whitespace linting * test more carefully * use commas like a smart boy * crank that adapt delta handle * Update R/create.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/get.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/opts.R Co-authored-by: Sebastian Funk <[email protected]> * Update R/dist.R Co-authored-by: Sebastian Funk <[email protected]> * fixed @internal and brackets + fcase * don't export c.dist_spec * drop c() examle from plot --------- Co-authored-by: Sam Abbott <[email protected]> Co-authored-by: GitHub Action <[email protected]>
Updates the distribution interface to use the
dist_spec
S3 class. In the process also implements nonparametric distribution specifications (indicated_np
vs._p
for parametric) and multiple generation times (e.g. for vector-bourne diseases). All fixed delay distributions are now calculated in R using the existing functionality indist_skel
.Open questions:
dist_spec
a good title (maybe clearer asdelay_dist
or perhaps justdist
)?mean
, andsd
for the lognormal be interpreted as the natural means/sd? I think this would be less confusing, and we could addlogmean
andlogsd
ormu
andsigma
as additional ways of specifying this inlognormal_dist_def
, just as it is ingamma_dist_def
.This fixes #356 and #357. It should also pave the way to implement #313 in the future (as delay distributions can be named and could then be referenced by different observation specifications).