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

add more flexibility with delays #305

Merged
merged 42 commits into from
Nov 18, 2022
Merged

add more flexibility with delays #305

merged 42 commits into from
Nov 18, 2022

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Jul 21, 2022

This adds support for

  • fixed delays (mean only), or fixed delay distributions (mean + sd)
  • fixed truncation distributions (mean + sd)
    as well as unit tests for these.

It is based on #304 but separated out for clarity.

@sbfnk sbfnk requested a review from seabbs July 21, 2022 11:07
Base automatically changed from clearer-truncation to master August 31, 2022 09:35
@seabbs
Copy link
Contributor

seabbs commented Aug 31, 2022

See #308 for an alternative approach where delays are first estimated in R vs in model. This should be more efficient in theory but is incomplete.

seabbs added a commit that referenced this pull request Aug 31, 2022
@sbfnk sbfnk mentioned this pull request Sep 14, 2022
@sbfnk
Copy link
Contributor Author

sbfnk commented Sep 14, 2022

Precomputing the PMFs is of course a good idea to improve performance. This now contains the changes from #311 and #312 but implements the precomputation in the transformed data block of the stan model.

Benefits compared to the approach in #312 are that it avoids bleeding/duplication of model code into R and supports a mixture of fixed and variable delays. Downside is that it adds stan code and thus makes the stan model harder to read.

Passes checks locally but needs some more thorough checking that it hasn't introduced a bug in the model itself. Also the indexing of generation time and delay PMF needs review - I think as implemented it bases the discretised generation times at 1 and delays/truncation at 0.

@sbfnk sbfnk marked this pull request as draft September 14, 2022 11:32
@seabbs
Copy link
Contributor

seabbs commented Sep 22, 2022

This in principle looks good though a little confused where everything is coming from. Did this draw code from the convolve_pmf branch or roll it's own implementation. If it's own I need to get my head around if it's handling convolving pmts correctly.

Code from convolve_pmf for reference.

https://github.com/epiforecasts/EpiNow2/blob/convolve_pmf/inst/stan/functions/convolve.stan

@sbfnk sbfnk mentioned this pull request Sep 23, 2022
@sbfnk
Copy link
Contributor Author

sbfnk commented Sep 23, 2022

It includes the improved discrete convolution from #311 - not sure how that relates to the convolve_pmf. It doesn't contain anything from convolve_pmf explicitly.

Copy link
Contributor

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

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

Sorry, I was just trying to get more familiar with EpiNow2 and the current development and ended up making some comments 🙈

R/opts.R Outdated Show resolved Hide resolved
R/opts.R Outdated Show resolved Hide resolved
R/opts.R Outdated Show resolved Hide resolved
@sbfnk sbfnk changed the base branch from main to develop October 31, 2022 20:34
@sbfnk sbfnk force-pushed the flexible-delays branch 2 times, most recently from 4668ad7 to 7c6bfa3 Compare November 2, 2022 09:23
@sbfnk sbfnk marked this pull request as ready for review November 17, 2022 10:10
@sbfnk sbfnk force-pushed the flexible-delays branch 2 times, most recently from 65ade60 to 0ede0b4 Compare November 18, 2022 19:18
@sbfnk sbfnk merged commit feaa6d4 into develop Nov 18, 2022
@sbfnk sbfnk deleted the flexible-delays branch November 18, 2022 19:20
sbfnk added a commit that referenced this pull request Nov 24, 2022
sbfnk added a commit that referenced this pull request May 3, 2024
sbfnk added a commit that referenced this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants