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

Deprecate adjust_infection_to_report(), report_cases(), and sample_approx_dist() #597

Merged
merged 21 commits into from
Mar 20, 2024

Conversation

jamesmbaazam
Copy link
Contributor

Description

This PR closes #520.

The functions sample_approx_dist(), report_cases(), and adjust_infection_reports() have been deprecated as the functionality they provide can now be achieved with simulate_secondary().

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.

@jamesmbaazam jamesmbaazam changed the title Deprecate adjust_infection_to_report(), report_cases(), and adjust_infection_reports()` Deprecate adjust_infection_to_report(), report_cases(), and adjust_infection_reports() Mar 4, 2024
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff.

I think it would be good to point the user to how they would simulate_secondary to reproduce the functionality of the deprecated functions - but that should wait until #504 is merged as otherwise the syntax will have to be updated again.

I'd suggest to address the comments/issues and then convert to a draft PR to revisit after #504 is merged.

R/adjust.R Outdated Show resolved Hide resolved
R/adjust.R Outdated Show resolved Hide resolved
R/dist.R Outdated Show resolved Hide resolved
R/report.R Outdated Show resolved Hide resolved
tests/testthat/test-adjust_infection_to_report.R Outdated Show resolved Hide resolved
tests/testthat/test-dist.R Outdated Show resolved Hide resolved
tests/testthat/test-report_cases.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@jamesmbaazam
Copy link
Contributor Author

Good stuff.

I think it would be good to point the user to how they would simulate_secondary to reproduce the functionality of the deprecated functions - but that should wait until #504 is merged as otherwise the syntax will have to be updated again.

I'd suggest to address the comments/issues and then convert to a draft PR to revisit after #504 is merged.

Thanks for the review. I'll convert the PR to a draft now until #504 is merged.

@jamesmbaazam jamesmbaazam marked this pull request as draft March 5, 2024 12:11
R/adjust.R Outdated Show resolved Hide resolved
@jamesmbaazam jamesmbaazam force-pushed the deprecate-redundant-funcs branch from f5ab06c to c25965c Compare March 6, 2024 12:08
@sbfnk sbfnk linked an issue Mar 11, 2024 that may be closed by this pull request
@sbfnk
Copy link
Contributor

sbfnk commented Mar 13, 2024

@jamesmbaazam I've now merged main which seems to pass tests. I think once we add some guidance to the user this is good to go.

Not quite sure where this best lives but here is some code for adjust_infection_to_report:

cases <- data.table::copy(example_confirmed)[, cases := as.integer(confirm)]
delay_def <- lognorm_dist_def(
  mean = 5, mean_sd = 1, sd = 3, sd_sd = 1,
  max_value = 30, samples = 1, to_log = TRUE
)
report <- adjust_infection_to_report(
  cases, delay_defs = delay, reporting_model = function(n) rpois(length(n), n)
)
print(report)


cases <- data.table::copy(example_confirmed)[, primary := as.integer(confirm)]
uncertain_delay <- LogNormal(mean = Normal(5, 1), sd = Normal(3, 1), max = 30)
delay <- fix_dist(uncertain_delay, strategy = "sample")
report <- simulate_secondary(
  cases, delays = delay_opts(delay), obs = obs_opts(family = "poisson")
)
print(report)

and report_cases:

cases <- example_confirmed[1:40]
cases <- cases[, cases := as.integer(confirm)]
cases <- cases[, confirm := NULL][, sample := 1]
reported_cases <- report_cases(
  case_estimates = cases,
  delays = delay_opts(example_incubation_period + example_reporting_delay),
  type = "sample"
)
print(reported_cases$samples)

cases <- example_confirmed[1:40]
cases <- cases[, primary := as.integer(confirm)]
report <- simulate_secondary(
  cases,
  delays = delay_opts(
    fix_dist(example_incubation_period + example_reporting_delay)
  ),
  obs = obs_opts(family = "poisson")
)
print(report)

@jamesmbaazam
Copy link
Contributor Author

jamesmbaazam commented Mar 15, 2024

@jamesmbaazam I've now merged main which seems to pass tests. I think once we add some guidance to the user this is good to go.

Not quite sure where this best lives but here is some code for adjust_infection_to_report:

cases <- data.table::copy(example_confirmed)[, cases := as.integer(confirm)]
delay_def <- lognorm_dist_def(
  mean = 5, mean_sd = 1, sd = 3, sd_sd = 1,
  max_value = 30, samples = 1, to_log = TRUE
)
report <- adjust_infection_to_report(
  cases, delay_defs = delay, reporting_model = function(n) rpois(length(n), n)
)
print(report)


cases <- data.table::copy(example_confirmed)[, primary := as.integer(confirm)]
uncertain_delay <- LogNormal(mean = Normal(5, 1), sd = Normal(3, 1), max = 30)
delay <- fix_dist(uncertain_delay, strategy = "sample")
report <- simulate_secondary(
  cases, delays = delay_opts(delay), obs = obs_opts(family = "poisson")
)
print(report)

and report_cases:

cases <- example_confirmed[1:40]
cases <- cases[, cases := as.integer(confirm)]
cases <- cases[, confirm := NULL][, sample := 1]
reported_cases <- report_cases(
  case_estimates = cases,
  delays = delay_opts(example_incubation_period + example_reporting_delay),
  type = "sample"
)
print(reported_cases$samples)

cases <- example_confirmed[1:40]
cases <- cases[, primary := as.integer(confirm)]
report <- simulate_secondary(
  cases,
  delays = delay_opts(
    fix_dist(example_incubation_period + example_reporting_delay)
  ),
  obs = obs_opts(family = "poisson")
)
print(report)

We could add these to the example files of the deprecated functions and add a details argument to deprecate_warn() to point users to it., i.e., "Use simulate_secondary() instead. See ?<deprecated_func()> for equivalent usage. Note that this will be removed in v2.0.0." See suggested changes in 9737603 and 0570abc.

@jamesmbaazam jamesmbaazam marked this pull request as ready for review March 15, 2024 18:13
@jamesmbaazam jamesmbaazam changed the title Deprecate adjust_infection_to_report(), report_cases(), and adjust_infection_reports() Deprecate adjust_infection_to_report(), report_cases(), and sample_approx_dist() Mar 15, 2024
@sbfnk sbfnk enabled auto-merge (squash) March 20, 2024 08:39
@sbfnk
Copy link
Contributor

sbfnk commented Mar 20, 2024

Great work!

@sbfnk sbfnk merged commit 21d0166 into main Mar 20, 2024
10 checks passed
@sbfnk sbfnk deleted the deprecate-redundant-funcs branch March 20, 2024 15:22
sbfnk added a commit that referenced this pull request May 3, 2024
…le_approx_dist()` (#597)

* Deprecate adjust_infection_to_report()

* Deprecate sample_approx_dist()

* Deprecate report_cases()

* Generate .Rd file

* Add NEWS item

* Fix wrong function name in NEWS

* Linting: turn off missing_argument_linter for deprecation warning

* Linting: use file.path() to construct paths instead of paste0.

* Remove unnecessary nolint tags

* Remove trailing comma

* Add more details to deprecation warning

* Assign details argument explicitly

* Add pointers to replacement functions in the deprecation warnings

* Revise example in report_cases to show use of simulate_secondary()

* Refer to the right function in warning message

* Revise examples in adjust_infection_to_reports to demonstrate use of simulate_secondary

* Remove trailing whitespace

---------

Co-authored-by: Sebastian Funk <[email protected]>
sbfnk added a commit that referenced this pull request May 3, 2024
…le_approx_dist()` (#597)

* Deprecate adjust_infection_to_report()

* Deprecate sample_approx_dist()

* Deprecate report_cases()

* Generate .Rd file

* Add NEWS item

* Fix wrong function name in NEWS

* Linting: turn off missing_argument_linter for deprecation warning

* Linting: use file.path() to construct paths instead of paste0.

* Remove unnecessary nolint tags

* Remove trailing comma

* Add more details to deprecation warning

* Assign details argument explicitly

* Add pointers to replacement functions in the deprecation warnings

* Revise example in report_cases to show use of simulate_secondary()

* Refer to the right function in warning message

* Revise examples in adjust_infection_to_reports to demonstrate use of simulate_secondary

* Remove trailing whitespace

---------

Co-authored-by: Sebastian Funk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants