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

Discretise with two-day censoring windows #518

Merged
merged 6 commits into from
Mar 25, 2024
Merged

Discretise with two-day censoring windows #518

merged 6 commits into from
Mar 25, 2024

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Nov 17, 2023

This has been shown to reduce bias in the discretisation. More detail in https://package.epinowcast.org/dev/articles/distributions.html

Closes #411

@sbfnk sbfnk force-pushed the improve-delays branch 3 times, most recently from a8a3131 to 84ff91b Compare November 17, 2023 09:47
@sbfnk
Copy link
Contributor Author

sbfnk commented Nov 17, 2023

Some delay-related tests failing - will revisit after merging #504

@sbfnk sbfnk marked this pull request as draft January 23, 2024 11:23
@sbfnk sbfnk force-pushed the main branch 2 times, most recently from 7568882 to 23229a8 Compare February 19, 2024 20:09
@sbfnk sbfnk marked this pull request as ready for review March 13, 2024 12:33
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 19b5707 is merged into main:

  • ✔️default: 33.8s -> 28.6s [-31.69%, +1.19%]
  • ✔️no_delays: 38s -> 37.6s [-19.14%, +17.2%]
  • ✔️random_walk: 9.78s -> 9.13s [-17.13%, +3.75%]
  • 🚀stationary: 21s -> 17.2s [-32.28%, -4.74%]
  • ✔️uncertain: 55.4s -> 55.9s [-24.82%, +26.41%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk requested a review from seabbs March 14, 2024 08:51
jamesmbaazam
jamesmbaazam previously approved these changes Mar 18, 2024
Copy link
Contributor

@jamesmbaazam jamesmbaazam left a 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.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3b3b2a7 is merged into main:

  • 🚀default: 38.3s -> 31.4s [-30.03%, -6.3%]
  • ✔️no_delays: 40.8s -> 41.3s [-28.71%, +30.99%]
  • 🚀random_walk: 10.1s -> 9.07s [-19.9%, -0.5%]
  • 🚀stationary: 21.7s -> 18.5s [-25.33%, -3.98%]
  • ✔️uncertain: 1.06m -> 50.4s [-47.55%, +6.52%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk requested a review from jamesmbaazam March 22, 2024 10:22
@sbfnk
Copy link
Contributor Author

sbfnk commented Mar 22, 2024

I've crosschecked this against the epinowcast vignette on discretised delays and it looks good to me.

Thanks! Added you as a reviewer - if happy please re-approve.

jamesmbaazam
jamesmbaazam previously approved these changes Mar 22, 2024
Copy link
Contributor

@seabbs seabbs 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 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.

@sbfnk
Copy link
Contributor Author

sbfnk commented Mar 22, 2024

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.

Yes it should be - we're already testing that in

test_that("distributions are the same in R and stan", {

@sbfnk sbfnk enabled auto-merge (squash) March 22, 2024 21:05
@sbfnk sbfnk disabled auto-merge March 22, 2024 21:05
Copy link
Contributor

@jamesmbaazam jamesmbaazam left a 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.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 93c072d is merged into main:

  • ✔️default: 38.4s -> 36.3s [-36.3%, +25%]
  • ✔️no_delays: 48.7s -> 39.7s [-65.74%, +28.97%]
  • ✔️random_walk: 10.3s -> 10.6s [-21.68%, +26.07%]
  • ✔️stationary: 20.6s -> 18.2s [-25.65%, +2.42%]
  • ✔️uncertain: 58.9s -> 53.7s [-28.07%, +10.44%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk enabled auto-merge (squash) March 25, 2024 10:13
@sbfnk sbfnk disabled auto-merge March 25, 2024 10:14
@sbfnk sbfnk merged commit f7ded41 into main Mar 25, 2024
15 checks passed
@sbfnk sbfnk deleted the improve-delays branch March 25, 2024 10:14
sbfnk added a commit that referenced this pull request May 3, 2024
* 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
sbfnk added a commit that referenced this pull request May 3, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Discretisation
3 participants