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

Replace get functions with examples for delay distributions #481

Merged
merged 13 commits into from Oct 23, 2023
Merged

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Oct 16, 2023

Fixes #390.

Also introduces a fix_dist function for removing the uncertainty from delay distributions(😲).

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.

Content-wise this looks good. I think it goes a bit far the other way in that there are now no examples of using dist_spec. In particular, in the README some of the text doesn't really make any sense and some parts of the code just show rather pointless reallocation.

The example reporting delay also has no uncertainty which alters a fair number of the examples and tests where it was used. Perhaps we could just have two or have it be uncertain and use the new helper function to make it have no uncertainty (which is a very nice addition).

R/dist.R Outdated Show resolved Hide resolved
R/epinow.R Outdated Show resolved Hide resolved
R/estimate_infections.R Outdated Show resolved Hide resolved
R/get.R Show resolved Hide resolved
R/get.R Outdated Show resolved Hide resolved
data-raw/reporting-delay.R Show resolved Hide resolved
tests/testthat/test-delays.R Show resolved Hide resolved
tests/testthat/test-epinow.R Show resolved Hide resolved
touchstone/setup.R Outdated Show resolved Hide resolved
touchstone/setup.R Outdated Show resolved Hide resolved
@sbfnk
Copy link
Contributor Author

sbfnk commented Oct 16, 2023

Content-wise this looks good. I think it goes a bit far the other way in that there are now no examples of using dist_spec. In particular, in the README some of the text doesn't really make any sense and some parts of the code just show rather pointless reallocation.

That makes sense. I'm not sure what the right balance is, or if we even want/need these examples. Will revisit starting from where you pointed out it being an issue.

The example reporting delay also has no uncertainty which alters a fair number of the examples and tests where it was used. Perhaps we could just have two or have it be uncertain and use the new helper function to make it have no uncertainty (which is a very nice addition).

Yes I think if we make it uncertain (which may have been my original intention) the rest of the code makes sense.

@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:default: 47.6s -> 44.1s [-18.9%, +4.09%]
  •   :ballot_box_with_check:no_delays: 47.7s -> 44.8s [-19.22%, +6.85%]
  •   :ballot_box_with_check:random_walk: 13.5s -> 13.7s [-7.69%, +10.55%]
  •   :ballot_box_with_check:stationary: 28.5s -> 28s [-14.38%, +10.72%]
  •   :ballot_box_with_check:uncertain: 1.02m -> 1.05m [-7.65%, +14.44%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:default: 48.8s -> 49.4s [-9.31%, +12.07%]
  •   :ballot_box_with_check:no_delays: 47.9s -> 44.9s [-19.13%, +6.74%]
  •   :ballot_box_with_check:random_walk: 14.2s -> 14s [-8.68%, +5.61%]
  •   :ballot_box_with_check:stationary: 29s -> 27.3s [-15.26%, +3.82%]
  •   :ballot_box_with_check:uncertain: 1.14m -> 1.04m [-21.41%, +4.03%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:default: 1.05m -> 58.9s [-22.38%, +8.33%]
  •   :ballot_box_with_check:no_delays: 1m -> 1.06m [-13.77%, +24.87%]
  •   :ballot_box_with_check:random_walk: 18.6s -> 18s [-20.92%, +14.11%]
  •   :ballot_box_with_check:stationary: 36.6s -> 37.3s [-10.72%, +14.61%]
  •   :ballot_box_with_check:uncertain: 1.3m -> 1.38m [-8.68%, +21.13%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:default: 42.1s -> 45.3s [-9.19%, +24.33%]
  •   :ballot_box_with_check:no_delays: 45.9s -> 43.4s [-21.14%, +10.15%]
  •   :ballot_box_with_check:random_walk: 14.5s -> 14.1s [-13.97%, +8.47%]
  •   :ballot_box_with_check:stationary: 27.5s -> 28.2s [-6.09%, +11.54%]
  •   :ballot_box_with_check:uncertain: 1.16m -> 1.18m [-18.93%, +22.86%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk requested a review from seabbs October 20, 2023 14:17
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.

Overall looks good but it looks like lazy loading isn't working as expected when combining distributions or directly specifiying them as args. See the rendered version of the epinow vignette for an example. It may be that they need to be explicitly allocated? A little confused about why that would be the case though? Perhaps due to not having the dev version of the package installed and rendering directly rather than using devtools?

vignettes/epinow.Rmd Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:default: 49.1s -> 49.3s [-14.5%, +15.5%]
  •   :ballot_box_with_check:no_delays: 48.2s -> 44.7s [-23.41%, +8.98%]
  •   :ballot_box_with_check:random_walk: 14.3s -> 13.8s [-10.78%, +2.67%]
  •   :ballot_box_with_check:stationary: 28.3s -> 28.8s [-5%, +8.43%]
  •   :ballot_box_with_check:uncertain: 1.02m -> 1.15m [-0.66%, +26.78%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk requested a review from seabbs October 23, 2023 11:14
@sbfnk
Copy link
Contributor Author

sbfnk commented Oct 23, 2023

This is ready for re-review. Note touchstone failure is expecting as the corresponding scripts have changed.

seabbs
seabbs previously approved these changes Oct 23, 2023
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.

LGTM. Small grammar suggestion and flagging some maybe transient fitting issues in the README (I think this should be its own issue if they start around).

README.Rmd Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor

seabbs commented Oct 23, 2023

Looks much improved.

@sbfnk sbfnk merged commit 070d4e1 into main Oct 23, 2023
@sbfnk sbfnk deleted the issue-390 branch October 23, 2023 18:06
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.

Deprecate get_incubation_period and get_generation_time due to reimplementation elsewhere
3 participants