Skip to content

No default priors - #284

Open
sschildhauer wants to merge 53 commits into
mainfrom
no_default_priors
Open

No default priors#284
sschildhauer wants to merge 53 commits into
mainfrom
no_default_priors

Conversation

@sschildhauer

@sschildhauer sschildhauer commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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 Bot commented Aug 11, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
61 1 60 0
View the top 1 failed test(s) by shortest run time
prep_priors::Omit_omega_param_under_power_decay
Stack Traces | 0.029s run time
Error in `prep_priors(max_antigens = 2, mu_hyp_param = c(1, 5, 0, -2, -1), prec_hyp_param = c(0.01, 0.01, 0.01, 0.01), wishdf_param = 15, prec_logy_hyp_param = c(4, 1))`: Need to specify 5 priors for `prec_hyp_param`
Backtrace:
    x
 1. +-testthat::expect_error(...) at test-prep_priors.R:42:3
 2. | \-testthat:::expect_condition_matching_(...)
 3. |   \-testthat:::quasi_capture(...)
 4. |     +-testthat (local) .capture(...)
 5. |     | \-base::withCallingHandlers(...)
 6. |     \-rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 7. \-serodynamics::prep_priors(...)
 8.   \-cli::cli_abort("Need to specify 5 priors for {.arg prec_hyp_param}") at serodynamics/R/prep_priors.R:91:5
 9.     \-rlang::abort(...)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@sschildhauer

Copy link
Copy Markdown
Collaborator Author

@claude, review

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sschildhauer
sschildhauer requested a review from imelainew August 21, 2026 18:04
@sschildhauer

Copy link
Copy Markdown
Collaborator Author

@claude, review

@github-actions

This comment has been minimized.

@github-actions
github-actions Bot removed the request for review from imelainew August 21, 2026 18:10
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment thread R/prep_priors.R
Comment on lines -98 to +109
n_params <- 5 # Assuming 5 model parameters [ y0, y1, t1, alpha, shape]
n_params <- if (decay_type == "power") 5L else 4L

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.

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.

@d-morrison

Copy link
Copy Markdown
Member

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.

@sschildhauer fixed!

@d-morrison

Copy link
Copy Markdown
Member

/review

@github-actions
github-actions Bot removed the request for review from d-morrison August 31, 2026 09:00
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
github-actions Bot requested a review from d-morrison August 31, 2026 09:05
@d-morrison

Copy link
Copy Markdown
Member

Claude finished reviewView run

Confirmed. Now I have everything needed to write the review.

Review: #284 — "No default priors"

Follow-up review. Since the prior round (commit 4908763), the author pushed the commit range 49087638..e2157ae2 (24 commits), which primarily adds exponential-decay-aware prior validation to prep_priors() and hardens the CI JAGS-install steps. I re-verified the file contents directly and confirmed the previously-open finding.

Previously-flagged issues: status

#2vignettes/articles/getting-started.qmd stratified-example call missing priors — Fixed. vignettes/articles/getting-started.qmd:220-234 now supplies all five explicit prior arguments to the fitted_stratified call, matching the fix already applied to the earlier run-model chunk. Both run_serodynamics() calls in the vignette now have explicit priors, so the vignette should render end-to-end again.

#6 (remaining half) — NEWS.md wording/labeling. NEWS.md:5-7 now carries an explicit **Breaking change:** label (matching the NEWS.md:104-style precedent) and the earlier missing-space typo before the backtick is fixed. This closes out both outstanding sub-points from the prior round.

New findings (introduced in 49087638..e2157ae2)

1. (Blocking) prep_priors() silently accepts an invalid decay_type, bypassing all length validation — R/prep_priors.R:78-96,109 The validation for mu_hyp_param/prec_hyp_param/omega_param is now written as two mutually exclusive branches, gated on decay_type == "power" and decay_type == "exponential" respectively — with no else/default branch and no match.arg()/arg.match() call on decay_type itself:

if (length(mu_hyp_param) != 5 && decay_type == "power") {
  cli::cli_abort(...)
} else if (length(mu_hyp_param) != 4 && decay_type == "exponential") {
  cli::cli_abort(...)
}

and later:

n_params <- if (decay_type == "power") 5L else 4L

If decay_type is anything other than the exact strings "power"/"exponential" (a typo, wrong case, trailing whitespace, NULL, etc.), neither cli_abort() branch fires regardless of the supplied prior lengths — the length checks are simply skipped — and n_params falls through to 4L (the "exponential" branch) via the bare else. A caller who does prep_priors(max_antigens = 2, mu_hyp_param = 1:5, ..., decay_type = "Power") gets no error at all and silently gets treated as a 4-parameter exponential model.

This is not hypothetical: prep_priors() is @exported and documented/called directly (e.g. inst/examples/examples-prep_priors.R, tests/testthat/fixtures/make-example-runjags-inputs.R), so it is reachable by a user without going through run_serodynamics()'s match.arg(decay_type, c("power", "exponential")) guard (R/run_serodynamics.R:129), which is the only place decay_type is actually validated in this PR. The git history shows this was previously caught: commit 5effb7b ("Adding match.arg for decay_type") added exactly this guard directly inside prep_priors(), but the very next relevant commit, a280916 ("Changing workflow so run_serodynamics explicitly passes decay_type to prep_priors"), removed it again — leaving prep_priors() unguarded. This is the same class of bug as finding #1 from the very first review round (a validation branch that silently passes through instead of erroring), and it directly undermines this PR's stated goal of forcing users to supply correct, deliberate priors: an invalid decay_type typo combined with a 5-length mu_hyp_param now runs to completion with a mismatched, half-specified model instead of failing loudly. Suggest restoring decay_type <- match.arg(decay_type, c("power", "exponential")) as the first line of the function body (as commit 5effb7b had it).

There is also no test covering this path — tests/testthat/test-prep_priors.R exercises "power" and "exponential" explicitly, but nothing exercises an invalid/misspelled decay_type, which would have caught this.

2. (Minor) Roxygen doc miscounts exponential parameters — R/prep_priors.R:18-19, propagated to man/prep_priors.Rd:32 and man/run_serodynamics.Rd:91

#' When `decay_type = "exponential"` only 5 parameters (y0, y1, t1, alpha) are 
#' used.

The sentence says "5 parameters" but lists four (y0, y1, t1, alpha) — for exponential decay it should read "4 parameters," consistent with the rest of the same paragraph (prec_hyp_param/omega_param docs correctly say "4 values ... when decay_type = "exponential"") and with the validation code itself (length(...) != 4 && decay_type == "exponential"). This propagates automatically into both .Rd files since run_serodynamics.Rd's copy is roxygen-inherited (@inheritDotParams prep_priors), so a single one-word fix in R/prep_priors.R resolves all three occurrences.

Not flagged (checked, no issue)

  • @inheritParams run_serodynamics decay_type (R/prep_priors.R:41) is valid roxygen2 syntax — @inheritParams accepts an optional list of specific argument names after the source function (same mechanism as @inheritDotParams), confirmed against the roxygen2 tags-reuse reference docs. man/prep_priors.Rd's decay_type entry is correctly generated from it, not hand copy-pasted.
  • The "Aiemjoy K et al. ... (2022)" citation added to vignettes/articles/getting-started.qmd:143 is a real, verifiable paper (Lancet Microbe 2022;3:e578-87) — not a hallucination.
  • New tests "Preparing priors for exponential", "Expect error for {mu_hyp_param,prec_hyp_param,omega_param} under exponential" correctly exercise the exponential branch and its error paths; snapshot additions in tests/testthat/_snaps/prep_priors.md match.
  • test-plot_residuals.R's new heavy-JAGS exponential test correctly routes through run_serodynamics(decay_type = "exponential", ...), which is protected by match.arg(), so it isn't exposed to finding Jags2 #1.
  • .github/workflows/*.yaml changes (from the saved diff, since the on-disk copies are the default-branch fallback per this run's instructions): the Windows JAGS install fix (start /wait "" wjags.exe /S instead of a bare, non-blocking wjags.exe /S) is a genuine correctness fix — previously the script could delete/move on before the installer finished. The added curl -fSL --retry 3 --retry-connrefused flags are a reasonable hardening of a flaky download. The install rjags step's switch to if (!requireNamespace("rjags", quietly = TRUE)) install.packages(...) and dropping type = "source" is a plausible optimization (CRAN's rjags binaries for Windows/macOS are built against a specific JAGS version, and the workflow pins the same JAGS 4.3.1 that's installed), but I can't fully verify from a static read that binary rjags will always link correctly against the separately-installed JAGS across all matrix legs — flagging as informational rather than blocking since I can't execute the workflow to confirm.
  • No .data-in-tidyselect, branching-that-only-varies-columns, merge()-vs-join, or missing relationship= issues — this diff introduces no new dplyr code (the one dplyr-adjacent test-file change, test-plot_residuals.R:99, is a pure whitespace fix, attr(model,"decay_type")attr(model, "decay_type"), functionally identical).
  • DESCRIPTION version bump (.9017.9018) is a standard sequential dev bump.

Verdict

Needs more work — finding #1 (prep_priors() no longer validates decay_type, silently falling through to the 4-parameter/exponential path for any unrecognized value) is a genuine regression introduced in this round and undercuts the PR's core goal of forcing users to supply correct, deliberate priors. Finding #2 is a one-word doc fix. All previously-open findings from earlier rounds (the vignette's second run_serodynamics() call, and the NEWS.md wording/label) are now resolved.

Reviewed commit: e2157ae

@sschildhauer please address this review

@d-morrison
d-morrison removed their request for review August 31, 2026 15:57
@sschildhauer

Copy link
Copy Markdown
Collaborator Author

Claude finished reviewView run
Confirmed. Now I have everything needed to write the review.

Review: #284 — "No default priors"

Follow-up review. Since the prior round (commit 4908763), the author pushed the commit range 49087638..e2157ae2 (24 commits), which primarily adds exponential-decay-aware prior validation to prep_priors() and hardens the CI JAGS-install steps. I re-verified the file contents directly and confirmed the previously-open finding.

Previously-flagged issues: status

#2vignettes/articles/getting-started.qmd stratified-example call missing priors — Fixed. vignettes/articles/getting-started.qmd:220-234 now supplies all five explicit prior arguments to the fitted_stratified call, matching the fix already applied to the earlier run-model chunk. Both run_serodynamics() calls in the vignette now have explicit priors, so the vignette should render end-to-end again.
#6 (remaining half) — NEWS.md wording/labeling. NEWS.md:5-7 now carries an explicit **Breaking change:** label (matching the NEWS.md:104-style precedent) and the earlier missing-space typo before the backtick is fixed. This closes out both outstanding sub-points from the prior round.

New findings (introduced in 49087638..e2157ae2)

1. (Blocking) prep_priors() silently accepts an invalid decay_type, bypassing all length validation — R/prep_priors.R:78-96,109 The validation for mu_hyp_param/prec_hyp_param/omega_param is now written as two mutually exclusive branches, gated on decay_type == "power" and decay_type == "exponential" respectively — with no else/default branch and no match.arg()/arg.match() call on decay_type itself:

if (length(mu_hyp_param) != 5 && decay_type == "power") {
  cli::cli_abort(...)
} else if (length(mu_hyp_param) != 4 && decay_type == "exponential") {
  cli::cli_abort(...)
}

and later:

n_params <- if (decay_type == "power") 5L else 4L

If decay_type is anything other than the exact strings "power"/"exponential" (a typo, wrong case, trailing whitespace, NULL, etc.), neither cli_abort() branch fires regardless of the supplied prior lengths — the length checks are simply skipped — and n_params falls through to 4L (the "exponential" branch) via the bare else. A caller who does prep_priors(max_antigens = 2, mu_hyp_param = 1:5, ..., decay_type = "Power") gets no error at all and silently gets treated as a 4-parameter exponential model.
This is not hypothetical: prep_priors() is @exported and documented/called directly (e.g. inst/examples/examples-prep_priors.R, tests/testthat/fixtures/make-example-runjags-inputs.R), so it is reachable by a user without going through run_serodynamics()'s match.arg(decay_type, c("power", "exponential")) guard (R/run_serodynamics.R:129), which is the only place decay_type is actually validated in this PR. The git history shows this was previously caught: commit 5effb7b ("Adding match.arg for decay_type") added exactly this guard directly inside prep_priors(), but the very next relevant commit, a280916 ("Changing workflow so run_serodynamics explicitly passes decay_type to prep_priors"), removed it again — leaving prep_priors() unguarded. This is the same class of bug as finding #1 from the very first review round (a validation branch that silently passes through instead of erroring), and it directly undermines this PR's stated goal of forcing users to supply correct, deliberate priors: an invalid decay_type typo combined with a 5-length mu_hyp_param now runs to completion with a mismatched, half-specified model instead of failing loudly. Suggest restoring decay_type <- match.arg(decay_type, c("power", "exponential")) as the first line of the function body (as commit 5effb7b had it).
There is also no test covering this path — tests/testthat/test-prep_priors.R exercises "power" and "exponential" explicitly, but nothing exercises an invalid/misspelled decay_type, which would have caught this.
2. (Minor) Roxygen doc miscounts exponential parameters — R/prep_priors.R:18-19, propagated to man/prep_priors.Rd:32 and man/run_serodynamics.Rd:91

#' When `decay_type = "exponential"` only 5 parameters (y0, y1, t1, alpha) are 
#' used.

The sentence says "5 parameters" but lists four (y0, y1, t1, alpha) — for exponential decay it should read "4 parameters," consistent with the rest of the same paragraph (prec_hyp_param/omega_param docs correctly say "4 values ... when decay_type = "exponential"") and with the validation code itself (length(...) != 4 && decay_type == "exponential"). This propagates automatically into both .Rd files since run_serodynamics.Rd's copy is roxygen-inherited (@inheritDotParams prep_priors), so a single one-word fix in R/prep_priors.R resolves all three occurrences.

Not flagged (checked, no issue)

  • @inheritParams run_serodynamics decay_type (R/prep_priors.R:41) is valid roxygen2 syntax — @inheritParams accepts an optional list of specific argument names after the source function (same mechanism as @inheritDotParams), confirmed against the roxygen2 tags-reuse reference docs. man/prep_priors.Rd's decay_type entry is correctly generated from it, not hand copy-pasted.
  • The "Aiemjoy K et al. ... (2022)" citation added to vignettes/articles/getting-started.qmd:143 is a real, verifiable paper (Lancet Microbe 2022;3:e578-87) — not a hallucination.
  • New tests "Preparing priors for exponential", "Expect error for {mu_hyp_param,prec_hyp_param,omega_param} under exponential" correctly exercise the exponential branch and its error paths; snapshot additions in tests/testthat/_snaps/prep_priors.md match.
  • test-plot_residuals.R's new heavy-JAGS exponential test correctly routes through run_serodynamics(decay_type = "exponential", ...), which is protected by match.arg(), so it isn't exposed to finding Jags2 #1.
  • .github/workflows/*.yaml changes (from the saved diff, since the on-disk copies are the default-branch fallback per this run's instructions): the Windows JAGS install fix (start /wait "" wjags.exe /S instead of a bare, non-blocking wjags.exe /S) is a genuine correctness fix — previously the script could delete/move on before the installer finished. The added curl -fSL --retry 3 --retry-connrefused flags are a reasonable hardening of a flaky download. The install rjags step's switch to if (!requireNamespace("rjags", quietly = TRUE)) install.packages(...) and dropping type = "source" is a plausible optimization (CRAN's rjags binaries for Windows/macOS are built against a specific JAGS version, and the workflow pins the same JAGS 4.3.1 that's installed), but I can't fully verify from a static read that binary rjags will always link correctly against the separately-installed JAGS across all matrix legs — flagging as informational rather than blocking since I can't execute the workflow to confirm.
  • No .data-in-tidyselect, branching-that-only-varies-columns, merge()-vs-join, or missing relationship= issues — this diff introduces no new dplyr code (the one dplyr-adjacent test-file change, test-plot_residuals.R:99, is a pure whitespace fix, attr(model,"decay_type")attr(model, "decay_type"), functionally identical).
  • DESCRIPTION version bump (.9017.9018) is a standard sequential dev bump.

Verdict

Needs more work — finding #1 (prep_priors() no longer validates decay_type, silently falling through to the 4-parameter/exponential path for any unrecognized value) is a genuine regression introduced in this round and undercuts the PR's core goal of forcing users to supply correct, deliberate priors. Finding #2 is a one-word doc fix. All previously-open findings from earlier rounds (the vignette's second run_serodynamics() call, and the NEWS.md wording/label) are now resolved.
Reviewed commit: e2157ae

@sschildhauer please address this review

All issues addressed

  • Added additional cli_abort to test for spelling errors in decay type. This will be a good check overall for run_serodynamics as well.
  • Fixed mistake in prep_priors documentation.

@sschildhauer

Copy link
Copy Markdown
Collaborator Author

@claude , review

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

👀 Claude Review Dispatchedrun reviewing PR #284 at 213aecb.

@github-actions

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Review: #284 — "No default priors"

This is a follow-up review covering the changes since the last round (e2157ae2..213aecb1): the CI/permission commits are unrelated merge-in noise from main; the substantive work is in R/prep_priors.R, which now makes prior-vector length validation decay_type-aware (5 elements for "power", 4 for "exponential"), adds a decay_type argument check, forwards decay_type from run_serodynamics() into prep_priors() (previously it wasn't forwarded at all — a real fix), and adds/removes tests accordingly.

Previously-flagged issues: status

  • Jags2 #1 (prec_hyp_param validation bug) — still fixed; R/prep_priors.R:90-95 now correctly checks length(prec_hyp_param).
  • Jags2 #2 (vignette stratified-example chunk missing priors) — fixed; both run_serodynamics() calls in vignettes/articles/getting-started.qmd (lines ~139-149 and ~225-236) now supply explicit priors.
  • test that JAGS gives consistent results with the RNG inputs we provide #6 (NEWS.md breaking-change labeling / missing space) — fixed; NEWS.md:86-88 is now under ## New features, explicitly labeled **Breaking change:**, and the "forrun_serodynamics()" spacing bug is corrected.

New findings

1. (Moderate — test-coverage regression) The regression test that specifically caught the original prec_hyp_param bug was removed and not replaced for the default ("power") path
tests/testthat/test-prep_priors.R
Round 2 added "Expect error when only some supplied", which omitted only prec_hyp_param (supplying the other four) and asserted an error — this is exactly the test that would catch a recurrence of the round-1 bug (checking length(mu_hyp_param) instead of length(prec_hyp_param)). In this round's rewrite of the test file, that test is gone. The only remaining prec_hyp_param-specific test is "Expect error for prec_hyp_param under exponential" (lines 35-41), which exercises the decay_type == "exponential" branch (length(prec_hyp_param) != 4), not the default "power" branch (length(prec_hyp_param) != 5 && decay_type == "power", R/prep_priors.R:90-91). "Expect error when priors are not supplied" (lines 23-26) omits everything, so it only ever exercises the first (mu_hyp_param) check before cli_abort halts execution — it never reaches the prec_hyp_param branch. Net effect: the power-path prec_hyp_param check has no dedicated test today, so a future copy-paste regression of the exact bug this PR already had once would go uncaught by CI. Given the lab manual's testing guidance and that this specific safeguard was explicitly praised in the round-2 review, I'd restore an equivalent "omit only prec_hyp_param under power decay" test.

2. (Minor — dead/duplicated logic) configure_decay_priors() now performs a no-op re-slice for exponential decay
R/decay_helpers.R:38-68 (not touched by this PR, but its behavior is made redundant by it) / R/run_serodynamics.R:163
Before this round, prep_priors() always produced 5-parameter arrays, and configure_decay_priors() was the place that truncated them to 4 params for decay_type == "exponential". Now prep_priors() itself already returns n_params = 4 arrays (and requires exactly 4-element prior vectors) when decay_type == "exponential" (R/prep_priors.R:114). So when run_serodynamics() calls configure_decay_priors(priorspec, decay_type) afterward, the seq_len(4) re-slicing of priorspec$mu.hyp/prec.hyp/omega/used_priors operates on arrays that are already exactly 4-wide — it's a pure identity no-op. Not a bug (indexing [1:4] on a length-4 dimension is harmless), but it's now dead code duplicating a responsibility prep_priors() already owns, and a future maintainer reading configure_decay_priors() in isolation would incorrectly believe it's still doing real truncation work. Worth simplifying now that the two functions overlap.

3. (Minor — doc gap) NEWS.md doesn't mention the new decay-type-aware prior-length behavior
NEWS.md:85-88
The existing bullet only documents "no default priors." It doesn't mention that this round also changed how many values are required depending on decay_type — previously (per the pre-round docs this PR itself replaces, e.g. old R/run_serodynamics.R text) prep_priors() required a 5-element vector even for exponential decay (with the 5th shape value ignored); now it requires exactly 4 and errors on 5. That's a second, related breaking change to the same functions already called out in this NEWS entry, and is worth its own bullet for discoverability, consistent with how thoroughly this file documents adjacent behavior changes elsewhere (e.g. NEWS.md:114-116).

4. (Observation, non-blocking) The decay_type passthrough fix is only exercised by heavy-gated tests
R/run_serodynamics.R:161 (decay_type = decay_type now forwarded to prep_priors())
This is a real, valuable fix — previously run_serodynamics(..., decay_type = "exponential") never told prep_priors() about it, so prep_priors() used its "power" default and demanded a 5-element vector with an ignored shape entry. The only tests that exercise this fixed code path end-to-end (test-run_serodynamics.R's "exponential decay preserves the SEES output structure", test-plot_residuals.R's decay_type test) are gated behind Sys.getenv("RUN_HEAVY_TESTS") == "true" and thus skipped in normal CI. This matches the project's existing convention for full-JAGS-run tests, so I'm not marking it blocking, but flagging it since a regression here (e.g., dropping that one line again) wouldn't be caught by a normal PR's CI run.

Not flagged (checked, no issue)

  • @inheritParams run_serodynamics decay_type (R/prep_priors.R:41) is valid roxygen2 syntax — @inheritParams accepts an optional list of specific argument names after the source function to restrict which params are inherited (confirmed against the roxygen2 tags-reuse reference docs). The generated man/prep_priors.Rd correctly picked up only the decay_type entry, matching run_serodynamics()'s doc text verbatim — no CLAUDE.md doc-reuse violation, and not a hallucinated tag usage.
  • The new decay_type-aware length-validation logic in R/prep_priors.R:76-110 is internally consistent (if/else-if branches correctly gate on decay_type, no case is unreachable or double-fires) and the resulting array construction (n_params, diag() calls) is dimensionally consistent for both 4- and 5-parameter cases.
  • New snapshot # Preparing priors for exponential (tests/testthat/_snaps/prep_priors.md) is internally consistent with a diag(c(0.01,0.01,0.01,0.01)) construction replicated across max_antigens = 2 — manually verified the element ordering, not fabricated.
  • tests/testthat/test-plot_residuals.R's mu_hyp_param/prec_hyp_param/omega_param trims from 5→4 elements and the attr(model, "decay_type") spacing fix are both correct and match the exponential decay_type used in that test.
  • No .data-in-tidyselect, branching-that-only-varies-columns, merge()-vs-join, or missing relationship = issues — this round touches no dplyr code.
  • DESCRIPTION version bump (0.1.0.9018.9019) is a standard sequential dev bump.

Verdict

Needs more work — nothing here is an active bug (the decay_type-aware validation logic in prep_priors() is correct), but finding #1 (loss of the round-2 regression test that specifically guards against the exact historical prec_hyp_param validation bug this PR already had once) should be restored before merge, and finding #3 (NEWS.md gap for the new breaking behavior) is a quick fix in the same pass. Finding #2 (dead code in configure_decay_priors()) and #4 (heavy-gated-only test coverage for the passthrough fix) are optional cleanup, not blockers.

Structured Review Data (JSON)
{
  "schema_version": "1.0",
  "reviewer": "claude",
  "commit_sha": "213aecb1e70d7137a4d5001ced8a7b9a11f51add",
  "verdict": "NOT_CLEAN",
  "findings": [
    {"file": "tests/testthat/test-prep_priors.R", "line": 23, "category": "test", "message": "The round-2 regression test that isolated the prec_hyp_param length check under the default (power) decay_type by omitting only prec_hyp_param was removed and not replaced; the power-path prec_hyp_param check (R/prep_priors.R:90-91) now has no dedicated test, so a recurrence of the original round-1 copy-paste bug would go uncaught."},
    {"file": "NEWS.md", "line": 86, "category": "doc", "message": "The NEWS entry for removing default priors doesn't mention the new decay_type-aware breaking behavior change (exponential decay now requires exactly 4 prior values instead of 5 with an ignored shape entry)."},
    {"file": "R/decay_helpers.R", "line": 38, "category": "style", "message": "configure_decay_priors() now performs a fully redundant no-op re-slice for exponential decay, since prep_priors() already returns 4-parameter arrays for decay_type == 'exponential'; the truncation logic is now duplicated across two functions."}
  ]
}

Reviewed commit: 213aecb

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $1.1323 (review) — run

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.

prep_priors() length check

4 participants