Make age_limits parameter optional in pop_age()#270
Conversation
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)(Skipped — changes are internal to a single function and do not introduce multi-component control flow requiring a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/pop_age.R (1)
73-134: “Return unchanged whenage_limitsis NULL” isn’t fully true due to earlydata.tablecoercion / keying.Right now,
data.table(pop)+setkeyv()run even whenage_limitsis NULL, which can change attributes (and can mutate by-reference ifpopis already adata.table). If the contract is “unchanged”, early-return before coercion/keying.Proposed diff
if ( !is.data.frame(pop) || !all(hasName(pop, c(pop_age_column, pop_column))) ) { cli::cli_abort( "Expecting {.arg pop} to be a data.frame with columns {.arg {pop_age_column}} and {.arg {pop_column}}." ) } + if (is.null(age_limits)) { + return(pop) + } + pop <- data.table(pop) setkeyv(pop, pop_age_column) - if (!is.null(age_limits)) { - age_limits <- sort(age_limits) - max_age <- max(pop[, pop_age_column, with = FALSE]) - missing_ages <- setdiff( - age_limits[age_limits <= max_age], - pop[[pop_age_column]] - ) - if (length(missing_ages) > 0) { + age_limits <- sort(age_limits) + max_age <- max(pop[, pop_age_column, with = FALSE]) + missing_ages <- setdiff( + age_limits[age_limits <= max_age], + pop[[pop_age_column]] + ) + if (length(missing_ages) > 0) { cli::cli_warn( c( "Not all age groups represented in population data (5-year age band).", # nolint start "i" = "Linearly estimating age group sizes from the 5-year bands." # nolint end ) ) ..original.upper.age.limit <- NULL pop <- pop[, ..original.upper.age.limit := c(pop[[pop_age_column]][-1], NA) ] pop <- pop[, ..original.lower.age.limit := get(pop_age_column)] all_ages <- data.frame(age_limits[ age_limits <= max(pop[[pop_age_column]]) ]) colnames(all_ages) <- pop_age_column pop <- merge(pop, all_ages, all = TRUE, by = pop_age_column) pop <- pop[, ..segment := cumsum(!is.na(..original.lower.age.limit))] pop <- pop[, ..original.lower.age.limit := ..original.lower.age.limit[1], by = ..segment ] pop <- pop[, ..original.upper.age.limit := ..original.upper.age.limit[1], by = ..segment ] pop <- pop[, paste(pop_column) := get(pop_column)[1], by = ..segment] pop <- pop[, ..upper.age.limit := c(pop[[pop_age_column]][-1], NA)] pop[ !is.na(..original.upper.age.limit), population := round( population * (..upper.age.limit - get(pop_age_column)) / (..original.upper.age.limit - ..original.lower.age.limit) ) ] pop <- pop[, c(pop_age_column, pop_column), with = FALSE] - } - - pop <- pop[get(pop_age_column) >= min(age_limits)] - pop <- pop[, - paste(pop_age_column) := reduce_agegroups(get(pop_age_column), age_limits) - ] - pop <- pop[, list(..population = sum(get(pop_column))), by = pop_age_column] - setnames(pop, "..population", pop_column) } + pop <- pop[get(pop_age_column) >= min(age_limits)] + pop <- pop[, + paste(pop_age_column) := reduce_agegroups(get(pop_age_column), age_limits) + ] + pop <- pop[, list(..population = sum(get(pop_column))), by = pop_age_column] + setnames(pop, "..population", pop_column) + setkeyv(pop, pop_age_column) as.data.frame(pop) }
🧹 Nitpick comments (1)
tests/testthat/test-agegroups.r (1)
56-66: Good regression test for omitted/NULLage_limits.Optional: add a small case with
data.table::as.data.table(ages_it_2015)if you want to lock in “unchanged” behavior fordata.tableinputs too.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
R/pop_age.Rman/pop_age.Rdtests/testthat/test-agegroups.r
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.{R,r}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.{R,r}: Rundevtools::test()to ensure all tests pass before committing
Add tests for bug fixes (regression tests) or new features where appropriate
Files:
tests/testthat/test-agegroups.r
**/*.{R,r}
📄 CodeRabbit inference engine (CLAUDE.md)
Run
lintr::lint_package()to check for style issues and fix any in files you modified before committing
Files:
tests/testthat/test-agegroups.rR/pop_age.R
R/**/*.{R,r}
📄 CodeRabbit inference engine (CLAUDE.md)
Run
devtools::document()to regenerate documentation from roxygen comments before committing
Files:
R/pop_age.R
🧠 Learnings (6)
📓 Common learnings
Learnt from: sbfnk
Repo: epiforecasts/socialmixr PR: 248
File: R/contact-matrix-utils.R:206-216
Timestamp: 2025-12-12T19:08:46.874Z
Learning: In the socialmixr package, the add_age() helper function in R/contact-matrix-utils.R intentionally overwrites the age column (e.g., part_age, cnt_age) with values from the exact age column (e.g., part_age_exact, cnt_age_exact) when the exact column exists. This overwriting behavior is by design.
📚 Learning: 2025-12-12T19:08:46.874Z
Learnt from: sbfnk
Repo: epiforecasts/socialmixr PR: 248
File: R/contact-matrix-utils.R:206-216
Timestamp: 2025-12-12T19:08:46.874Z
Learning: In the socialmixr package, the add_age() helper function in R/contact-matrix-utils.R intentionally overwrites the age column (e.g., part_age, cnt_age) with values from the exact age column (e.g., part_age_exact, cnt_age_exact) when the exact column exists. This overwriting behavior is by design.
Applied to files:
man/pop_age.Rdtests/testthat/test-agegroups.rR/pop_age.R
📚 Learning: 2025-12-12T19:13:48.046Z
Learnt from: CR
Repo: epiforecasts/socialmixr PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T19:13:48.046Z
Learning: Applies to tests/**/*.{R,r} : Add tests for bug fixes (regression tests) or new features where appropriate
Applied to files:
tests/testthat/test-agegroups.r
📚 Learning: 2025-09-02T05:04:15.939Z
Learnt from: njtierney
Repo: epiforecasts/socialmixr PR: 196
File: R/contact-matrix-utils.R:7-13
Timestamp: 2025-09-02T05:04:15.939Z
Learning: In the socialmixr package, functions like est_part_age_mean create new columns (part_age) by imputing from other existing columns (part_age_exact, part_age_est_min, part_age_est_max). The condition checks the source columns, not the target column being created.
Applied to files:
R/pop_age.R
📚 Learning: 2026-01-07T10:52:15.874Z
Learnt from: sbfnk
Repo: epiforecasts/socialmixr PR: 259
File: R/load_survey.R:20-25
Timestamp: 2026-01-07T10:52:15.874Z
Learning: Use Zenodo concept DOIs (e.g., 10.5281/zenodo.1157918) in code examples and documentation so readers always access the latest dataset version without modifying code. Replace versioned or specific DOIs in examples (including R scripts like R/load_survey.R) with the concept DOI and include a brief note that it resolves to the latest version.
Applied to files:
R/pop_age.R
📚 Learning: 2026-01-09T19:13:19.631Z
Learnt from: sbfnk
Repo: epiforecasts/socialmixr PR: 266
File: tests/testthat/test-clean.R:197-208
Timestamp: 2026-01-09T19:13:19.631Z
Learning: In tests for the socialmixr package (e.g., any test files under tests/), ensure the clean() function preserves only columns present in the input data and does not add missing columns (such as country). Tests should explicitly verify that absent columns remain absent after cleaning, and that any changes to introduce missing columns are deliberate and intentional.
Applied to files:
R/pop_age.R
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: document
🔇 Additional comments (3)
R/pop_age.R (2)
7-8: Roxygen: optionalage_limitsis clearly documented.
28-37: Thedeprecated()function is already imported and does not need qualification.
deprecatedis properly imported viaimportFrom(lifecycle,deprecated)in NAMESPACE and declared via roxygen imports in R/socialmixr-package.R, so the unqualifieddeprecated()calls will not error. While usinglifecycle::deprecated()for explicitness is a stylistic choice, it is not required.man/pop_age.Rd (1)
7-16: Docs are aligned with the new default (age_limits = NULL).As per guidelines, ensure this came from
devtools::document()(and runlintr::lint_package()+devtools::test()) before merge.Also applies to: 21-23
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/pop_age.R`:
- Around line 81-126: The interpolation step uses a hardcoded population column
name; change the assignment at the population rounding (currently "population :=
round(...)") to use the dynamic pop_column variable instead. Specifically,
inside the block operating on pop (the data.table) where it filters with
!is.na(..original.upper.age.limit) and computes the fraction using
..upper.age.limit, get(pop_age_column), ..original.upper.age.limit and
..original.lower.age.limit, replace the literal population := ... with a dynamic
assignment like (pop_column) := round(get(pop_column) * (..upper.age.limit -
get(pop_age_column)) / (..original.upper.age.limit -
..original.lower.age.limit)) so the code honors custom pop_column names.
1 participant
Summary
age_limitsparameter explicitly optional withNULLdefaultif (!missing(age_limits))toif (!is.null(age_limits))pop_age()withoutage_limitsFixes #268
Summary by CodeRabbit
New Features
age_limitsis now optional; omitting or setting it to NULL returns the population data unchanged.Bug Fixes
age_limitsis NULL and improves handling of missing age groups.Documentation
Tests
age_limitsbehavior.✏️ Tip: You can customize this high-level summary in your review settings.