CRAN release/v1.4.1 with coverage and snapshot stabilization#510
CRAN release/v1.4.1 with coverage and snapshot stabilization#510kristinawlai wants to merge 53 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Prepares the serocalculator package for a CRAN submission by switching from a development version to a release version and updating the CRAN submission notes to reflect the current check status and recent CRAN archival context.
Changes:
- Updated
DESCRIPTIONversion from1.4.0.9008to1.4.1for the CRAN release. - Updated
cran-comments.mdto describe the prior archival reason and current R CMD check note(s).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cran-comments.md |
Updates CRAN resubmission narrative and summarizes current check results/notes. |
DESCRIPTION |
Bumps package version to the CRAN release version 1.4.1. |
| Package: serocalculator | ||
| Title: Estimating Infection Rates from Serological Data | ||
| Version: 1.4.0.9008 | ||
| Version: 1.4.1 |
There was a problem hiding this comment.
DESCRIPTION is being set to version 1.4.1, but NEWS.md currently starts with “serocalculator (development version)” and has no 1.4.1 section header. For a CRAN release, NEWS should typically include an explicit 1.4.1 section (and move the current top entries under it or under a new “development version” header below).
| Version: 1.4.1 | |
| Version: 1.4.1.9000 |
There was a problem hiding this comment.
Not making this change now for CRAN release.
There was a problem hiding this comment.
Not making this change now for CRAN release.
you already created the 1.4.1 tag on GitHub; now we should increment the dev version before merging back to main, as Copilot suggests; see step 5 of https://r-pkgs.org/release.html#sec-release-post-acceptance
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
|
…ons are not allowed in CRAN
…1 cores to only 2 (as recommended by CRAN). Also fix lints in affected files.
Co-authored-by: kristinawlai <57973437+kristinawlai@users.noreply.github.com> Agent-Logs-Url: https://github.com/UCD-SERG/serocalculator/sessions/84c8c89d-0194-4419-9edb-777b2ef5ba66
|
@copilot write tests to cover the 13 lines of |
|
@copilot write tests to cover the 13 lines of sim_pop_data_multi.R that are not currently covered by tests to meet the minimum coverage levels. |
|
Claude finished @kristinawlai's task in 2m 32s —— View job Code Review: CRAN release/v1.4.1
SummaryGood progress overall. The D-Morrison's CHANGES_REQUESTED — current status
Must-fix before merging (unblocks d-morrison's CHANGES_REQUESTED)1. d-morrison's review asked for the 24-line env-var block (lines 30–52) and the "`est_seroincidence_by()` has reduced its `num_cores` argument..."…which is wrong when the caller is 2. Both tags are still present in the source (flagged in every prior review): #' @param ... arguments passed to [sim.cs()] # stale: old name, redundant
#' @inheritDotParams sim_pop_data # wins in the generated .RdThe Should-fix (cosmetic / avoids future reviewer confusion)3. n_cores <- n_cores <- min(2, parallel::detectCores())R evaluates right-to-left so the result is correct, but 4. Header says 5. # Apply your existing safety checker after any cap"your" is second-person in source code. Change to: Cosmetic
What looks good
|
…calculator into CRANrelease/v1.4.1
|
Claude finished @kristinawlai's task in 2m 6s —— View job Code Review: CRAN release/v1.4.1
SummaryThe latest commit ( What the latest commit fixed ✅
D-Morrison's CHANGES_REQUESTED — current status
Items 6 and 7 are linked — the ask is to consolidate the 24-line env-var block (lines 29–51 in Remaining issues
test_that("`sim_pop_data_multi()` works consistently", {
skip_on_cran()
skip_on_os("linux") # <-- asymmetric platform skipThis is the same pattern removed from
Two spaces between What looks good
|
|
Claude finished @kristinawlai's task in 2m 36s —— View job Code Review: CRAN release/v1.4.1
SummaryThe latest commit ( Several items from prior reviews are now resolved (✅ below). Two structural issues remain. Critical bug (introduced in
|
| # | Item | Status |
|---|---|---|
| 1 | DESCRIPTION: bump version to 1.4.1.9000 |
✅ done |
| 2 | DESCRIPTION: add snapr back (CRAN version) |
✅ done |
| 3 | DESCRIPTION: remove Remotes: |
✅ done |
| 4 | NEWS.md: add dev-version placeholder heading |
✅ done (extra space, see below) |
| 5 | test-sim_pop_data.R: remove skip_on_os("mac"), use snapr::expect_snapshot_data() |
✅ done |
| 6 | Move _R_CHECK_LIMIT_CORES_ block into check_parallel_cores() |
|
| 7 | Move check_parallel_cores() call inside the env-var block |
Items fixed since prior review ✅
@param ... arguments passed to [sim.cs()]stale tag removed fromsim_pop_data_multi.R✅check_parallel_cores.Rmessage no longer namesest_seroincidence_by()✅cran-comments.mdnote count corrected to2 notes✅- Double assignment
n_cores <- n_cores <- ...fixed in vignette ✅ - Informal
# Apply your existing safety checker after any capcomment fixed ✅
Remaining minor issues
tests/testthat/test-sim_pop_data_multi.R:3 — skip_on_os("linux") in snapshot test
The platform skip is still present. Linux is the CI platform — this silently skips the snapshot on the machine most likely to catch regressions. If the RNG seeding is deterministic, the skip should not be needed. If Linux non-determinism is unavoidable (e.g. glibc differences), a comment explaining why would help future reviewers.
NEWS.md:1 — extra space after #
# serocalculator (development version)
Two spaces between # and serocalculator; should be one (consistent with the # serocalculator 1.4.1 header on line 5 and d-morrison's suggestion). Fix this →
What looks good
DESCRIPTIONversion correctly set to1.4.1.9000✓snaprrestored to Suggests (CRAN version, no version constraint);Remotes:deleted ✓skip_on_os("mac")removed fromtest-sim_pop_data.R✓withr::local_seed()with explicitMersenne-Twister/Inversion/RejectionRNG kind ✓csdata |> expect_snapshot_data(name = "sim_pop_data")— snapr API used correctly ✓num_coresdefault changed to2L— CRAN-safe ✓sapply→vapply(FUN.VALUE = integer(1))✓message()→cli::cli_inform()for verbose output ✓@param .../@inheritDotParamsduplicate tag removed ✓cran-comments.mdnote count matches ✓n_cores→num_coresconsistent across examples, vignettes, fixtures ✓
|
Claude finished @kristinawlai's task in 5m 47s —— View job Code Review: CRAN release/v1.4.1
SummaryThe latest commit ( Critical bug — still present
return(num_cores) # ← function exits here on line 20
chk <- Sys.getenv(...) # ← dead code: never runs (line 22)
if (nzchar(chk)) { ... } # ← dead code (lines 24–44)
if (verbose) { ... } # ← dead code; `verbose` is also not a parameter (lines 46–48)
There is also an additional bug in the dead block: Fix: Move the entire Correct structure: check_parallel_cores <- function(num_cores) {
requireNamespace("parallel", quietly = FALSE)
chk <- Sys.getenv("_R_CHECK_LIMIT_CORES_", "")
if (nzchar(chk)) {
chk_u <- toupper(chk)
if (chk_u %in% c("TRUE", "T", "YES", "Y")) {
num_cores <- min(num_cores, 2L)
} else if (!chk_u %in% c("FALSE", "F", "NO", "N")) {
chk_n <- suppressWarnings(as.integer(chk))
num_cores <- if (!is.na(chk_n) && chk_n >= 1L) min(num_cores, chk_n) else min(num_cores, 2L)
}
}
if (num_cores > (parallel::detectCores() - 1)) {
num_cores <- min(num_cores, parallel::detectCores() - 1)
cli::cli_inform(class = "reduced num_cores", c(...))
}
return(num_cores)
}Cascading test failure
The test mocks # line 126 — expects min(2L, 1L) = 1L when _R_CHECK_LIMIT_CORES_="1"
expect_identical(calls[[3]], expected_calls$numeric_cap) # 1L expected, 2L actual
# line 130
expect_identical(register_calls[[3]], expected_calls$numeric_cap) # same failureSince neither Once the dead code is fixed (block moved before
D-Morrison's CHANGES_REQUESTED — current status
Remaining issues
test_that("`sim_pop_data_multi()` works consistently", {
skip_on_cran()
skip_on_os("linux") # <-- CI platform silently skippedLinux (GitHub Actions ubuntu-latest) is the primary CI platform. Skipping the snapshot there means the snapshot is never validated in CI, which defeats its purpose. If the explicit RNG seeding with
Two spaces between What looks good
|
|
Claude finished @kristinawlai's task in 4m 4s —— View job Code Review: CRAN release/v1.4.1
Correction from the prior reviewThe 2026-06-12 19:00 review incorrectly stated that the Must fix1. The return(num_cores) # line 21 — exits here
chk <- Sys.getenv(...) # lines 23–50 — dead code, never runs
if (nzchar(chk)) { ... }
if (verbose) { ... } # `verbose` also not a parameter of this functionThe env-var cap works correctly because it's still in 2. "`est_seroincidence_by()` has reduced its `num_cores` argument..."This function is called from D-Morrison's CHANGES_REQUESTED — current status
For items 6 & 7: the path of least resistance now is to remove the dead block from Should fix (flagged across multiple prior reviews)
#' @param ... arguments passed to [sim.cs()] # stale, redundant
#' @inheritDotParams sim_pop_data # wins in the .RdBoth tags are still present. Remove line 10. Fix this →
n_cores <- n_cores <- min(2, parallel::detectCores())
Header says
Test concern
test_that("`sim_pop_data_multi()` works consistently", {
skip_on_cran()
skip_on_os("linux") # <-- GitHub Actions ubuntu-latest silently skippedLinux is the primary CI platform. The snapshot is never validated in CI. If the explicit What looks good
|
Code Review: CRAN release/v1.4.1
Working... |
- Remove stale @param ... [sim.cs()] tag that shadowed @inheritDotParams - Generalize check_parallel_cores() message (was est_seroincidence_by()-specific) - Fix double assignment n_cores <- n_cores <- in vignette - Correct cran-comments.md note count from 1 to 2 - Fix extra space in NEWS.md development version header Co-authored-by: Kristina W. Lai <undefined@users.noreply.github.com>
|
Claude finished @kristinawlai's task in 2m 35s —— View job Code Review: CRAN release/v1.4.1
SummaryThe latest commit ( What
|
| Item | Fixed |
|---|---|
check_parallel_cores.R — dead code after return() removed |
✅ |
check_parallel_cores.R — generic message (no longer names est_seroincidence_by()) |
✅ |
sim_pop_data_multi.R:10 — stale @param ... arguments passed to [sim.cs()] removed |
✅ |
vignettes/articles/simulate_xsectionalData.qmd:296 — double assignment fixed |
✅ |
cran-comments.md — note count corrected to 2 notes |
✅ |
sim_pop_data_multi.R:53 — "your" removed from comment |
✅ |
NEWS.md:1 — extra space after # cleaned up |
✅ |
D-Morrison's CHANGES_REQUESTED — current status
| # | Item | Status |
|---|---|---|
| 1 | DESCRIPTION: bump version to 1.4.1.9000 |
✅ done |
| 2 | DESCRIPTION: add snapr back (CRAN version) |
✅ done |
| 3 | DESCRIPTION: remove Remotes: |
✅ done |
| 4 | NEWS.md: add dev-version placeholder heading |
✅ done |
| 5 | test-sim_pop_data.R: remove skip_on_os("mac"), use snapr::expect_snapshot_data() |
✅ done |
| 6 | Move _R_CHECK_LIMIT_CORES_ block into check_parallel_cores() |
❌ still in sim_pop_data_multi.R |
| 7 | Move check_parallel_cores() call inside the env-var block / call unconditionally |
❌ same |
Items 6 & 7 are a judgment call for @d-morrison: the env-var cap works correctly in sim_pop_data_multi.R (lines 29–51) and check_parallel_cores.R is now clean (no dead code). Consolidating the logic into check_parallel_cores() is purely a structural preference. @kristinawlai has noted a preference to defer further refactors to a follow-up branch.
Remaining issue
tests/testthat/test-sim_pop_data_multi.R:3 — skip_on_os("linux") skips snapshot on CI
test_that("`sim_pop_data_multi()` works consistently", {
skip_on_cran()
skip_on_os("linux") # ← GitHub Actions ubuntu-latest silently skippedThis skip has been flagged in every review. Linux (GitHub Actions ubuntu-latest) is the primary CI platform — the snapshot is never validated in CI, which defeats its purpose. If the explicit withr::local_seed() seeding is sufficient for cross-platform determinism, the skip should not be needed. If Linux still produces different output for an unavoidable reason (e.g. platform-specific C library differences), a comment explaining why would help future reviewers. Fix this →
Cosmetic (NEWS.md)
NEWS.md lines 27 and 60 have trailing whitespace (a blank line with spaces). Not a functional issue, but lintr's trailing_whitespace_linter may flag these. Worth a quick cleanup. Fix this →
What looks good
DESCRIPTIONversion set to1.4.1.9000for post-release development ✓snaprin Suggests (CRAN version, no version constraint);Remotes:deleted ✓check_parallel_cores.Ris clean — no dead code, generic message ✓skip_on_os("mac")removed fromtest-sim_pop_data.R;expect_snapshot_data()used ✓withr::local_seed()with explicitMersenne-Twister/Inversion/RejectionRNG kind ✓num_coresdefault changed to2L— CRAN-safe ✓_R_CHECK_LIMIT_CORES_handling coversTRUE/T/YES/Y,FALSE/F/NO/N, numeric strings, and unrecognised values ✓sapply→vapply(FUN.VALUE = integer(1))✓message()→cli::cli_inform()for verbose output ✓@param .../@inheritDotParamsduplicate tag removed fromsim_pop_data_multi.R✓cran-comments.mdnote count matches the actual two notes ✓- Double assignment
n_cores <- n_cores <- ...fixed in vignette ✓ @inheritDotParamserror inplot_curve_params_one_ab.Rfixed; generated.Rdclean ✓n_cores→num_coresapplied consistently across examples, vignettes, fixtures ✓man/sim_pop_data_multi.Rdregenerated correctly —...documents thesim_pop_dataargs, no reference tosim.cs()✓
|
Claude finished @kristinawlai's task in 5m 21s —— View job Code Review: CRAN release/v1.4.1
SummaryCommit What
|
| Item | Fixed |
|---|---|
check_parallel_cores.R — dead code after return() removed |
✅ |
check_parallel_cores.R — chk and verbose now proper parameters |
✅ |
sim_pop_data_multi.R — reads chk and passes it to check_parallel_cores() |
✅ |
DESCRIPTION version 1.4.1.9000, snapr in Suggests, no Remotes: |
✅ |
skip_on_os("mac") removed from test-sim_pop_data.R |
✅ |
withr::local_seed() with explicit RNG kind/normal/sample args |
✅ |
cran-comments.md note count corrected to 2 notes |
✅ |
vignettes/…/simulate_xsectionalData.qmd:296 double assignment fixed |
✅ |
NEWS.md:1 single space after # |
✅ |
Stale @param ... [sim.cs()] tag removed from sim_pop_data_multi.R |
✅ |
Generic message in check_parallel_cores.R (no est_seroincidence_by() reference) |
✅ |
D-Morrison's CHANGES_REQUESTED — current status
| # | Item | Status |
|---|---|---|
| 1 | DESCRIPTION: bump to 1.4.1.9000 |
✅ done |
| 2 | DESCRIPTION: add snapr back (CRAN version) |
✅ done |
| 3 | DESCRIPTION: remove Remotes: |
✅ done |
| 4 | NEWS.md: dev-version placeholder heading |
✅ done |
| 5 | test-sim_pop_data.R: remove skip_on_os("mac"), use snapr |
✅ done |
| 6 | Move _R_CHECK_LIMIT_CORES_ block into check_parallel_cores() |
✅ done (via chk parameter) |
| 7 | Move check_parallel_cores() call inside the env-var block |
✅ done (unconditionally called when num_cores > 1L) |
Must-fix (new issues in this round)
1. R/check_parallel_cores.R:51–58 — duplicate verbose messages
The verbose block now emits two nearly identical messages:
if (verbose) {
cli::cli_inform(
"Setting up parallel processing with `num_cores` = {num_cores}."
)
cli::cli_inform(
"Set up parallel processing with `num_cores`={num_cores}" # ← duplicate
)
}The second message ("Set up parallel processing...") was moved from sim_pop_data_multi.R, but the first was already present in check_parallel_cores.R. Remove one. The first is more consistently formatted (spaces around =, trailing period, backtick-quoted arg name). Fix this →
2. tests/testthat/test-sim_pop_data_multi.R:80 — mock function signature mismatch
The check_parallel_cores mock is function(x), but the production call in sim_pop_data_multi() now uses three named arguments:
# production code (sim_pop_data_multi.R:30):
num_cores <- check_parallel_cores(num_cores = num_cores, chk = chk, verbose = verbose)
# mock (test-sim_pop_data_multi.R:80):
check_parallel_cores = function(x) {
calls <<- c(calls, list(x))
x
}In R, a named actual argument (num_cores = 2L) cannot be matched to an unrelated formal (x) — R would raise an "unused argument" error at call time. The mock needs to accept the same named parameters:
check_parallel_cores = function(num_cores, chk, verbose) {
calls <<- c(calls, list(num_cores))
num_cores
}Additionally, the expected_calls$numeric_cap = min(num_cores, 1L) = 1L is wrong: with the mock bypassing check_parallel_cores, the env-var cap ("1" → cap at 1) is never applied, so calls[[3]] would be 2L (the original num_cores), not 1L. The expected values for the calls list should be 2L across all scenarios. The register_calls assertions (lines 128–131) test the value passed to registerDoParallel, which would also be 2L since the mock returns num_cores unchanged. If the intent is to verify that the env-var cap is applied, the mock on check_parallel_cores should be removed and parallel::detectCores should be mocked instead to avoid the hardware-limit path. Fix this →
Remaining from prior reviews
3. tests/testthat/test-sim_pop_data_multi.R:3 — skip_on_os("linux") skips snapshot on CI
This skip is still present in the sim_pop_data_multi() consistency test. ubuntu-latest (Linux) is the primary CI platform — the snapshot is never validated there. If the explicit set.seed(54321) seeding is enough for cross-platform determinism, remove the skip. If Linux still produces different output, a comment explaining why is warranted. Fix this →
4. R/check_parallel_cores.R:50 — misleading # (Unchanged) comment
# 3. Report setup if verbose (Unchanged)This block was moved from sim_pop_data_multi.R, so it's not "unchanged". Remove the parenthetical. Fix this →
What looks good
DESCRIPTIONversion1.4.1.9000,snaprin Suggests, noRemotes:✓check_parallel_cores()signature now takesnum_cores,chk = "",verbose = FALSE— clean and testable ✓_R_CHECK_LIMIT_CORES_block is beforereturn()— no dead code ✓- Env-var cap covers
TRUE/T/YES/Y,FALSE/F/NO/N, numeric strings, and unrecognized values ✓ sim_pop_data_multi()readschkfrom env and passes it explicitly — correct and mockable design ✓num_coresdefault is2L— CRAN-safe ✓skip_on_os("mac")removed fromtest-sim_pop_data.R✓withr::local_seed()with explicitMersenne-Twister/Inversion/RejectionRNG kind ✓csdata |> expect_snapshot_data(name = "sim_pop_data")— snapr API correct ✓sapply→vapply(FUN.VALUE = integer(1))✓message()→cli::cli_inform()for verbose output ✓cran-comments.mdnote count matches (2 notes) ✓- Double assignment fixed in vignette ✓
NEWS.mddev-version header correct ✓@inheritDotParamserror inplot_curve_params_one_ab.Rfixed ✓n_cores→num_coresconsistent across examples, vignettes, fixtures ✓
Merge branch 'CRANrelease/v1.4.1' of https://github.com/UCD-SERG/serocalculator into CRANrelease/v1.4.1 # Conflicts: # R/check_parallel_cores.R
Code Review: CRAN release/v1.4.1
Working... |
- Fix line length lint errors in R/sim_pop_data_multi.R (lines 28, 30
exceeded 80 chars; wrap check_parallel_cores() call across lines)
- Fix check_parallel_cores mock signature in test-sim_pop_data_multi.R:
mock was function(x) but production code calls with named args
(num_cores, chk, verbose); rewrite test to mock detectCores instead
so the full cap path (including env-var cap inside check_parallel_cores)
is exercised end-to-end via register_calls
- Remove duplicate cli::cli_inform in check_parallel_cores() verbose block
- Remove misleading '(Unchanged)' annotation from comment
- Update man/check_parallel_cores.Rd: \verb{} -> \code{} for
_R_CHECK_LIMIT_CORES_ (roxygen2 8.0.0 uses \code{} for strings
without Rd markup characters)
Co-authored-by: Kristina W. Lai <undefined@users.noreply.github.com>
|
📖 https://UCD-SERG.github.io/serocalculator/preview/pr510/ |
Thank you for taking the time to submit a pull request!
To maximize the chances of acceptance:
-.testthat).For more information see Contributing.
This update reflects the added sim_pop_data_multi core-limit test coverage and stabilized sim_pop_data snapshot RNG behavior.
Changes
DESCRIPTIONversion from1.4.0.9008to1.4.1for the CRAN release.cran-comments.mdto describe the prior archival reason and current R CMD check note(s), with wording corrected to reflect this is a patch (maintenance) release and to referenceinst/WORDLISTaccurately.sim_pop_data_multi()to comply with CRAN core-use policies; defaultnum_coreschanged frommax(1, parallel::detectCores() - 1)to2L.sapplytovapplywithFUN.VALUE = integer(1)inR/sim_pop_data_multi.Rfor type-safe output.tests/testthat/test-sim_pop_data_multi.Rto cover_R_CHECK_LIMIT_CORES_branches and verify expected core cap values plus output structure.sim_pop_data()snapshot by fixing RNG kind/seed intests/testthat/test-sim_pop_data.R.⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.