Skip to content

Sample size em edits - #369

Open
d-morrison wants to merge 15 commits into
mainfrom
sample-size-em-edits
Open

Sample size em edits#369
d-morrison wants to merge 15 commits into
mainfrom
sample-size-em-edits

Conversation

@d-morrison

Copy link
Copy Markdown
Member

Thank you for taking the time to submit a pull request!

To maximize the chances of acceptance:

  • The title of your PR should briefly describe the change.
  • The body of your PR should contain Fixes #issue-number (if relevant).
  • Commit/merge messages to be included in NEWS.md should begin with -.
  • Code should follow the tidyverse style guide.
  • Documentation should use roxygen2, with Markdown syntax.
  • Contributions should include unit tests (using testthat).

For more information see Contributing.

@codecov

codecov Bot commented Feb 4, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@d-morrison
d-morrison marked this pull request as ready for review May 19, 2026 02:40
Copilot AI review requested due to automatic review settings May 19, 2026 02:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 adds a new data-raw/ script to run parallelized seroincidence simulations for exploring sample-size behavior, and updates package metadata to include additional optional dependencies used by supporting workflows/scripts.

Changes:

  • Add data-raw/sample-size.R to simulate cross-sectional serology data and summarize CI coverage across scenarios.
  • Add furrr and svglite to Suggests in DESCRIPTION (for optional parallel mapping and SVG-based visual regression testing support).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
DESCRIPTION Adds optional dependencies (furrr, svglite) needed by scripts/testing tools.
data-raw/sample-size.R New simulation + summarization script for evaluating sample size / CI coverage under different incidence rates.

Comment thread data-raw/sample-size.R Outdated
Comment thread data-raw/sample-size.R Outdated
Comment thread data-raw/sample-size.R Outdated
Comment thread data-raw/sample-size.R
Comment thread data-raw/sample-size.R Outdated
Comment thread data-raw/sample-size.R Outdated
github-actions Bot added a commit that referenced this pull request May 19, 2026
@claude

This comment has been minimized.

github-actions Bot added a commit that referenced this pull request Jun 3, 2026
@claude

This comment has been minimized.

- Replace deprecated load_curve_params/sim.cs/est.incidence with
  load_sr_params/sim_pop_data/est_seroincidence (and sr_params arg)
- Rename objects with dots to snake_case (object_name_linter)
- Fix mislabeled lambda=0.01 scenario and add per-call seeds
- Use seq_along(); declare future in Suggests; move library(future) to top

Copy link
Copy Markdown
Member Author

Addressed the review findings in data-raw/sample-size.R:

  • Critical/deprecated API: replaced load_curve_params()load_sr_params(), sim.cs()sim_pop_data() (renamed n.smpl/age.rng/n.mc/renew.params/add.noisen_samples/age_range/n_mcmc_samples/renew_params/add_noise), and est.incidence(curve_params=)est_seroincidence(sr_params=) (fixes the missing-sr_params runtime error).
  • snake_case: renamed results_100_0.1/results_100_0.01/final_table_100_0.1/ft_100_0.01 to underscore forms and updated all references.
  • Logic fix: the λ=0.01 scenario now actually passes lambda = 0.01.
  • Reproducibility: added set.seed() before the two unseeded runs.
  • Minor: seq_along() instead of 1:length(), moved library(future) to the top, and declared future in Suggests.

This is an internal maintainer script in data-raw/ (not user-visible / not installed), so I applied the no changelog + no version increment labels for the version-check and changelog gates, and added gh to WORDLIST to fix spellcheck.

@claude review


Generated by Claude Code

@claude

This comment has been minimized.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

👀 Picked up by workflow run #26910326827. R setup runs first; Claude itself responds after that.

github-actions Bot added a commit that referenced this pull request Jun 3, 2026

Copy link
Copy Markdown
Member Author

@claude review


Generated by Claude Code

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

👀 Picked up by workflow run #26911728637. R setup runs first; Claude itself responds after that.

Copy link
Copy Markdown
Member Author

Working on this — paws off until I'm done.


Generated by Claude Code

claude added 2 commits June 30, 2026 19:09
- Use seq_len(n_sim) instead of 1:n_sim to avoid the reverse-sequence
  bug when n_sim = 0.
- Stop discarding the lambda=0.1/0.01 coverage results; assign and
  print all four scenarios consistently instead of relying on
  paste() on a data frame, which produced an unreadable message.
@claude

This comment has been minimized.

Copy link
Copy Markdown
Member Author

ARD summary

This PR adds a maintainer-only simulation script (data-raw/sample-size.R) for exploring CI coverage of est_seroincidence() across sample sizes/incidence rates, plus furrr/future/svglite in Suggests. All findings from the original Copilot review (deprecated API calls, snake_case violations, a mislabeled λ scenario, missing seeds) were already Addressed in earlier commits on this branch and confirmed resolved by a follow-up Claude review on 2026-06-03. That review left two minor open items, both now addressed in this session:

Finding Disposition
1:n_sim in furrr::future_map() is fragile when n_sim = 0 (reverse sequence) Addressed — changed to seq_len(n_sim)
λ=0.1 / λ=0.01 coverage results computed but discarded; paste() called on a data frame produced an unreadable message Addressed — all four coverage tables (n=50, n=100 λ=0.2/0.1/0.01) are now assigned and printed consistently via cat() + print()

Additionally, since this PR was opened in Feb 2025 and main had drifted 59 commits ahead:

  • Merged main into this branch. One real conflict, in DESCRIPTION's Version: field — resolved by taking main's value.
  • That left the dev version equal to (not greater than) main's, which fails the version-check CI gate, so bumped to 1.4.1.9001.

Pushed as commits 55b46060 (merge), a9b3ff7a (nits), 9ff9ed2c (version bump).

CI status: still running/pending as of last check — not yet confirmed fully green. Will need a follow-up check before this is mergeable.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Done for now — unclaimed.


Generated by Claude Code

@claude

This comment has been minimized.

github-actions Bot added a commit that referenced this pull request Jun 30, 2026

Copy link
Copy Markdown
Member Author

Working on this (resolving the conflict with main, addressing the two remaining nits, and tidying stale threads) --- paws off until I'm done.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Merged main into the branch and drove this to a clean state:

  • Resolved the main conflict (DESCRIPTION Version: only) and bumped the dev version to 1.4.1.9005 so it stays above main's 1.4.1.9004 (version-check parity).
  • Addressed the two remaining review nits in data-raw/sample-size.R: corrected the stale # Loop through each of the 100 results comment (the loop is sample-size-agnostic), and renamed the lambda() helper to get_lambda() so it no longer shadows the lambda value used throughout the script.
  • Resolved all 6 stale inline threads from the original Copilot review --- the deprecated-API and dotted-object-name items were all superseded by later commits (object names already use underscores; lint CI is green).

Review verdict was already clean. Leaving the merge decision to a human.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Heads-up: after the merge + version bump I pushed, lint-changed-files will fail on data-raw/sample-size.R --- but that's the repo-wide lintr 3.4.0 indentation_linter regression (#558), not a defect in this PR. This PR is otherwise clean and is blocked pending the lint fix in #558.


Generated by Claude Code

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code Review (confirming pass)

  • Diff current HEAD (888b1617) against the last-reviewed commit (a9b3ff7, prior verdict)
  • Verify no regressions were introduced by the main merge
  • Re-check function signatures cited in the script against current R/ source (hallucination check)
  • Confirm CI status

Summary

The only changes since the 2026-06-30 review are a merge of main into the branch and a DESCRIPTION version bump (1.4.1.90041.4.1.9005) for version-check parity. data-raw/sample-size.R itself was also improved beyond that review: both optional nits it raised are now fixed —

  • the stale # Loop through each of the 100 results comment is corrected to describe the loop generically, and
  • the lambda() helper (which shadowed the lambda value used elsewhere) is renamed to get_lambda().

That prior verdict — all critical/blocking findings resolved, script in good shape — still stands, and the remaining nits from that round are now also closed out.

Independently re-verified for this pass (not just trusting changelog):

  • est_seroincidence(pop_data=, sr_params=, noise_params=, lambda_start=, build_graph=, verbose=, antigen_isos=, ...) — matches current signature in R/est_seroincidence.R.
  • sim_pop_data(lambda=, n_samples=, age_range=, antigen_isos=, n_mcmc_samples=, renew_params=, add_noise=, curve_params=, noise_limits=, format=, ...) — matches current signature in R/sim_pop_data.R.
  • load_sr_params(file_path, antigen_isos = NULL) — matches current signature in R/load_sr_params.R.
  • No dotted object names remain; all identifiers are valid snake_case.
  • set.seed() precedes all four simulate_seroincidence() calls; seq_len()/seq_along() used throughout (no reverse-sequence risk).
  • furrr, future, svglite correctly declared in Suggests; data-raw/ remains .Rbuildignored.
  • PR carries the no changelog label, so the missing NEWS.md bullet is expected/exempt.

CI status: All required checks are green — lint-changed-files, Spellcheck, version-check, docs-check, Check Changelog Action, test-coverage, and all R-CMD-check matrix jobs (ubuntu/macos/windows release) are ✅. (Note: an earlier comment on this thread flagged a possible lint-changed-files failure from a repo-wide lintr regression — that check is currently passing, so it's not blocking.)

No new issues found.

Verdict

Ready for merge

@github-actions

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Posted a confirming review on PR #369: comment.

Summary: Since the last review round (2026-06-30), the only changes are a main merge and a version bump — the two optional nits from that review (stale comment, lambda() shadowing) have since been fixed by the author. I independently re-verified the est_seroincidence(), sim_pop_data(), and load_sr_params() calls against current R/ source (no hallucinated APIs), confirmed no dotted object names remain, and checked that all CI checks are currently green.

Verdict: Ready for merge

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $1.0235 (review) — run

github-actions Bot added a commit that referenced this pull request Jul 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📖 https://UCD-SERG.github.io/serocalculator/preview/pr369/
Preview documentation for this PR (at commit 888b161)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants