Skip to content

Fix package dependencies, namespace exports, and package infrastructure#12

Merged
Kwan-Jenny merged 11 commits intomanuscriptfrom
copilot/sub-pr-11
Mar 31, 2026
Merged

Fix package dependencies, namespace exports, and package infrastructure#12
Kwan-Jenny merged 11 commits intomanuscriptfrom
copilot/sub-pr-11

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 1, 2026

Addresses code review feedback on dependency management and ::: operator usage, and fixes CI package-load failures caused by stale namespace exports and missing mock data objects.

Dependency Fixes

  • Move tibble and tidyr to Imports (used by package runtime code)
  • Add serocalculator and rlang to Imports (missing declarations)
  • Keep testthat (>= 3.0.0) in Suggests with Config/testthat/edition: 3
  • Move purrr back to Suggests after confirming no purrr:: usage in R/*.R

Replace ::: with Internal Implementations

Created R/utils_internal.R with internal replacements to avoid non-exported access:

  • get_timeindays_var()
  • ab()

These implementations were verified and aligned with ucdavis/serodynamics source behavior, and all serodynamics::: calls were removed from:

  • compute_residual_metrics.R
  • predict_posterior_at_times.R
  • fig2_overall_newperson.R

Code Cleanup

  • Remove unused cohort_name selection in process_shigella_data.R
  • Fix rlang::rnorm()rnorm() in mock data generation script

CI Package-Load Fixes (Namespace/Data)

Resolved shared root cause for docs-check, test-coverage, and lint-changed-files failures:

  • Removed stale export postprocess_jags_output from NAMESPACE
  • Removed stale man/postprocess_jags_output.Rd
  • Regenerated and committed current man/*.Rd files
  • Generated and committed required mock data objects:
    • data/mock_posterior_draws.rda
    • data/mock_case_data.rda
  • Updated ignore rules to allow tracking those two .rda files

Package Infrastructure

CI/CD:

  • R CMD check workflow (ubuntu-latest, r-release + r-devel)
  • pkgdown workflow with gh-pages deployment

Tests:

  • test-compute_residual_metrics.R - validates output structure, metrics sanity
  • test-predict_posterior_at_times.R - multi-subject predictions
  • test-process_shigella_data.R - data reshaping, NA handling
  • test-model_comparison_table.R - comparison logic, percent improvements
  • test-utils_internal.R - internal utility coverage (ab(), get_timeindays_var())

Documentation:

  • data-raw/mock_data.R - generates synthetic posterior draws + case data
  • R/data.R - roxygen docs for mock datasets
  • vignettes/getting-started.Rmd - package overview, usage examples
  • Updated README.Rmd with installation and usage
  • Updated .Rbuildignore to exclude simulation scripts and qmd artifacts

Mock Data Structure:

mock_posterior_draws  # Subject, Iso_type, Chain, Iteration, Parameter, value
mock_case_data        # id, antigen_iso, timepoint, value (with case_data class)

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 5 commits February 1, 2026 07:11
- Move tibble, tidyr, purrr from Suggests to Imports
- Add serocalculator and rlang to Imports
- Remove unused cohort_name selection in process_shigella_data
- Replace serodynamics::: calls with internal helper functions
- Add utils_internal.R with get_timeindays_var() and ab() implementations

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
- Add testthat tests for main functions
- Add R CMD check and pkgdown GitHub Actions workflows
- Add data-raw/ with mock data generation script
- Add R/data.R with mock data documentation
- Add getting-started vignette
- Update README.Rmd with package overview and usage
- Update .Rbuildignore to exclude non-package files

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Use base R rnorm() instead of non-existent rlang::rnorm()

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Set explicit GITHUB_TOKEN permissions (contents: read) for security

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Update comment to reflect internal implementation instead of external ::: call

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Copilot AI changed the title [WIP] Reorganize repository into package-ready structure for R Fix package dependencies, remove ::: usage, add package infrastructure Feb 1, 2026
Copilot AI requested a review from Kwan-Jenny February 1, 2026 07:18
@Kwan-Jenny
Copy link
Copy Markdown
Collaborator

Kwan-Jenny commented Mar 25, 2026

@copilot I've reviewed all 18 changed files in this PR. Most changes look good, but there are several issues that need to be fixed before merging. Please address them in order:


1. Critical: Verify ab() function accuracy (R/utils_internal.R)

The ab() function is the core antibody kinetics model used across compute_residual_metrics.R, predict_posterior_at_times.R, and fig2_overall_newperson.R. If this doesn't match the original serodynamics:::ab() exactly, all downstream analyses will produce wrong results.

Please do the following:

  • Look up the original ab() function source code in ucdavis/serodynamics (check R/ directory for files containing the ab function definition).
  • Compare it line-by-line with the implementation in this PR's R/utils_internal.R.
  • Specifically check:
    • Is the decay formula exp(-alpha * ((t - t1)^shape)) or exp(-(alpha * (t - t1))^shape)? The parenthesization matters — these are different Weibull parameterizations.
    • Does the original handle t1 = 0 (division by zero in the rising phase t / t1)?
    • Does the original handle t < 0?
  • If there are ANY differences, update our ab() to match the original exactly.
  • Add a code comment at the top of ab() referencing where the original lives (e.g., # Mirrors serodynamics:::ab() — see ucdavis/serodynamics/R/<filename>.R).

2. Critical: Verify get_timeindays_var() against original

Similarly, please compare get_timeindays_var() in R/utils_internal.R with the original serodynamics:::get_timeindays_var() source code. Our implementation has a multi-step fallback (check attributes → check column names), but the original may be simpler. If the original only checks attr(dataset, "timeindays"), simplify ours to match.


3. Fix unnecessary skip_if_not_installed in tests

tests/testthat/test-model_comparison_table.R

Remove both skip_if_not_installed("serodynamics") and skip_if_not_installed("serocalculator") lines from both test blocks. The model_comparison_table() function is pure data.frame arithmetic — it never calls serodynamics or serocalculator. These skips cause the tests to be silently skipped in CI (since serodynamics is not on CRAN), making it look like tests pass when they actually never ran.

tests/testthat/test-compute_residual_metrics.R

Check whether compute_residual_metrics() actually calls any serodynamics:: or serocalculator:: function at runtime (not :::, since we replaced those).

  • If it only uses our internal ab() and get_timeindays_var() plus serocalculator::get_values_var(), then keep only skip_if_not_installed("serocalculator") and remove the serodynamics skip.
  • If serocalculator is available as a dependency in DESCRIPTION Imports (which it is), then the skip for serocalculator is also unnecessary — remove it too.

4. Add unit tests for internal functions (ab() and get_timeindays_var())

Create a new file tests/testthat/test-utils_internal.R with the following tests:

For ab():

test_that("ab() returns y0 at t = 0", {
 result <- ab(t = 0, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
 expect_equal(result, 200)
})

test_that("ab() returns y1 at t = t1", {
 result <- ab(t = 10, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
 expect_equal(result, 2000)
})

test_that("ab() shows linear rise for t < t1", {
 result <- ab(t = 5, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
 expected <- 200 + (2000 - 200) * (5 / 10)  # = 1100
 expect_equal(result, expected)
})

test_that("ab() decays toward y0 for large t", {
 result <- ab(t = 10000, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
 expect_true(abs(result - 200) < 1)  # Should be very close to y0
})

test_that("ab() handles vector input", {
 times <- c(0, 5, 10, 30, 90)
 result <- ab(t = times, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
 expect_length(result, 5)
 expect_true(all(is.finite(result)))
 expect_true(all(result >= 200))  # Should never go below baseline
})

test_that("ab() does not error when t1 = 0", {
 # Edge case: immediate peak
 expect_no_error(ab(t = c(0, 10, 30), y0 = 200, y1 = 2000, t1 = 0, alpha = 0.02, shape = 1.2))
})

For get_timeindays_var():

test_that("get_timeindays_var() finds time variable from attribute", {
 mock_data <- data.frame(timepoint = c(0, 7, 30), value = c(100, 200, 300))
 attr(mock_data, "timeindays") <- "timepoint"
 expect_equal(get_timeindays_var(mock_data), "timepoint")
})

test_that("get_timeindays_var() errors when no time variable found", {
 mock_data <- data.frame(x = 1:3, y = 4:6)
 expect_error(get_timeindays_var(mock_data))
})

5. Check purrr placement in DESCRIPTION

Search all files under R/ for any usage of purrr:: (e.g., purrr::map, purrr::map_dfr, etc.):

  • If purrr:: is used in any R/*.R file → keep it in Imports (current state). ✅
  • If purrr:: is NOT used in any R/*.R file → move it back to Suggests. It was originally in Suggests, and if only vignettes or scripts use it, that's where it belongs.

6. Minor: Add source reference comment in R/utils_internal.R

At the top of the file, after the NULL line, add a comment block:

# NOTE: ab() and get_timeindays_var() are internal re-implementations of
# functions from the serodynamics package (ucdavis/serodynamics).
# They are duplicated here to avoid using ::: (non-exported access),
# which is not allowed in R packages passing R CMD check.
# If serodynamics updates these functions, this file must be updated to match.

This helps future contributors (and Ezra) understand why these functions exist here.


Summary of changes needed:

# File Action Priority
1 R/utils_internal.R Verify ab() matches serodynamics:::ab() exactly Critical
2 R/utils_internal.R Verify get_timeindays_var() matches original Important
3 tests/testthat/test-model_comparison_table.R Remove skip_if_not_installed lines Important
4 tests/testthat/test-compute_residual_metrics.R Remove unnecessary skips Important
5 tests/testthat/test-utils_internal.R Create new test file (code provided above) Important
6 DESCRIPTION Move purrr back to Suggests if unused in R/ Minor
7 R/utils_internal.R Add source reference comment Minor

Please address items 1-5 first, then 6-7. Thanks!

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

@Kwan-Jenny
Copy link
Copy Markdown
Collaborator

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot+gpt-5.3-codex) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

Check again above comment @copilot

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

1 similar comment
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@copilot I've reviewed all 18 changed files in this PR. Most changes look good, but there are several issues that need to be fixed before merging. Please address them in order:


...

Addressed in commit 9badd49.

  • ab() now mirrors ucdavis/serodynamics/R/ab.R exactly (including bt() and the same piecewise formula/parenthesization).
  • get_timeindays_var() now mirrors ucdavis/serodynamics/R/get_timeindays.R exactly.
  • Added source-reference comments in R/utils_internal.R (file-level + ab() reference).
  • Removed unnecessary skip_if_not_installed() lines from test-model_comparison_table.R and test-compute_residual_metrics.R.
  • Added tests/testthat/test-utils_internal.R for internal utility coverage.
  • Checked R/ for purrr:: usage (none) and moved purrr back to Suggests.

Targeted test execution could not be run in this sandbox because Rscript is unavailable.

Screenshot: N/A (no UI changes; code/test-only update).

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to...

Re-checked and completed. The requested fixes from the prior comment are now applied in commit 9badd49 (including exact parity updates for ab() and get_timeindays_var(), test skip removals, new internal-utils tests, and purrr moved back to Suggests).

Screenshot: N/A (no UI changes; code/test-only update).

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

2 similar comments
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@Kwan-Jenny The model claude-sonnet-4.5 is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it.

You can try again without specifying a model (just @copilot) to use the default, or choose a different model from the model picker.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: f161c036-d343-476a-8f51-1649d9abb44d

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 69.91150% with 34 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
R/model_comparison_table.R 58.62% 24 Missing ⚠️
R/fig2_overall_newperson.R 0.00% 7 Missing ⚠️
R/compute_residual_metrics.R 85.00% 3 Missing ⚠️
Files with missing lines Coverage Δ
R/predict_posterior_at_times.R 100.00% <100.00%> (ø)
R/process_shigella_data.R 100.00% <100.00%> (ø)
R/utils_internal.R 100.00% <100.00%> (ø)
R/compute_residual_metrics.R 86.74% <85.00%> (ø)
R/fig2_overall_newperson.R 0.00% <0.00%> (ø)
R/model_comparison_table.R 29.56% <58.62%> (ø)

@Kwan-Jenny
Copy link
Copy Markdown
Collaborator

@copilot All 3 CI failures (docs-check, test-coverage, lint-changed-files) have the same root cause. The package fails to load because of namespace/export issues. Please fix both:


Error 1: undefined exports: postprocess_jags_output

The function postprocess_jags_output is listed in NAMESPACE as an export, but the actual R file (R/postprocess_jags_output.R) was deleted in PR #11. So when R tries to load the package, it finds an export declaration for a function that doesn't exist.

Fix:

  • Run roxygen2::roxygenise() to regenerate NAMESPACE from the current roxygen @export tags in R/*.R files. This will remove the stale postprocess_jags_output entry automatically.
  • Alternatively, manually remove the line export(postprocess_jags_output) from NAMESPACE.
  • Important: Check if there are any OTHER stale exports in NAMESPACE that reference deleted functions (e.g., create_incidence_table, create_noise_df, plot_summary_metrics, simulate_seroincidence, prepare_df_for_serocalculator, generate_final_table, create_xs_data). All of these were deleted in PR Reorganize repo into package-ready structure with pkgdown + mock data + tests #11. Remove all of them.

Error 2: 'mock_posterior_draws' is not an exported object from 'namespace:shigella'

The file R/data.R likely documents mock_posterior_draws and mock_case_data with @export tags, expecting the .rda files to exist in the data/ directory. But data-raw/mock_data.R (which generates these files via usethis::use_data()) was never actually executed, so the data/*.rda files don't exist in this branch.

Fix — choose one of these approaches:

Option A (Recommended): Run data-raw/mock_data.R so that data/mock_posterior_draws.rda and data/mock_case_data.rda are generated and committed to the repo. Then R/data.R will work correctly.

source("data-raw/mock_data.R")
# This creates data/mock_posterior_draws.rda and data/mock_case_data.rda
# Commit both .rda files

Option B: If you cannot run R scripts in this environment, create the data/ directory and generate minimal .rda files inline. The key requirement is that data/mock_posterior_draws.rda and data/mock_case_data.rda must exist and contain the objects described in R/data.R.


After fixing both:

  1. Make sure NAMESPACE is regenerated cleanly (no references to deleted functions).
  2. Make sure data/mock_posterior_draws.rda and data/mock_case_data.rda exist in the repo.
  3. Verify the package loads without errors: the equivalent of running devtools::load_all() should succeed.

These two fixes should resolve all 3 CI failures since they all stem from the package failing to load.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 30, 2026

@copilot All 3 CI failures (docs-check, test-coverage, lint-changed-files) have the same root cause. The package fails to load because of namespace/export issues. Please fix both:


...

Fixed in commit a274fff.

  • Removed stale namespace export issue by updating NAMESPACE (no postprocess_jags_output export remains; current exports now match existing R functions).
  • Removed stale man/postprocess_jags_output.Rd.
  • Generated and committed required mock data files:
    • data/mock_posterior_draws.rda
    • data/mock_case_data.rda
  • Updated ignore rules so those two .rda files are tracked (.gitignore, data/.gitignore).
  • Committed regenerated man/*.Rd files for current functions/datasets.

This addresses the shared package-load root cause seen in docs-check, test-coverage, and lint-changed-files.

Screenshot: N/A (no UI changes; code/package metadata/data files only).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cloud.r-project.org
    • Triggering command: /usr/lib/R/bin/exec/R /usr/lib/R/bin/exec/R --no-echo --no-restore -e install.packages(c(&#39;roxygen2&#39;,&#39;usethis&#39;),~&#43;~repos=&#39;REDACTED&#39;) get --global -data-downloader credential.usern/var/lib/dpkg/info/hicolor-icon-theme.postinst (dns block)
  • https://api.github.com/repos/UCD-SERG/serodynamics/contents/DESCRIPTION
    • Triggering command: /usr/lib/R/bin/exec/R /usr/lib/R/bin/exec/R --no-echo --no-restore -e remotes::install_github(&#39;UCD-SERG/serodynamics&#39;,~&#43;~upgrade=&#39;never&#39;,~&#43;~dependencies=FALSE);~&#43;~remotes::install_github(&#39;ucd-serg/serocalculator&#39;,~&#43;~upgrade=&#39;never&#39;,~&#43;~dependencies=FALSE) (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Fix package dependencies, remove ::: usage, add package infrastructure Fix package dependencies, namespace exports, and package infrastructure Mar 30, 2026
@Kwan-Jenny
Copy link
Copy Markdown
Collaborator

@copilot Good job! — the namespace/export issue is fixed and the package now installs successfully. Two remaining CI failures:

1. docs-check: DESCRIPTION out of sync

The "Check for changed files" step shows that running roxygen2::roxygenise() modifies DESCRIPTION. This means the committed DESCRIPTION doesn't match what roxygen generates. Please run devtools::document() and commit the updated DESCRIPTION and any other changed files (e.g., NAMESPACE, man/*.Rd).

2. test-coverage: testthat tests failing

The package installs fine now, but testthat.R is failing. Please check the actual test failure output — expand the "Show testthat output" step in the CI log if available, or run the tests locally:

devtools::test()

The most likely cause is one of these:

  • The new test-utils_internal.R tests may be failing if ab() or get_timeindays_var() are not exported or not found in the test environment. Since they are internal (@noRd), tests need to access them via shigella:::ab() or the functions need to be available through devtools::load_all().
  • The test-compute_residual_metrics.R tests may be failing now that the skip_if_not_installed() lines were removed — if serocalculator::get_values_var() is not available in CI.

Please fix all failing tests and then run devtools::document() to ensure DESCRIPTION/NAMESPACE/man files are in sync before committing.

…xygen note

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/47d8cc5c-e58f-4f67-a13f-b5f56651a62e

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 30, 2026

@copilot Good job! — the namespace/export issue is fixed and the package now installs successfully. Two remaining CI failures:

1. docs-check: DESCRIPTION out of sync

...

Addressed in commit e5d5155.

  • Fixed test-coverage failure by adding and exporting model_comparison_table() (tests were failing with could not find function "model_comparison_table").
  • Kept make_model_comparison_table() unchanged for backward compatibility.
  • Updated NAMESPACE and added man/model_comparison_table.Rd.
  • Updated DESCRIPTION RoxygenNote to 7.3.3 to match CI roxygen output and eliminate docs-check drift.

Screenshot: N/A (no UI changes; code/docs metadata only).

@Kwan-Jenny
Copy link
Copy Markdown
Collaborator

@copilot Two remaining failures:


1. docs-check: NAMESPACE still out of sync

The CI runs roxygen2::roxygenise() and then checks if any files changed. It found that NAMESPACE was modified, meaning the committed NAMESPACE doesn't match what roxygen generates.

Fix: Run devtools::document() locally and commit the updated NAMESPACE file. Do NOT manually edit NAMESPACE — let roxygen generate it.


2. lint-changed-files: Three categories of lint issues

A. .data pronoun warnings (object_usage_linter)

Almost all warnings are no visible binding for global variable '.data'. This happens in every file that uses dplyr with .data$column_name.

Fix: Add this line to R/utils_internal.R (or create R/shigella-package.R):

#' @importFrom rlang .data
NULL

Then run devtools::document() so this import is added to NAMESPACE. This single line fixes ALL the .data warnings across all files.

Also add for the antigen and iso variables in R/fig2_overall_newperson.R — if these are column names used in aes(), add at the top of that file:

utils::globalVariables(c("antigen", "iso"))

B. Line length warnings (line_length_linter)

Many lines exceed 80 characters in these files:

  • R/compute_residual_metrics.R
  • R/fig2_overall_newperson.R
  • R/model_comparison_table.R
  • R/predict_posterior_at_times.R
  • R/process_shigella_data.R
  • R/data.R

Fix: Break all lines longer than 80 characters. For roxygen comments, use line breaks. For code, break at pipe operators (|>), commas in function arguments, or + in ggplot chains.

C. Style issues in R/fig2_overall_newperson.R

  • commas_linter: Add a space after every comma (lines 30, 32, 54, 72)
  • undesirable_function_linter in R/compute_residual_metrics.R: Replace stop(...) with cli::cli_abort(...) (line 79)

D. Indentation issue in R/compute_residual_metrics.R

  • Line 119: indentation should be 13 spaces but is 6. Fix the alignment.

Summary

  1. Add @importFrom rlang .data to make .data pronoun recognized (fixes ~90% of lint warnings)
  2. Add utils::globalVariables(c("antigen", "iso")) in R/fig2_overall_newperson.R
  3. Break all lines > 80 characters in all R/ files listed above
  4. Fix comma spacing in R/fig2_overall_newperson.R
  5. Replace stop() with cli::cli_abort() in R/compute_residual_metrics.R
  6. Fix indentation on line 119 of R/compute_residual_metrics.R
  7. Run devtools::document() and commit the updated NAMESPACE and DESCRIPTION

Please do all of these in a single commit!

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/d635bda4-41b7-4feb-8fc3-330a60fec1ac

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 30, 2026

@copilot Two remaining failures:


...

Done in commit 7de2277.

  • Synced roxygen-managed files (NAMESPACE/man) by rerunning roxygen generation after changes.
  • Added @importFrom rlang .data in R/shigella-package.R so .data pronoun usage is recognized.
  • Added utils::globalVariables(c("antigen", "iso")) in R/fig2_overall_newperson.R.
  • Fixed requested style issues:
    • line wrapping to <= 80 chars in the listed files,
    • comma spacing in R/fig2_overall_newperson.R,
    • stop() -> cli::cli_abort() in R/compute_residual_metrics.R,
    • indentation alignment in R/compute_residual_metrics.R.
  • Added cli to DESCRIPTION Imports for cli::cli_abort().
  • Also fixed tidyselect deprecation warnings in model_comparison_table() by using string column names in dplyr::select().

Targeted verification run: tests/testthat/test-model_comparison_table.R passes locally.

Screenshot: N/A (no UI changes; code/lint/docs updates only).

@Kwan-Jenny
Copy link
Copy Markdown
Collaborator

@copilot

file: R/compute_residual_metrics.R
line: 137
expected: 6 spaces
found: 13 spaces

Please fix the indentation on this line to use 6 spaces instead of 13, consistent with the project’s linting rules.

Suggested fix:

  • Align the line with surrounding code blocks (likely inside a function or conditional).
  • Ensure consistent indentation across the file (multiples of 2 or project standard).

After correction, run:

lintr::lint_package()

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/6af850e8-229a-4ffd-a552-ff9467c5bbbb

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 30, 2026

@copilot

file: R/compute_residual_metrics.R
...

Fixed in commit 66bacb7.

The indentation in R/compute_residual_metrics.R at the pointwise dplyr::select(...) line is now adjusted to 6 spaces to match the project linting rule.

I attempted to run lintr::lint_package(), but Rscript is not available in this current session environment.

Screenshot: N/A (no UI changes; formatting-only update).

@Kwan-Jenny Kwan-Jenny marked this pull request as ready for review March 31, 2026 00:34
Copilot AI review requested due to automatic review settings March 31, 2026 00:34
@Kwan-Jenny Kwan-Jenny merged commit 9b608fe into manuscript Mar 31, 2026
7 checks passed
@Kwan-Jenny Kwan-Jenny deleted the copilot/sub-pr-11 branch March 31, 2026 00:37
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens the package’s dependency/namespace setup, removes reliance on non-exported upstream functions (:::) by adding internal equivalents, and adds package infrastructure (tests, mock datasets, CI, docs) to resolve package-load and CI failures.

Changes:

  • Updates DESCRIPTION/NAMESPACE/man/ to align exports/imports and fix package-load + docs-check issues.
  • Replaces serodynamics::: calls with internal implementations (R/utils_internal.R) and updates call sites.
  • Adds mock datasets, tests, a getting-started vignette/README updates, and GitHub Actions workflows for R CMD check + pkgdown.

Reviewed changes

Copilot reviewed 35 out of 38 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
vignettes/getting-started.Rmd New vignette introducing package workflows using mock data.
tests/testthat/test-utils_internal.R Adds unit tests for internal helpers (ab, get_timeindays_var).
tests/testthat/test-process_shigella_data.R Adds tests for reshaping/filtering logic.
tests/testthat/test-predict_posterior_at_times.R Adds tests for posterior prediction helper output structure.
tests/testthat/test-model_comparison_table.R Adds tests for comparison-table deltas and percent improvements.
tests/testthat/test-compute_residual_metrics.R Adds tests for residual metrics on original/log scales.
tests/testthat.R Standard testthat entrypoint for running package tests.
README.Rmd Expands README with overview, install, usage, and documentation links.
R/utils_internal.R Adds internal replacements for upstream non-exported functions.
R/shigella-package.R Imports rlang::.data to satisfy NSE/linting patterns.
R/process_shigella_data.R Removes unused column selection and formats roxygen/code for lint.
R/predict_posterior_at_times.R Stops using serodynamics::: and pivots/expands time grid more readably.
R/model_comparison_table.R Adds exported model_comparison_table() for compact comparison summaries.
R/fig2_overall_newperson.R Stops using serodynamics::: and registers global variables for NSE.
R/data.R Adds roxygen docs for included mock datasets.
R/compute_residual_metrics.R Replaces serodynamics::: time-var accessor with internal helper; improves formatting/errors.
NAMESPACE Regenerates exports/imports to match current roxygen output.
man/utils_internal.Rd Documents internal utilities topic.
man/process_shigella_data.Rd New/updated Rd for process_shigella_data().
man/prep_newperson_params.Rd New/updated Rd for prep_newperson_params().
man/predict_posterior_at_times.Rd New/updated Rd for posterior prediction helper.
man/postprocess_jags_output.Rd Removes stale Rd for removed export.
man/model_comparison_table.Rd New Rd for model_comparison_table().
man/mock_posterior_draws.Rd Adds dataset documentation.
man/mock_case_data.Rd Adds dataset documentation.
man/make_model_comparison_table.Rd Adds/updates Rd for model comparison helper.
man/fmt_mci.Rd Adds/updates Rd for formatting helper.
man/fig2_overall_newperson.Rd Adds/updates Rd for plotting function.
man/compute_residual_metrics.Rd Adds/updates Rd for residual metrics function.
DESCRIPTION Updates Imports/Suggests/config for runtime deps + testthat edition.
data/mock_posterior_draws.rda Adds tracked mock posterior draws data.
data/mock_case_data.rda Adds tracked mock case dataset.
data/.gitignore Allows tracking the two mock .rda files while ignoring others.
data-raw/mock_data.R Adds generator script for mock datasets.
.Rbuildignore Ignores dev/data-raw/quarto artifacts in package build.
.gitignore Stops blanket-ignoring data/ so mock .rda can be committed.
.github/workflows/R-CMD-check.yaml Adds R CMD check workflow (release + devel).
.github/workflows/pkgdown.yaml Adds pkgdown build + GitHub Pages deploy workflow.
Comments suppressed due to low confidence (1)

R/compute_residual_metrics.R:87

  • time_var/value_var are character scalars (derived from attributes), but rename(t = {{ time_var }}, obs = {{ value_var }}) uses {{ }} (curly-curly) which quotes the symbol time_var rather than using its string value. This will attempt to rename columns literally named time_var/value_var and will fail for real case_data inputs (including the mock dataset in tests). Use dplyr::rename(t = dplyr::all_of(time_var), obs = dplyr::all_of(value_var)) (or !!rlang::sym(time_var) / !!rlang::sym(value_var)) instead.
  time_var  <- get_timeindays_var(dataset)
  value_var <- serocalculator::get_values_var(dataset)

  observed_data <- dataset |>
    dplyr::rename(t = {{ time_var }}, obs = {{ value_var }}) |>
    dplyr::select(dplyr::all_of(c("id", "t", "obs", "antigen_iso"))) |>

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1 to +34
test_that("ab() returns y0 at t = 0", {
result <- ab(t = 0, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
expect_equal(result, 200)
})

test_that("ab() returns y1 at t = t1", {
result <- ab(t = 10, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
expect_equal(result, 2000)
})

test_that("ab() shows expected rise for t < t1", {
result <- ab(t = 5, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
expected <- 200 * exp((log(2000 / 200) / 10) * 5)
expect_equal(result, expected)
})

test_that("ab() decays toward zero for large t (shape > 1)", {
result <- ab(t = 10000, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
expect_true(is.finite(result))
expect_true(result >= 0)
})

test_that("ab() handles vector input", {
times <- c(0, 5, 10, 30, 90)
result <- ab(t = times, y0 = 200, y1 = 2000, t1 = 10, alpha = 0.02, shape = 1.2)
expect_length(result, 5)
expect_true(all(is.finite(result)))
expect_equal(result[1], 200)
expect_equal(result[3], 2000)
})

test_that("ab() does not error when t1 = 0", {
expect_no_error(ab(t = c(0, 10, 30), y0 = 200, y1 = 2000, t1 = 0, alpha = 0.02, shape = 1.2))
})
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

These tests call ab() directly, but ab is an internal (non-exported) function, so it won’t be attached by library(shigella) and the symbol lookup will fail during R CMD check. Access it via getFromNamespace("ab", "shigella") (or shigella:::ab within tests) before calling it.

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +45
test_that("get_timeindays_var() finds time variable from attribute", {
mock_data <- data.frame(timepoint = c(0, 7, 30), value = c(100, 200, 300))
attr(mock_data, "timeindays") <- "timepoint"
expect_equal(get_timeindays_var(mock_data), "timepoint")
})

test_that("get_timeindays_var() defaults to timeindays when attribute missing", {
mock_data <- data.frame(timeindays = c(0, 7, 30), value = c(100, 200, 300))
expect_equal(get_timeindays_var(mock_data), "timeindays")
})
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

get_timeindays_var() is also internal (not exported), so calling it unqualified will fail when only library(shigella) is loaded in tests/testthat.R. Consider retrieving it with getFromNamespace("get_timeindays_var", "shigella") (or shigella:::get_timeindays_var) inside the test before use.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +26
test_that("predict_posterior_at_times returns expected structure", {
# Create minimal mock posterior data
mock_model <- data.frame(
Subject = rep("test_id", 15),
Iso_type = rep("IgG", 15),
Chain = rep(1, 15),
Iteration = rep(1:3, 5),
Parameter = rep(c("y0", "y1", "t1", "alpha", "shape"), each = 3),
value = c(
200, 210, 190, # y0
2000, 2100, 1900, # y1
10, 12, 9, # t1
0.02, 0.025, 0.015, # alpha
1.0, 1.1, 0.9 # shape
)
)

times <- c(0, 30, 90)

result <- predict_posterior_at_times(
model = mock_model,
ids = "test_id",
antigen_iso = "IgG",
times = times
)

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

These tests call predict_posterior_at_times() directly, but the function is not exported (it’s marked internal and is absent from NAMESPACE), so it won’t be on the search path after library(shigella) and the tests will error. Access it via getFromNamespace("predict_posterior_at_times", "shigella") (or shigella:::predict_posterior_at_times) before invoking it.

Copilot uses AI. Check for mistakes.
Comment thread DESCRIPTION
RoxygenNote: 7.3.3
Imports:
cli,
dplyr,
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The package uses the .by argument in dplyr::summarise() (e.g., compute_residual_metrics() and fig2_overall_newperson()), which requires dplyr >= 1.1.0. To avoid installation/runtime failures on environments with older dplyr, add a minimum version constraint in Imports (e.g., dplyr (>= 1.1.0)).

Suggested change
dplyr,
dplyr (>= 1.1.0),

Copilot uses AI. Check for mistakes.
Comment thread DESCRIPTION
testthat (>= 3.0.0),
tidyverse
Depends:
R (>= 3.5)
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The package code uses the base pipe operator |> throughout R/ (e.g., compute_residual_metrics(), process_shigella_data(), etc.), which requires R >= 4.1 to parse. The Depends: R (>= 3.5) entry is therefore inaccurate and will cause installation failures on R < 4.1. Either bump the minimum R version in DESCRIPTION (e.g., R (>= 4.1)) or replace |> with a pipe supported on older R versions (e.g., %>%).

Suggested change
R (>= 3.5)
R (>= 4.1)

Copilot uses AI. Check for mistakes.
Kwan-Jenny added a commit that referenced this pull request Apr 8, 2026
… + tests (#11)

* Delete R/create_incidence_table.R

* Delete R/create_noise_df.R

* Delete R/plot_summary_metrics.R

* Delete R/simulate_seroincidence.R

* Delete R/prepare_df_for_serocalculator.R

* Delete R/postprocess_jags_output.R

* Delete R/generate_final_table.R

* Delete R/create_xs_data.R

* Delete R/process_shigella_data.R

* Add process_shigella_data function for data reshaping

This function filters and reshapes longitudinal Shigella data for serodynamics workflows, standardizing column names and preparing the dataset for conversion.

* Add function to summarize antibody trajectories for newperson

This function summarizes population-level antibody trajectories for a hypothetical new individual, computing median and credible intervals. It allows customization of various parameters for plotting and data return options.

* Add function to predict posterior at specified times

This function generates posterior predictions of antibody trajectories at specified time points for selected subjects and antigen/isotype. It processes posterior draws and evaluates the antibody curve using specified parameters.

* Add compute_residual_metrics function for antibody data

This function computes residuals between observed antibody measurements and posterior median predictions, returning pointwise residuals or aggregated error metrics (MAE, RMSE, SSE) at various summary levels.

* Add model comparison function with residual metrics

This function computes and compares residual metrics for serotype-specific and overall models, providing insights on their performance differences.

* Add fmt_mci function to format median and intervals

* Add function to extract and summarize newperson parameters

This function processes posterior draws for the 'newperson' subject, filtering and reshaping the data into a wide format suitable for analysis.

* Update R/fig2_overall_newperson.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update R/prep_newperson_params.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update R/model_comparison_table.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update R/compute_residual_metrics.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update R/process_shigella_data.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix package dependencies, namespace exports, and package infrastructure (#12)

* Initial plan

* Fix DESCRIPTION dependencies and remove ::: usage

- Move tibble, tidyr, purrr from Suggests to Imports
- Add serocalculator and rlang to Imports
- Remove unused cohort_name selection in process_shigella_data
- Replace serodynamics::: calls with internal helper functions
- Add utils_internal.R with get_timeindays_var() and ab() implementations

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Add package infrastructure: tests, workflows, vignettes, and mock data

- Add testthat tests for main functions
- Add R CMD check and pkgdown GitHub Actions workflows
- Add data-raw/ with mock data generation script
- Add R/data.R with mock data documentation
- Add getting-started vignette
- Update README.Rmd with package overview and usage
- Update .Rbuildignore to exclude non-package files

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix rlang::rnorm() to rnorm() in mock data generation

Use base R rnorm() instead of non-existent rlang::rnorm()

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Add permissions block to R-CMD-check workflow

Set explicit GITHUB_TOKEN permissions (contents: read) for security

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Update documentation to remove reference to serodynamics:::ab()

Update comment to reflect internal implementation instead of external ::: call

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Align internal utils with serodynamics and update tests

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>
Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/d36469e9-e5db-41bb-ad8f-cc452230b453

* Fix namespace export errors and add mock data .rda files

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/1327221c-702c-4c06-907f-1b381d75ecd9

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix failing tests by adding model_comparison_table export and sync roxygen note

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/47d8cc5c-e58f-4f67-a13f-b5f56651a62e

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix remaining docs-check and lint CI issues

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/d635bda4-41b7-4feb-8fc3-330a60fec1ac

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix compute_residual_metrics indentation at line 137

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/6af850e8-229a-4ffd-a552-ff9467c5bbbb

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix 5 CI failures: version bump, LICENSE, non-ASCII chars, VignetteBuilder, unused Imports, WORDLIST, line lengths

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/be474f7c-718f-491f-84a7-bf1b17ecce18

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Update man/fmt_mci.Rd to match roxygen docs (devtools::document() sync)

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/a9ff4314-f41a-414a-955e-d0c37e3e3dd8

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix review feedback: rlang::sym rename, t1=0 edge case, shigella::: in tests, TODO comment

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/a030fe7b-7a02-4d32-a346-01e663e62bad

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Restore ab() and bt() to exact upstream implementations; update t1=0 test

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/40bd4f82-f569-4387-98be-3bfec61029b2

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Update data-raw/mock_data.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix roxygen docs: give bt() its own params, move trajectory docs to ab()

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/9c7dfb28-b082-4292-98b7-37fbefb0b36b

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix Github -> GitHub capitalization in inst/WORDLIST

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/be13d809-cfdc-485c-889c-92a9d2ebd9c2

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

* Fix Github -> GitHub capitalization in NEWS.md

Agent-Logs-Url: https://github.com/UCD-SERG/shigella/sessions/9263f1b7-8152-4f90-9f85-e2bb428c4798

Co-authored-by: Kwan-Jenny <68584166+Kwan-Jenny@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants