Add serodynamics Stan model for Chapter 2 (Model 2a, Chapter1+α) - #259
Add serodynamics Stan model for Chapter 2 (Model 2a, Chapter1+α)#259Kwan-Jenny wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
Claude finished @Kwan-Jenny's task in 4m 32s —— View job Code Review: PR #259 — Model 2a Stan (Chapter1+α)Todo
Overall verdictThe statistical construction is sound — the conditional/Schur factorization is correctly implemented and the Must-fix1.
2.
Should-fix3. Dead code / misleading "strip attributes" comment (
4. A wrong-length 5. No tests The new data-prep, prior-prep, and curve helpers have no unit tests. Given that the test suite runs with Minor / informational6. 7. 8. 9. No hallucinations found
|
Codecov Report❌ Patch coverage is
|
| stan_data <- stan_data[!names(stan_data) %in% | ||
| c("class", "antigens", "n_antigens", "ids")] | ||
|
|
||
| mod <- cmdstanr::cmdstan_model(file_mod) |
There was a problem hiding this comment.
cmdstanr is not declared in DESCRIPTION.
cmdstanr::cmdstan_model() is called here (and the whole runner fails without it), but cmdstanr appears in neither Imports nor Suggests in DESCRIPTION. R CMD check will flag an undeclared namespace, and users without cmdstanr will get an opaque error.
Add to Suggests in DESCRIPTION and guard with rlang::check_installed("cmdstanr") before this call (or earlier in the function), following the pattern the package uses for other optional dependencies.
| priorspec_clean <- priorspec[setdiff(names(priorspec), "n_params")] | ||
| stan_data <- c(longdata, priorspec_clean) | ||
| # strip R-side attributes so CmdStanR sees a plain list of model inputs | ||
| stan_data <- stan_data[!names(stan_data) %in% |
There was a problem hiding this comment.
Dead code — attribute stripping is already done by c().
c(longdata, priorspec_clean) returns a plain list: R's c() on list objects drops all non-names attributes (class, antigens, n_antigens, ids) from the inputs. By the time this filter runs, stan_data is already a plain named list and none of the filtered names appear as element names, so the filter is a no-op.
The comment "strip R-side attributes" is therefore misleading; a future reader may trust it and assume it works, masking a regression if the data-prep function ever returns those fields as proper list elements. Either remove the filter (with a comment explaining why c() already strips attributes), or make the stripping explicit with attributes(stan_data) <- list(names = names(stan_data)) if belt-and-suspenders safety is desired.
| #' `sd`, `rhat`, `ess_bulk`. | ||
| #' @export | ||
| summarise_pop_2a <- function(fit, antigens = NULL) { | ||
| param_names <- c("log_y0", "log_y1y0", "log_t1", "log_alpha", "log_shape1") |
There was a problem hiding this comment.
Naming inconsistency with log_two_phase_r: log_y1y0 ≠ log_y1.
summarise_pop_2a labels parameter j = 2 as "log_y1y0", which is Stan's par[k][2] = log(y1 − y0) (the log of the differential).
log_two_phase_r (line 51) takes a parameter named log_y1, expecting the log of the absolute peak, i.e. log(y1) = log(y0 + exp(par[2])).
These are different quantities. A user who feeds summarise_pop_2a output directly into log_two_phase_r (a natural workflow) will get silently wrong curves. The required conversion is:
log_y1 <- log(exp(log_y0) + exp(log_y1y0))Fix by either:
- Renaming the
log_two_phase_rparameter fromlog_y1tolog_y1y0and updating its body (y1 <- exp(log_y0) + exp(log_y1y0)), or - Documenting the conversion prominently in both functions'
@paramdocs.
| #' @param shape decay shape `r` (`> 1`). | ||
| #' @returns numeric vector `log y(t)`. | ||
| #' @export | ||
| log_two_phase_r <- function(t, log_y0, log_y1, t1, alpha, shape) { |
There was a problem hiding this comment.
Docstring claims "identical to the Stan log_two_phase" but the signatures differ.
The Stan function signature is log_two_phase(t, log_y0, log_y1, y1, t1, alpha, shape) — it takes y1 (the actual peak value) as a pre-computed argument. The R function omits that argument and computes y1 <- exp(log_y1) internally.
The functions compute the same formula, but the signatures are intentionally different (Stan takes the pre-computed y1 for efficiency). The docstring should say "implements the same formula" rather than "identical to" to avoid confusion.
| predict_newperson_2a <- function(mu_draws, Sigma_draws, | ||
| predict_antigen = 2L, | ||
| given = NULL, | ||
| n_params = 5L) { |
There was a problem hiding this comment.
given length is not validated.
If given is not NULL and not exactly length n_params, the error surfaces inside the matrix multiply as an opaque dimension mismatch. A guard here would give a much clearer message:
if (!is.null(given) && length(given) != n_params) {
stop("`given` must be NULL or a length-", n_params, " vector; got length ", length(given), ".")
}| L <- tryCatch(chol(S), error = function(e) { | ||
| chol(S + diag(1e-8, nrow(S))) | ||
| }) | ||
| as.numeric(mu + t(L) %*% stats::rnorm(length(mu))) |
There was a problem hiding this comment.
chol() returns the upper Cholesky factor; t(L) is correct but the variable name L implies lower triangular.
R's chol(S) returns upper triangular U such that S = U'U. The sampling step mu + t(L) %*% z = mu + U' z is mathematically correct for drawing from N(mu, S). But naming the result L (conventional for the lower Cholesky factor) is misleading. Rename to U or add an inline comment explaining the convention.
| generated quantities { | ||
| // Marginal block-2 covariance and the assembled joint covariance. | ||
| matrix[n_params, n_params] Sigma_2 = | ||
| quad_form_sym(Sigma_1, B') + multiply_lower_tri_self_transpose(L_psi); |
There was a problem hiding this comment.
quad_form_sym orientation is correct — confirming for reviewers.
quad_form_sym(A, B) computes B' A B, so quad_form_sym(Sigma_1, B') = (B')' Sigma_1 B' = B Sigma_1 B', which matches the Schur formula Sigma_2 = B Sigma_1 B' + Psi. ✓
One note: quad_form_sym requires A to be symmetric. Sigma_1 = multiply_lower_tri_self_transpose(L_1) is PSD by construction, so this is fine. A future reader may want a brief inline comment explaining the B' argument.
| // DECAY correlation. The off-diagonal cross-parameter terms are 0 by design. | ||
| vector[n_params] cross_cor; | ||
| for (p in 1:n_params) { | ||
| cross_cor[p] = c_cross[p] / sqrt(Sigma_1[p, p] * Sigma_2[p, p]); |
There was a problem hiding this comment.
cross_cor is not clamped to [-1, 1].
The Schur construction guarantees the theoretical joint covariance is PD, so |cross_cor[p]| ≤ 1 in exact arithmetic. In practice, floating-point accumulation (especially when c_cross[p] is large relative to the geometric mean of the diagonal variances) can push a computed value slightly outside [-1, 1]. Downstream R code that interprets cross_cor as a correlation may be surprised.
Consider adding a post-hoc clamp or at least noting this in the generated quantities block:
cross_cor[p] = fmax(-1.0, fmin(1.0, c_cross[p] / sqrt(Sigma_1[p, p] * Sigma_2[p, p])));
Summary
Adds the Chapter 2 Stan model Model 2a ("Chapter1+α") to
serodynamics, plus its runner and supporting functions. Model 2a is the honest generalization of the Chapter 1 model: it keeps Chapter 1's two free 5×5 within-biomarkercovariance blocks unchanged and adds only the 5 same-parameter cross-biomarker covariances (a diagonal cross-block). Setting those 5 terms to zero recovers Chapter 1 exactly, so Model 2a strictly nests Chapter 1 (35 vs 30 covariance
parameters).
This PR is built on
mainand is self-contained: it does not depend on the (unmerged) Chapter 1 Stan PR.What this enables
A subject's IgA and IgG curve parameters can now be correlated (e.g. a person with fast IgA decay also tends to have fast IgG decay), via the same-parameter cross-biomarker terms
c = (c_y0, c_y1, c_t1, c_alpha, c_r). Chapter 1 forces these to zero (biomarkers independent).The covariance structure
Positive-definiteness is guaranteed for any
cvia a conditional (Schur) construction:θ_IgG ~ MVN(μ_G, Σ_G),θ_IgA | θ_IgG ~ MVN(μ_A + B(θ_IgG − μ_G), Ψ)withB = C Σ_G⁻¹. This yields a diagonal cross-block exactly and reduces to the block-diagonal Chapter 1 model whenC = 0.Why this structure (and not the alternatives)
Σ_B ⊗ Σ_P(17 params): ties the two biomarkers to one shared within-block correlation, and is not nested with Chapter 1 — so the "Chapter 1 converges, therefore this should too" argument does not apply.Changes (committed)
inst/extdata/model_2a.stanNormal(0, ·)prior on the 5 cross-couplingsR/run_serodynamics_stan_2a.Rmodel_2a.stan.file_modlets calling code point at an alternative model with the same data interface. Requires exactly 2 biomarkers.R/prep_data_serodynamics_stan_2a.Rprep_data()frommain)R/prep_priors_serodynamics_stan_2a.RR/serodynamics_stan_2a_helpers.Rsummarise_pop_2a(),log_two_phase_r(),predict_newperson_2a()(conditional new-person prediction)Naming follows the lab rename
run_mod()→run_serodynamics(); the Chapter 2 runner uses the new convention (run_serodynamics_stan_2a).Priors
Matched to Chapter 1, with Chapter 1 at the prior mode:
Σ_GandΨ(the conditional/Schur covariance of the IgA block) each get the Chapter 1 per-antigen LKJ + lognormal factorization. AtC = 0,Ψ = Σ_A, so the prior is exactly Chapter 1's at the nesting point.c_p ~ Normal(0, scale_p), centred at 0 (so the model leans toward "no cross-correlation" until the data say otherwise).scale_pis derived from the matched marginal scales inside the model.Sampler defaults copy the Chapter 1 values validated on this likelihood (
adapt_delta = 0.95,max_treedepth = 12, default init).Dependencies
prep_data()andsim_case_data()frommain.Validation (not in this PR)
Validation is done locally and is intentionally not committed (
validation/is in.gitignore/.Rbuildignore). It generates a no-correlation control and two correlated datasets (same marginals, same truth) and fits both the Chapter 1 baseline and Model 2a on each, checking:A write-up will follow as an
articles/*.qmdonce the results are finalized (separate from this PR).Notes for review
main; once the Chapter 1 Stan PR merges, the temporary duplicates (prep_*_serodynamics_stan_2a, and the validation-only Chapter 1 baseline) can be replaced by the sharedprep_*_stan/run_mod_stan. The runner already takesfile_modand isolates its prep, so that switch is a small change.model_2a.stanmirrors the validated Chapter 1model.stanas closely as possible — only the covariance construction changes.