Skip to content

Simulate infections #557

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.

Sign up for GitHub

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

Jump to bottom
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 •
edited
Loading

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.

@sbfnk sbfnk force-pushed the simulate-infectinos branch from 93a6b3e to 45204f8 Compare February 19, 2024 11:33
jamesmbaazam
jamesmbaazam reviewed Feb 19, 2024
View reviewed changes
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.

jamesmbaazam
jamesmbaazam reviewed Feb 19, 2024
View reviewed changes
@sbfnk sbfnk force-pushed the simulate-infectinos branch 2 times, most recently from 32b5dc8 to 502be82 Compare February 20, 2024 10:00
@sbfnk sbfnk mentioned this pull request Feb 20, 2024
Open
@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
View reviewed changes
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.

@sbfnk sbfnk force-pushed the simulate-infectinos branch from 545fb3c to c25de34 Compare February 20, 2024 10:57
seabbs
seabbs previously approved these changes Feb 20, 2024
View reviewed changes
@sbfnk sbfnk requested a review from jamesmbaazam February 20, 2024 11:33
@github-actions GitHub Actions
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
View reviewed changes
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.

sbfnk and others added 21 commits February 20, 2024 13:44
@sbfnk
add missing truncation
a32036f
@sbfnk
swap check for seeding time
4eef015
@sbfnk
update extract function to work with simulations
dfc611e
@sbfnk
add news item
6bdca31
@sbfnk @jamesmbaazam
fix typo
1b34849 
Co-authored-by: James Azam <james.azam@lshtm.ac.uk>
@sbfnk
add more tests
81d0423
@sbfnk
correctly index infections when no delay
b3533eb
@sbfnk
generation times are now zero-indexed
c85dbe6
@sbfnk
ensure overdispersion is fixed
dbeb911
@sbfnk
improve checks in simulate_infections
b0b1e42
@sbfnk
expand simulate_infections documentation
7ae6e1a
@sbfnk
add simulate_infections snapshot test
1da5b24
@sbfnk
add backend argument
af90765
@sbfnk
use simpler Poisson for example
506b7bb
@sbfnk
single comment hash for package style
f30f13a
@sbfnk
re-render documentation
bdd5042
@sbfnk @jamesmbaazam
add reviewer
f0bb605 
Co-authored-by: James Azam <james.azam@lshtm.ac.uk>
@sbfnk
only extract frac_obs if obs_scale_sd exists
f159f40
@sbfnk @jamesmbaazam
Apply suggestions from code review
c158a94 
Co-authored-by: James Azam <james.azam@lshtm.ac.uk>
@sbfnk
add deprecation context and re-render doc
8e2e2d8
@sbfnk
improve documentation
9bf0212
@sbfnk sbfnk force-pushed the simulate-infectinos branch from a4713ec to 9bf0212 Compare February 20, 2024 13:44
@github-actions GitHub Actions
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
@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
Simulate infections (#557)
8e20998
sbfnk added a commit that referenced this pull request May 3, 2024
@sbfnk
Simulate infections (#557)
329a344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seabbs seabbs seabbs left review comments

@jamesmbaazam jamesmbaazam jamesmbaazam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

forecast_infections
3 participants
@sbfnk @seabbs @jamesmbaazam