Fix pop_age() to use pop_column parameter in interpolation#271
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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
🧹 Nitpick comments (2)
R/pop_age.R (1)
111-121: Bug fix is correct; consider using idiomatic data.table dynamic LHS for robustness.The replacement of hard-coded
populationwithpaste(pop_column)/get(pop_column)in the interpolation path matches the intended behavior and aligns with Line 111.Optional follow-up: use
(pop_column) :=instead ofpaste(pop_column) :=(canonical data.table; avoids surprises ifpop_columnisn’t scalar).Proposed tweak (no behavior change intended)
- pop <- pop[, paste(pop_column) := get(pop_column)[1], by = ..segment] + pop <- pop[, (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), - paste(pop_column) := round( + (pop_column) := round( get(pop_column) * (..upper.age.limit - get(pop_age_column)) / (..original.upper.age.limit - ..original.lower.age.limit) ) ]tests/testthat/test-agegroups.r (1)
56-79: Test covers the regression well; prefer asserting (not suppressing) the interpolation warning.Right now
suppressWarnings()could hide unexpected warnings, andexpect_identical()can fail on harmless type differences.Suggested test adjustment
- # Test with interpolation (age_limits not matching existing groups) - # nolint start: implicit_assignment_linter - result <- suppressWarnings( - pop_age( - pop_data, - age_limits = c(0, 8, 15), - pop_age_column = "age_lower", - pop_column = "pop_count" - ) - ) - # nolint end + # Test with interpolation (age_limits not matching existing groups) + expect_warning( + result <- pop_age( + pop_data, + age_limits = c(0, 8, 15), + pop_age_column = "age_lower", + pop_column = "pop_count" + ), + "Linearly estimating" + ) expect_named(result, c("age_lower", "pop_count")) - expect_identical(result$age_lower, c(0, 8, 15)) + expect_equal(result$age_lower, c(0, 8, 15)) # Total population should be preserved - expect_identical(sum(result$pop_count), sum(pop_data$pop_count)) + expect_equal(sum(result$pop_count), sum(pop_data$pop_count))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/pop_age.Rtests/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 (5)
📓 Common learnings
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.
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:
tests/testthat/test-agegroups.rR/pop_age.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:
tests/testthat/test-agegroups.rR/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). (10)
- GitHub Check: ubuntu-latest (oldrel-1)
- GitHub Check: ubuntu-latest (devel)
- GitHub Check: macOS-latest (release)
- GitHub Check: windows-latest (release)
- GitHub Check: ubuntu-latest (release)
- GitHub Check: check-surveys
- GitHub Check: test-coverage
- GitHub Check: pkgdown
- GitHub Check: lint-changed-files
- GitHub Check: document
1 participant
Summary
populationcolumn in interpolation code to use thepop_columnparameterpopulation :=→paste(pop_column) :=population *→get(pop_column) *Fixes #267
Summary by CodeRabbit
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.