Conversation
sbfnk
force-pushed
the
advance-deprecations
branch
from
49345c5 to
a1f64fc
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request removes deprecated public function arguments, converts deprecation warnings to errors for object accessors, removes deprecated public functions, and changes numeric population parameter handling from warnings to errors. The changes affect core option constructors and estimation functions across the package. Changes
Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sbfnk
force-pushed
the
advance-deprecations
branch
4 times, most recently
from
dc58a74 to
d6460ae
Compare
Remove deprecated arguments that have been erroring since v1.7.0: - gp_opts(alpha_mean, alpha_sd) - obs_opts(phi, na) - estimate_infections(filter_leading_zeros, zero_threshold, horizon) - estimate_secondary(filter_leading_zeros, zero_threshold) - epinow(filter_leading_zeros, zero_threshold, horizon) - regional_epinow(horizon) Advance 1.7.0 deprecation warnings to errors: - rt_opts(pop = numeric) and simulate_infections(pop = numeric) Advance 1.8.0 deprecation warnings to errors: - Deprecated accessors on model objects ($samples, $summarised, etc.) - summary.epinow(output) and summary.estimate_infections(type = 'samples') - extract_parameter_samples() Convert check_generation_time deprecation to plain error. Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
sbfnk
force-pushed
the
advance-deprecations
branch
from
ae3d538 to
bdd1fa1
Compare
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
man/obs_opts.Rd (1)
67-67:⚠️ Potential issue | 🟡 MinorPre-existing doc bug: example comment contradicts the code.
The comment says "Turn off day of the week effect" but the code sets
week_effect = TRUE. This is not introduced by this PR, but since the surrounding documentation was updated, it may be worth fixing in passing.Suggested fix
-# Turn off day of the week effect -obs_opts(week_effect = TRUE) +# Turn off day of the week effect +obs_opts(week_effect = FALSE)tests/testthat/test-regional_summary.R (1)
45-64:⚠️ Potential issue | 🟡 MinorTest description no longer matches the test body.
The test is described as "works with a lower and upper bound of 0" but the body no longer sets any bounds to zero — it simply calls
regional_summarywith the unmodified fit, which is effectively the same as the first test case (line 5). Either the edge-case scenario should be restored with the new accessor patterns, or the test should be removed/renamed to avoid a misleading description.man/cash-.estimate_secondary.Rd (1)
5-5:⚠️ Potential issue | 🟡 MinorUpdate roxygen2
@titleto match other accessor documentation files.The title for the
$accessor documentation inR/estimate_secondary.Rsays "with deprecated warnings" but should say "with deprecation errors" to be consistent withcash-.estimate_infections.Rdandcash-.epinow.Rd.
🤖 Fix all issues with AI agents
In `@R/opts.R`:
- Around line 349-361: After the numeric deprecation guard in rt_opts, add an
explicit type assertion to validate pop is a dist_spec: call assert_class(pop,
"dist_spec") immediately after the is.numeric(...) deprecate_stop block so
non-numeric, non-<dist_spec> values (e.g., strings) error early rather than
being assigned to opts$pop; keep this validation consistent with other checks
(see existing assert_class usage) and ensure it runs before opts$pop is set.
🧹 Nitpick comments (4)
tests/testthat/test-regional_epinow.R (1)
126-135: Duplicated accessor validation block — consider extracting a helper.Lines 126–135 are identical to lines 48–57. A small helper (e.g.,
expect_valid_accessors(fit)) would reduce duplication and make future accessor changes easier to maintain.tests/testthat/test-epinow.R (1)
44-52: Consider extracting repeated accessor assertions into a shared helper.The same block of accessor checks (
get_samples,summary,estimates_by_report_date,plot) is repeated across five test cases. A small helper (similar to the existingdf_non_zero) would reduce duplication and make it easier to update if accessors change again.♻️ Example helper
assert_epinow_accessors <- function(out, check_plots = TRUE) { df_non_zero(get_samples(out)) df_non_zero(summary(out, type = "parameters")) df_non_zero(estimates_by_report_date(out)$summarised) expect_true(!is.null(summary(out))) if (check_plots) { expect_equal( names(plot(out, type = "all")), c("summary", "infections", "reports", "R", "growth_rate") ) } }Then each test block becomes:
- df_non_zero(get_samples(out)) - df_non_zero(summary(out, type = "parameters")) - df_non_zero(estimates_by_report_date(out)$summarised) - expect_true(!is.null(summary(out))) - expect_equal( - names(plot(out, type = "all")), - c("summary", "infections", "reports", "R", "growth_rate") - ) + assert_epinow_accessors(out)Also applies to: 73-81, 97-105, 121-129, 170-174
NEWS.md (1)
16-22: Some NEWS entries don't start with action verbs.As per coding guidelines, NEWS.md entries should start with action verbs like 'Added', 'Fixed', 'Updated', 'Changed'. A few entries here use passive or subject-first phrasing:
- Line 16: "The
popargument…" → "Changed thepopargument…"- Line 17: "Deprecated accessors…" → "Changed deprecated accessors…"
- Line 22:
`summary.epinow(output)` and…→ "Changedsummary.epinow(output)and…"📝 Suggested wording
-- The `pop` argument in `rt_opts()` and `simulate_infections()` now errors when passed a numeric value. Use `Fixed(pop)` instead. +- Changed the `pop` argument in `rt_opts()` and `simulate_infections()` to error when passed a numeric value. Use `Fixed(pop)` instead. -- Deprecated accessors on model objects now error instead of warning: +- Changed deprecated accessors on model objects to error instead of warning: -- `summary.epinow(output)` and `summary.estimate_infections(type = 'samples')` now error. +- Changed `summary.epinow(output)` and `summary.estimate_infections(type = 'samples')` to error.As per coding guidelines, NEWS.md items should "Start with action verbs: 'Added', 'Fixed', 'Updated', 'Changed'".
R/estimate_infections.R (1)
257-271: Minor inconsistency with$.estimate_secondarypattern.Here
.subset2(x, name)is called after theswitchstatement, whereas in$.estimate_secondaryit's the default case insideswitch. Both are functionally correct —deprecate_stopthrows before.subset2is reached for deprecated names, and for non-deprecated names.subset2runs either way. Just noting the stylistic divergence across accessor methods in the package.
sbfnk
force-pushed
the
advance-deprecations
branch
from
fc8817d to
fd16d5a
Compare
seabbs
left a comment
seabbs
left a comment
There was a problem hiding this comment.
This all looks good to me.
2 participants
Description
This PR closes #1309.
Advances the deprecation cycle following the v1.8.0 CRAN release:
gp_opts(ls_mean, ls_sd, ls_min, ls_max, alpha_mean, alpha_sd)obs_opts(phi, na)estimate_infections(CrIs, weigh_delay_priors, filter_leading_zeros, zero_threshold, horizon)estimate_secondary(filter_leading_zeros, zero_threshold)epinow(filter_leading_zeros, zero_threshold, horizon)regional_epinow(horizon)format_fit(burn_in, start_date)default_fill_missing_obs()functiondeprecate_stop("1.9.0")(were warnings in v1.7.0/v1.8.0):rt_opts(pop = numeric)andsimulate_infections(pop = numeric)$samples,$summarised,$predictions,$posterior,$data,$dist,$obs,$last_obs,$cmf,$estimates, etc.)summary.epinow(output)andsummary.estimate_infections(type = "samples")extract_parameter_samples()Initial submission checklist
devtools::test()anddevtools::check()).devtools::document()).lintr::lint_package()).Summary by CodeRabbit
Release Notes
Chores
Documentation