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

estimate r from initial R #923

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

estimate r from initial R #923

wants to merge 24 commits into from

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Jan 10, 2025

Description

This PR closes #920. For now it's using the existing estimate for initial number of infections. As an initial step I'm interested in how doing this affects performance and/or recovery of estimates.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

@seabbs
Copy link
Contributor

seabbs commented Jan 10, 2025

it seems like there are some issues when accumulating to weekly

Copy link
Contributor

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

  • ❗🐌default: 22.4s -> 31.8s [+27.84%, +56.7%]
  • ❗🐌no_delays: 24.4s -> 35s [+28.06%, +58.29%]
  • ❗🐌random_walk: 9.08s -> 12.7s [+34.19%, +45.36%]
  • ❗🐌stationary: 12.9s -> 19.2s [+27.9%, +70.83%]
  • ❗🐌uncertain: 36.7s -> 47.2s [+16.65%, +40.78%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk marked this pull request as ready for review January 14, 2025 10:16
@sbfnk
Copy link
Contributor Author

sbfnk commented Jan 14, 2025

This is now ready for initial review but it might require a bit more testing (and doc updates / news item etc) before releasing it in the wild. A few observations so far

  • I couldn't get the package to compile when using the algebraic newton solver either locally or on GHA. I could get it to compile with the Powell solver but this led to occasional failures. It all seems to work well (and possibly faster) hand-rolled Newton solver. For now these two methods are both available as stan functions.
  • Initialisation of the number of infections remains a critical issue. If doing this independently of estimated growth we get a strong correlation with the initial R as predicted by @SamuelBrand1 - for now what's estimated is roughly the initial number of infections related to the first week of data, which is then scaled based on the growth rate and length of the seeding period; doing it this way reduces the correlation and improves fits, but means we can no longer interpret the corresponding parameter as being the initial number of infections. If going ahead with this we'll have to adjust the simulation functions.
  • touchstone is failing now for unknown reasons, which is annoying because it would be good to measure the performance impact of the latest version.

@sbfnk sbfnk requested a review from seabbs January 14, 2025 10:23
@seabbs
Copy link
Contributor

seabbs commented Jan 17, 2025

touchstone is failing now for unknown reasons, which is annoying because it would be good to measure the performance impact of the latest version.

I see this in epinowcast as well. Precisely what is happening is not clear to me.

inst/stan/functions/rt.stan Outdated Show resolved Hide resolved
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.

t a high level I think this looks good and it seems reasonable to rescale initial infections I think. A few comments and questions about output changes. I have just site read so far so need to have a play with the models.

inst/stan/functions/rt.stan Show resolved Hide resolved
tests/testthat/_snaps/simulate-infections.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/simulate-infections.md Outdated Show resolved Hide resolved
@sbfnk
Copy link
Contributor Author

sbfnk commented Jan 20, 2025

touchstone is failing now for unknown reasons, which is annoying because it would be good to measure the performance impact of the latest version.

I see this in epinowcast as well. Precisely what is happening is not clear to me.

I now see the same error locally on a linux box. Pretty sure it's unrelated to touchstone or package code but why exactly it happens is a mystery to me, too (as the action goes from success to failure with nearly identical logs, i.e. same package versions, same runner version etc.).

@seabbs seabbs self-requested a review January 20, 2025 16:23
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.

Use growth approximation to initialise growth rate
2 participants