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

Simulate infections #557

Merged
merged 24 commits into from
Feb 20, 2024
Merged

Simulate infections #557

merged 24 commits into from
Feb 20, 2024

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Feb 19, 2024

Description

This PR closes #509

It re-introduces the simulate_infections function, this time to simulate from given parameters (at the minimum, a trajectory of R and the number of initial infections). It also adds truncation to the simulation model (which had been missing).

Doing so required a small change to extract_parameter_samples which won't affect any existing functionality.

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.

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.

Thanks, Seb. Neat enhancement. I've left a few non-earth-shattering comments.

NEWS.md Outdated Show resolved Hide resolved
@sbfnk
Copy link
Contributor Author

sbfnk commented Feb 20, 2024

Thanks, @jamesmbaazam. Added your suggestions and a few tests which revealed few minor issues in the stan code which have now been fixed.

seabbs
seabbs previously approved these changes Feb 20, 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.

From an overall functionality perspective this looks good to me. One note about a testing gap that is somewhat optional.

I think wait for @jamesmbaazam to be happy with the software dev side and then merge.

R/simulate_infections.R Show resolved Hide resolved
seabbs
seabbs previously approved these changes Feb 20, 2024
Copy link
Contributor

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

  • ✔️default: 31.6s -> 36s [-0.03%, +28.21%]
  • ✔️no_delays: 35.6s -> 37.7s [-22.76%, +34.88%]
  • ✔️random_walk: 8.96s -> 9.23s [-6.75%, +12.9%]
  • ✔️stationary: 24.9s -> 18.4s [-92.81%, +40.53%]
  • ✔️uncertain: 52.7s -> 52.1s [-18.93%, +16.82%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

jamesmbaazam
jamesmbaazam previously approved these changes Feb 20, 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.

Thanks, Seb. LGTM.

Copy link
Contributor

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

  • ✔️default: 32s -> 32.6s [-4.11%, +7.3%]
  • ✔️no_delays: 34.6s -> 34.9s [-16.24%, +18.16%]
  • ✔️random_walk: 30.2s -> 9.25s [-229.87%, +91.14%]
  • ✔️stationary: 17.7s -> 17.6s [-15.61%, +14.89%]
  • ✔️uncertain: 52.5s -> 51.3s [-16.94%, +12.48%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk merged commit 4c1f460 into main Feb 20, 2024
14 checks passed
@sbfnk sbfnk deleted the simulate-infectinos branch February 20, 2024 16:16
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.

forecast_infections
3 participants