No default priors - #284
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Merge commit '3b4716133fef058c422a27876729a296fdf1a4d6' #Conflicts: # DESCRIPTION # NEWS.md
|
@claude, review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@claude, review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| n_params <- 5 # Assuming 5 model parameters [ y0, y1, t1, alpha, shape] | ||
| n_params <- if (decay_type == "power") 5L else 4L |
There was a problem hiding this comment.
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.
@sschildhauer fixed! |
|
/review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@sschildhauer please address this review |
All issues addressed
|
|
@claude , review |
This comment has been minimized.
This comment has been minimized.
|
Claude finished review — View run Review: #284 — "No default priors"This is a follow-up review covering the changes since the last round ( Previously-flagged issues: status
New findings1. (Moderate — test-coverage regression) The regression test that specifically caught the original 2. (Minor — dead/duplicated logic) 3. (Minor — doc gap) NEWS.md doesn't mention the new decay-type-aware prior-length behavior 4. (Observation, non-blocking) The Not flagged (checked, no issue)
VerdictNeeds more work — nothing here is an active bug (the 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 |
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.