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

Enhancements to epidemic_size() #212

Merged
merged 10 commits into from
Apr 22, 2024
Merged

Enhancements to epidemic_size() #212

merged 10 commits into from
Apr 22, 2024

Conversation

pratikunterwegs
Copy link
Collaborator

This PR addresses issues raised in #190:

  1. epidemic_size() now optionally returns a <data.table> of epidemic sizes at specified timepoints via the logical simplify argument; the default behaviour remains returning a numeric vector (for backwards compatibility);
  2. Epidemic sizes at multiple timepoints can be requested by passing a vector to stage;
  3. Epidemic sizes can be requested at specific timepoints (e.g. $t = 100, 200, 300$) instead of a proportion of epidemic time;
  4. A data.table is automatically returned, overriding the simplify argument, if a vector is passed to stage or time.

@pratikunterwegs pratikunterwegs self-assigned this Apr 11, 2024
@pratikunterwegs pratikunterwegs linked an issue Apr 11, 2024 that may be closed by this pull request
3 tasks
Copy link

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

  • ✔️default_ode: 14.5ms -> 14.5ms [-1.23%, +1.16%]
  • ✔️default_ode_interventions: 75.8ms -> 75.5ms [-1.11%, +0.39%]
  • ✔️default_ode_param_vec: 848ms -> 848ms [-1.15%, +1.06%]
  • ✔️default_ode_paramvec_intervs: 6.33s -> 6.34s [-0.71%, +1.08%]
  • ✔️ebola: 39.4ms -> 39ms [-4.34%, +1.91%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link

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

  • ✔️default_ode: 13.9ms -> 14ms [-0.57%, +1.04%]
  • ✔️default_ode_interventions: 71.5ms -> 71.7ms [-0.39%, +0.84%]
  • ✔️default_ode_param_vec: 823ms -> 821ms [-1.06%, +0.66%]
  • ✔️default_ode_paramvec_intervs: 6.16s -> 6.17s [-0.17%, +0.67%]
  • ❗🐌ebola: 36.8ms -> 603ms [+1531.63%, +1547.31%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link

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

  • ✔️default_ode: 14.5ms -> 14.4ms [-2.99%, +1.81%]
  • ❗🐌default_ode_interventions: 71.7ms -> 72.2ms [+0.02%, +1.44%]
  • ✔️default_ode_param_vec: 823ms -> 825ms [-0.37%, +0.96%]
  • ✔️default_ode_paramvec_intervs: 6.27s -> 6.24s [-1.61%, +0.78%]
  • ✔️ebola: 629ms -> 629ms [-0.89%, +0.77%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
Copy link
Member

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

This looks great, @pratikunterwegs. The enhancements provide more flexibility to the user. It all LGTM. I left some comments, where I thought some changes could be made to your own satisfaction.

@pratikunterwegs
Copy link
Collaborator Author

Thanks @jamesmbaazam, will address these and merge later today.

Copy link

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

  • ✔️default_ode: 11ms -> 10.9ms [-2.27%, +0.45%]
  • ✔️default_ode_interventions: 73.4ms -> 72.7ms [-1.94%, +0.02%]
  • ✔️default_ode_param_vec: 823ms -> 823ms [-0.42%, +0.46%]
  • ✔️default_ode_paramvec_intervs: 6.24s -> 6.25s [-0.26%, +0.6%]
  • ✔️ebola: 611ms -> 609ms [-1.09%, +0.35%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@pratikunterwegs pratikunterwegs merged commit 376cc99 into main Apr 22, 2024
12 checks passed
@pratikunterwegs pratikunterwegs deleted the develop branch April 22, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Enhance epidemic_size()
2 participants