No default priors

[sschildhauer]

Taking away default priors and forcing users to input their priors manually. Changing documentation accordingly including examples. Priors are now listed as NULL in the input for prep_priors().
Prep_priors was also changed to correspond with the input decay type -- 5 for power and 4 for exponential for mu_hyp, prec_hyp, and omega.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
R/prep_priors.R	92.85%	1 Missing ⚠️

Files with missing lines	Coverage Δ
R/run_serodynamics.R	100.00% <100.00%> (ø)
R/prep_priors.R	91.83% <92.85%> (+4.33%)	⬆️

[sschildhauer]

@claude, review

[sschildhauer]

@claude, review

[github-actions]

👀 Claude Review Dispatched — run reviewing PR #284 at 4908763.

[github-actions]

Claude finished review — View run

---

Review: #284 — "No default priors"

This is a follow-up review. Since the prior review (commit 3eda3b4), the author pushed several commits that address most of the earlier findings. I re-verified each against the current head, 4908763 (490876385da2a878ee90d47919dc19e37c89449c).

Previously-flagged issues: status

#1 — prec_hyp_param validation bug — Fixed. R/prep_priors.R now checks length(prec_hyp_param) != 5 instead of length(mu_hyp_param) != 5. Verified in the diff and covered by the new test "Expect error when only some supplied" (tests/testthat/test-prep_priors.R), which omits prec_hyp_param and asserts an error.

#2 — vignettes/articles/getting-started.qmd will fail to render — Only partially fixed (still blocking).
The first run_serodynamics() call (run-model chunk, now around line 135) was updated with explicit prior arguments. However, the second call in the stratified-example chunk was not:

vignettes/articles/getting-started.qmd:218-227

fitted_stratified <- run_serodynamics(
  data = stratified_data,
  file_mod = serodynamics_example("model.jags"),
  nchain = 2,
  nadapt = 100,
  nburn = 100,
  nmc = 10,
  niter = 20,
  strat = "pathogen"  # Specify stratification variable
)

No prior arguments are supplied and there is no eval: false on this chunk. With defaults removed from prep_priors(), this call will still hit cli::cli_abort("Need to specify 5 priors for {.arg mu_hyp_param}") at render time, so the vignette still fails to build end-to-end. This is the same finding as before — one of the two originally-cited call sites (line ~213-222 in the prior review) was missed by the fix.

#3 — Fixture/data-raw regeneration scripts — Fixed. tests/testthat/fixtures/make-example-runjags-inputs.R, tests/testthat/fixtures/make_example_runjags_output.R, and data-raw/nepal_sees_jags_output.R all now pass explicit priors matching the old defaults.

#4 — No test coverage for mandatory-argument behavior — Fixed. Two new tests added in tests/testthat/test-prep_priors.R: "Expect error when not supplied" and "Expect error when only some supplied". The latter specifically exercises (and would have caught) finding #1.

#5 — Stale roxygen @description — Fixed. R/prep_priors.R now reads "Priors must be specified as an option in run_serodynamics().", consistent with the run_serodynamics() roxygen text.

#6 — NEWS.md entry undersells breaking change — Partially fixed. The entry was moved from ## Internal to ## New features as suggested. Two smaller points from the original finding remain open, both minor:

• It's still not explicitly labeled **Breaking change:** the way the NEWS.md:104 precedent does.
• New issue introduced by this edit: NEWS.md:76 — * Default priors are no longer supplied forrun_serodynamics(). Users must is missing a space between "for" and the opening backtick. In CommonMark, a code span doesn't require surrounding whitespace to open, so this renders as "for" immediately butted up against the code-styled run_serodynamics() with no gap between them (reads as "forrun_serodynamics()" visually) — a genuine rendering defect, not just a source-line-wrap nit.

Not flagged (checked, no issue)

• man/*.Rd files regenerated consistently with the roxygen source changes.
• New test "Expect error when only some supplied" correctly targets the prec_hyp_param code path via the pipe-into-expect_error() idiom — this works correctly under R's lazy argument evaluation / promise semantics (same pattern the existing "priors are modifiable" test already used with expect_snapshot_value()), so it is not a false test.
• All updated example/test/fixture prior vectors match the original removed defaults exactly — no drift.
• No .data-in-tidyselect, branching-only-varies-columns, merge()-vs-join, or missing relationship= issues.

ℹ️ Inline comments were unavailable (not surfaced as a tool in this environment); findings appear above with path:line anchors.

Verdict

Needs more work — finding #2 (the stratified-example chunk in vignettes/articles/getting-started.qmd:218-227) is still open and blocking: the vignette will fail to render because that run_serodynamics() call has no priors. This should be fixed the same way the first call in the vignette was. The remaining NEWS.md items are minor and can be folded into the same pass.

Reviewed commit: 4908763

[github-actions]

💰 Cost: $0.7772 (review) — run

[Copilot]

Pull request overview

This PR removes default prior values from the modeling workflow and updates package docs/examples/tests so users must explicitly provide priors when running run_serodynamics() (and prep_priors() now defaults prior inputs to NULL).

Changes:

• Removed default prior arguments from prep_priors() and updated tests/fixtures to supply explicit priors.
• Updated user-facing examples (vignette + inst/examples) and Rd docs to reflect the new “no default priors” behavior.
• Added/updated NEWS + bumped package version.

Reviewed changes

Copilot reviewed 15 out of 18 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
vignettes/articles/getting-started.qmd	Updates getting-started examples to pass priors explicitly.
tests/testthat/test-runjags.R	Updates test to call prep_priors() with explicit prior vectors.
tests/testthat/test-run_serodynamics.R	Updates integration-style tests to pass priors explicitly into run_serodynamics().
tests/testthat/test-prep_priors.R	Removes snapshot of default priors; adds tests for erroring when priors omitted and for exponential priors.
tests/testthat/fixtures/make-example-runjags-inputs.R	Updates fixture generator to supply explicit priors.
tests/testthat/fixtures/make_example_runjags_output.R	Updates fixture generator to supply explicit priors.
tests/testthat/_snaps/prep_priors.md	Updates snapshots to reflect changed prep_priors() behavior.
R/run_serodynamics.R	Updates roxygen text to state priors must be specified.
R/prep_priors.R	Removes default priors (defaults now NULL), adds decay_type arg, and updates validation/docs.
NEWS.md	Documents the breaking change (no default priors).
man/run_serodynamics.Rd	Regenerated docs/examples to include explicit prior arguments.
man/prep_priors.Rd	Regenerated docs to reflect new prep_priors() signature and guidance.
man/postprocess_jags_output.Rd	Regenerated docs/examples to include explicit priors.
inst/examples/run_serodynamics-examples.R	Updates examples to pass priors explicitly.
inst/examples/postprocess_jags_output-examples.R	Updates examples to pass priors explicitly.
inst/examples/examples-prep_priors.R	Removes example that relied on default priors.
DESCRIPTION	Bumps development version.
data-raw/nepal_sees_jags_output.R	Updates data-raw script to pass priors explicitly.

Files not reviewed (3)

• man/postprocess_jags_output.Rd: Generated file
• man/prep_priors.Rd: Generated file
• man/run_serodynamics.Rd: Generated file

Suppressed comments (2)

R/prep_priors.R:29

• The omega_param documentation currently says both that exponential uses 4 values and that the vector must be 5 values long, and it also has a ).. typo. The length requirements should be stated consistently (5 for power, 4 for exponential).

#' @param omega_param A [numeric] [vector] of 5 values corresponding to the
#' diagonal entries representing the Wishart hyperprior
#' distributions of `prec_hyp_param`, describing how much we expect parameters
#' to vary between individuals (4 values 
#' when `decay_type == "exponential"`)..
#' Must be 5 values long corresponding to the 5 estimated parameters.

tests/testthat/test-prep_priors.R:37

• Similar to the test above: expect_error() without checking the message can mask regressions (e.g., failing earlier for the wrong reason). Consider asserting the specific exponential-length error (4 priors for mu_hyp_param).

test_that("Expect error for mu.hyp.param under exponential", {
  prep_priors(max_antigens = 2, 
              mu_hyp_param = c(1.0),
              prec_hyp_param = c(0.01),
              omega_param = c(1.0),
              wishdf_param = 15,
              prec_logy_hyp_param = c(4.0, 1.0),
              decay_type = "exponential") |>
    expect_error()
})

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[sschildhauer]

@claude, please review and recommend changes regarding the failing R-CMD-check from windows.

[imelainew]

Looks good! Ready for Ezra from my side.

[sschildhauer]

Hi @d-morrison, this PR is ready for your review. It is not passing the windows R-CMD check, but it looks like it is a rjags installation issue that may require workflow changes, which I am hesitant to jump into myself. Thank you for taking a look.

[imelainew]

I have a question about order right: y0, y1, t1, alpha, shape, and it matches model.jags where position 4 is alpha and position 5 is log(shape minus 1). But the roxygen for mu_hyp_param above lists it as y0, y1, t1, r, alpha, which swaps the last two, and then the sentence below that says exponential uses y0, y1, t1, alpha, contradicting its own list. That ordering was already wrong on main, but it matters more now that exponential requires exactly 4 values: someone following the roxygen list will pass the shape prior as their fourth value and it will be used as the prior on log alpha with no error. Worth fixing the roxygen to match this line while you're in here.
Two smaller things in the same block: the exponential sentence says 5 parameters where it should say 4, and the returns section still describes n_params as 5 and the array dimensions as max_antigens by 5.