-
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
Discretise with two-day censoring windows #518
Conversation
a8a3131
to
84ff91b
Compare
Some delay-related tests failing - will revisit after merging #504 |
7568882
to
23229a8
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 19b5707 is merged into main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've crosschecked this against the epinowcast vignette on discretised delays and it looks good to me.
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3b3b2a7 is merged into main:
|
Thanks! Added you as a reviewer - if happy please re-approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry i haven't had a chance to fully review but the stan code looks in line with what I expect. Happy to see this merged once @jamesmbaazam is happy (I think we want a big news note about this at the least though as it will change what people are getting).
I think this is true but also we should double check that the R side discretisation implementation is either the same approximation or the more accurate simulation base approach epinowcast uses.
Note both of these should be replaced soon with and at least R side standalone package that handles this all numerically.
Yes it should be - we're already testing that in EpiNow2/tests/testthat/test-dist.R Line 4 in 93c072d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy for this to be merged.
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 93c072d is merged into main:
|
* discretise with two-day window in stan code * discretise with two-day window in `dist_skel` * update tests * add news item * add @jamesmbaazam as reviewer * highlight update as major change
* discretise with two-day window in stan code * discretise with two-day window in `dist_skel` * update tests * add news item * add @jamesmbaazam as reviewer * highlight update as major change
This has been shown to reduce bias in the discretisation. More detail in https://package.epinowcast.org/dev/articles/distributions.html
Closes #411