Fit a factor covariate level by level, with crossed model weights (#33 stage 1) - #227
Conversation
Review — phase 2 of
|
|
Test follow-up: Worth noting what the suite does not cover, since it bears on that finding: there is no test that a non-pseudo-BMA weighting method is rejected, because there is currently nothing to reject it. If the check goes in, that is the test to add alongside it. |
|
Addressed the review finding on this PR. The pseudo-BMA assumption is now enforced, not just documentedThe docs were right and emphatic; nothing checked them, and Why it had to be captured up front rather than checked at the end: a
An error rather than a warning, deliberately: under stacking there is no correct crossed table to return, so returning one is worse than refusing. The message points at the per-level weights, which remain valid. Three tests added — the refusal, the message naming the offending method, and an absent The roxygen now says the identity is enforced rather than leaving the reader to assume it holds, and One of my own tests was wrong first time round — I had written UnchangedEverything else in the review stands: the stop condition is not triggered, all four queue hazards are handled, and both readings of the crossed table are reported. Still outstanding for the migration rather than this PR: this adds Also merged #224, #225 and #226's fixes below, and restacked #228 on top — the whole stack is conflict-free against |
CI fixed — the failure was not this PR's subject matterEvery platform reported All three arguments were added by the phase 3 fixes themselves — decision (b) added
Also carried up from the base: No version bump — the monotone pinning is what keeps the merge path conflict-free. The restack merged with zero conflicts. |
ee0b705 to
1c4dae9
Compare
Review — blockers, and the stage 2 questionReviewed the branch as it stands ( The pseudo-BMA fix from the last round is right. Four things below. One gates the merge and is not this PR's fault; two are defects in the code; one is a factual correction to the rationale given last round. 1. Merge-gating: the bottom of the stack, not this PR227 → 226 → 225 → 224 → dev. #224 and #225 are both 2.
|
fccc2af to
f48134c
Compare
Fixes applied — and one correction to my own reviewAll four defects addressed, plus the queue's missing third reading. The two silent-wrong-answer routes
This changes the default column names from
|
bnec_group() fits the model set independently within each level and returns a bayesnecgroupfit; crossed_group_weights() reports both the unrestricted maximum (different equations per level -- the premise example4 has carried since the beginning) and the diagonal (which single equation best describes every level, which bayesnec could not previously answer). Family chosen once from the whole response: per-subset selection could pick different families and put their elpd contributions on different scales, making the crossed weights meaningless. Dispersion stays per level, because a shared one would break the factorisation those weights depend on. Deliberately NOT touching nec()/ecx()/nsec()/bnec_newdata(): every level is an ordinary bayesnec object, so the level-aware forms are a map over them rather than a reimplementation. Those files are what the toxval migration moves, and needing to reach inside them would have meant the stage boundary was drawn in the wrong place. Closes #33 stage 1. Stage 2, the joint refit, stays out of scope.
\link{set_distribution} does not resolve -- it is not exported -- so R CMD
check warned on every platform. Same class as the check_normalisation link
caught in #139 and the [0.71, 1.44] interval caught in #148; devtools::test()
never builds Rd, so CI is the first signal every time.
notes/scripts/check_rd_links.R scans man/ for unresolvable targets and exits
non-zero. Run after document(), before pushing.
Refs #33.
The outer-product identity holds for pseudo-BMA only, and that was documented but not checked -- while loo_controls reaches bnec() through bnec_group()'s dots, so stacking weights were reachable and would have produced a crossed table that looked right and was not. A bayesmanecfit does not record which method produced its wi, so bnec_group() captures it up front and crossed_group_weights() refuses anything else. Review finding on PR 227. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four findings from the review of this PR. group_estimate_table() read positions 1:3 of whatever the underlying method returned, while passing `...` straight through to methods that take `posterior` and `prob_vals`. nec(group_fit, posterior = TRUE) therefore reported draws 1, 2 and 3 as an estimate and its credible interval, and a longer prob_vals was silently truncated. Columns now come from the returned vector's own names, and posterior = TRUE is refused with a pointer at lapply(x$fits, nec, ...). This changes the default column names from Estimate/Q2.5/Q97.5 to Q50/Q2.5/Q97.5, which is what nec() itself returns per level. An NA in the grouping column passed every check -- factor() drops it from levels() and table() ignores it -- but data[grp == lev, ] is logical indexing with NA present, which puts an all-NA row into every level's subset. Refused. compare_posterior() moves down from #228, where it had been placed by mistake: it is package API for the class this PR introduces, so a reviewer of this PR should see it rather than finding it in a vignette branch. crossed_group_weights(pooled =) adds the queue's third reading -- does the factor matter at all. A pooled fit is scored on the same observations, so the grouped and pooled WAIC are directly comparable. The standard error needs the pointwise values, which do not survive every route a fit can take, so it is NA where they are absent rather than quietly omitted. Also corrects the rationale given last round: expand_manec() *does* record the weighting method, as attr(mod_stats$wi, "method"). It survives cbind and is dropped by row-subsetting, so capturing the request at fit time is still needed -- but the attribute is now read as a cross-check, since it reflects what the fit did rather than what was asked for, and levels weighted by different methods are refused. 02_deferred.md records the full seven-method toxval collision, verified against its NAMESPACE, and the methods this PR adds to that surface. test-bnec_group.R: 30 -> 55 assertions, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SE pairs each observation's pointwise WAIC across the grouped and pooled fits, so the level-ordered grouped values have to be matched against the pooled fit's row order rather than simply concatenated. Tested on data whose grouping column is deliberately not in level order, which is the case that would go wrong silently, and on a pooled fit scored on a different number of observations, where the point estimates still come back and the SE stays NA. Also corrects a comment that described the mismatch as refused; it is not, and should not be -- the WAIC point estimates are always comparable, and only the SE depends on the pointwise values lining up. test-bnec_group.R: 55 -> 63 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified end to end on real fits, and the first run came back with se_diff NA where the docs said it should be computed. The cause is in expand_manec(): mod_fits is snapshotted at R/expand_classes.R:268, before the expand_nec() loop that calls add_criteria(), so a bayesmanecfit stores its component fits without criteria. It keeps each model's WAIC point estimate in mod_stats and none of the pointwise values. So the standard error is available when every level and the pooled fit settled on a single model, and not when any of them is model-averaged -- which is the common case, and not what the previous wording implied. Documented as such, with the reason, in the roxygen, NEWS and a comment at the point of use. Verified on real fits: two single-model levels against a single-model pooled fit give diff 143.9 with se_diff 20.5 over 100 observations, and manec_example as a model-averaged pooled fit gives the difference with se_diff NA. Both pinned. test-bnec_group.R: 63 -> 66 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cosmetic only, no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5e63942 to
232ed94
Compare
Restacked onto #224The stack moved under this PR while it was in review, so it has been rebuilt. What changed. This PR was based on What it is now. Six commits replayed onto 14 files, +1373 lines against #224 — #33's own footprint and nothing else. Verified absent: Nothing in #33 depended on #224 or #225 in the first place — Two consequences worth flagging. The release tier changed.
Verification on the new base. Full suite: The pre-restack head is kept at #228 needs the same treatment — it is stacked on this branch and still carries the old stack, including #225's content and a duplicate of the |
RF: this is significant new functionality that never existed, so it warrants a release of its own rather than folding into a bug-fix tier. That is also what 00_protocol.md:121-125 lays out -- 2.1.4 for bug fixes, docs and CI; 2.2.0 for the feature tier (#136, #209, #148); 2.3.0 for the factor covariate (#33 and the #6/#33 vignette). The restack had folded this entry into 2.1.4 because that is the only heading the new base carries. NEWS.md against #224 is now purely additive: the 2.1.4 content is untouched and the bnec_group() entry sits under a new 2.3.0 heading above it. Note the wider drift, which is not this PR's to fix: 2.1.4 on dev and #224 has absorbed #136's rate() aterm and #148's check_fit()/pp_check()/check_sampling(), all three slated for 2.2.0. There is currently no 2.2.0 heading at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RF, 2026-08-25. The original plan was 2.1.4 bug fixes / 2.2.0 features / 2.3.0 factor covariate. #136 and #148 were folded into the 2.1.4 heading when their branches were restacked onto dev, and #209 is on hold, so the feature tier no longer exists as a separate release. Rather than unpick it, 2.1.4 becomes everything up to and including the feature work and 2.2.0 becomes the factor covariate release -- bnec_group() is significant new functionality that never existed and warrants a release of its own. There is no 2.3.0 tier. Updates the NEWS heading, the tier table in 00_protocol.md, the queue rows for items 5-9, and the precompile note: with #136 and #148 inside 2.1.4 the only vignette content crossing the boundary is now the #6/#33 grouping vignette. Not pushed -- held for RF. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Release tier: 2.2.0 — opens that heading in
NEWS.md. Stacked on #224(
#136, therate()aterm), which lands first.Restacked on 2026-08-25. It previously sat on #226, which has since merged to
dev, and its branch carried #225's hurdle-count work — now on hold — throughthe old stack. This branch is #33's own six commits and nothing else: 14 files,
+1373 lines against #224, with no
hurdle_poisson/hurdle_negbinomialortest-hurdle_counts.Rcontent. The 2.2.0 and 2.3.0 headings the old stack usedwere revised on 2026-08-25:
2.1.4is now everything up to and including thefeature work (#136, #209, #148), and
2.2.0is the factor covariate release—
bnec_group()is significant new functionality that never existed andwarrants a release of its own. There is no
2.3.0tier.NEWS.mdagainst #224is purely additive: the
2.1.4content is untouched and thebnec_group()entry opens a new
2.2.0heading above it.Closes #33 stage 1. Stage 2 stays out of scope — see below.
What this adds
bnec_group()fits the model set independently within each level of a factor,model-averaging within each level, and returns a
bayesnecgroupfit.crossed_group_weights()reports the model weights across levels, andcompare_posterior()— now a generic — compares the levels themselves.Verified on a real two-level fit of
nec_datawith the response scaled by 0.8in one level:
The levels separate as constructed, and the family is chosen once and shared.
Why the levels can be fitted separately
Levels partition the data disjointly and share no parameters, so the log
likelihood is a sum over levels and
elpdis additive. Under pseudo-BMA — thepackage default — the crossed weight of a combination is the product of its
per-level weights, which is the same identity
crossed_weights()rests on forthe two blocks of a hurdle fit.
The identity is specific to pseudo-BMA, and it is enforced, not assumed.
Stacking optimises a different objective whose solution is not an outer product,
so multiplying stacked weights gives a table that looks right and is not.
crossed_group_weights()refuses rather than returning one: there is no correctcrossed table under stacking, so returning one is worse than refusing.
The method is established two ways, because neither is reliable alone.
expand_manec()stamps it on the weight vector asattr(mod_stats$wi, "method"), which is what the fit actually did — but that attribute survivescbindand is dropped by row-subsetting, and a single-model level is abayesnecfitwith nomod_statsat all. Sobnec_group()also records therequest at fit time, and
crossed_group_weights()prefers whatever the fitsstill carry. Levels weighted by different methods are refused outright.
The table is computed on demand, never materialised: with 23 models and G
levels it has 23^G cells.
Three readings of the crossed table
The unrestricted maximum picks the best model per level independently and
will typically assign different equations to different levels. That is the
direct answer to the premise
vignette("example4")has carried since thebeginning — that the functional form may change between levels, not just the
parameters.
The diagonal,
w_m ∝ Π_g w_gm, asks which single equation best describesevery level.
bayesneccould not answer that before, and it is often thequestion a reader of the analysis actually has. It covers only models every
level fitted: one dropped from a level by
check_models()cannot be the commonform.
Grouped versus pooled — does the factor matter at all? Pass
pooled, abnec()fit of the same model set to the whole data with the factor ignored.It is scored on exactly the same observations, so the WAICs are directly
comparable:
waic_pooled - waic_grouped, positive favouring the grouped fit.A standard error accompanies the difference when every level and the pooled
fit settled on a single model, and is
NAwhen any of them ismodel-averaged, rather than being quietly omitted. The reason is in
expand_manec():mod_fitsis snapshotted atR/expand_classes.R:268, beforethe
expand_nec()loop that callsadd_criteria(), so abayesmanecfitstores its component fits without criteria — it keeps each model's WAIC point
estimate in
mod_statsand none of the pointwise values an SE needs. Wherethey are present the observations are additionally checked to line up.
The comparison is best model per level against best pooled model, not the
model-averaged predictions of either: the averaged predictive density is not
the weighted sum of the components'.
Verified on real fits — two single-model levels against a single-model pooled
fit on
nec_datawith one level scaled by 0.8:The factor matters, by about 7 standard errors, as constructed.
compare_posterior()across levelscompare_posterior()is now a generic —compare_posterior.default()is theprevious function body unchanged, so every existing caller and vignette behaves
identically — with a
bayesnecgroupfitmethod that delegates tox$fits. Thelevels are already the named list the function takes, so this is dispatch, not
new machinery.
It answers a different question from
crossed_group_weights(), and the two candisagree: the weights say which equation best describes each level, this says
whether the levels differ in the quantity being reported — the NEC, an ECx,
or the fitted curve. Two levels can favour the same form while differing in
where the threshold falls, and two can favour different forms while their NEC
posteriors almost entirely overlap. The levels share no parameters, so the
posteriors are independent and the pairwise probabilities are read directly,
with no multiple-comparison adjustment implied.
Decisions carried from the feasibility assessment
The family is chosen once, from the whole response, and passed down.
set_distribution()applied per subset could select different families atdifferent levels, which would put their
elpdcontributions on different scalesand make the crossed weights meaningless.
Dispersion stays per level, deliberately. Separate fits give each level its
own
sigma/shape/phi; a shared one would break the factorisation thecrossed weights depend on. Structure spanning levels — shared dispersion, a
group-level effect crossing levels, an explicit contrast — is a different model
and is not what this fits.
The stop condition held
The queue set an explicit one: if stage 1 turned out to need changes inside
nec(),ecx(),nsec()orbnec_newdata()beyond a level-aware wrapper,stop and report, because that would mean the stage boundary was drawn in the
wrong place.
It did not. Every level is an ordinary
bayesnecfit/bayesmanecfit, so thelevel-aware forms are a
lapplyover them. None of those four files is touchedby this PR, which is what keeps stage 1 clear of the
toxvalmigration.Stage 2 — the joint dummy-coded refit — is not attempted. The gate was
verified rather than assumed:
toxvalstill carriesbayesnecinImports, sothe dependency runs backwards, and registers seven methods colliding with
bayesnec's own. Stage 2's formula assembly would touch none of the migratingcode, but its payoff would:
nec()/nsec()readb_nec_Interceptby fixedname and
bnec_newdata()/fitted()/predict()carry no group handling, sobuilding only the assembly half yields a fit no per-level estimate can be read
from. The assessment also finds the joint refit inferentially identical to these
fits up to Monte Carlo noise, given per-level dispersion and per-level priors.
02_deferred.mdnow records the method surface this PR adds.Validation
A grouping column must exist, be categorical, have more than one level, and
carry no missing values, and each level needs at least 4 observations because it
is fitted as a complete concentration-response model in its own right.
The numeric case errors rather than silently factorising: a numeric column is
almost always a predictor that belongs in
crf().NAerrors rather than beingdropped, because
data[grp == lev, ]is logical indexing withNApresent —which puts an all-
NArow into every level's subset, formodel.frame()toabsorb without a word.
Per-level estimates are tabulated by name, not by position
nec(),ecx()andnsec()on abayesnecgroupfitreturn one row per level.The columns come from the names of the vector each underlying method returns:
those methods take
prob_vals, so a longer request was being silently truncatedto three columns and a reordered one mislabelled.
posterior = TRUEis refusedrather than accommodated — it makes the underlying method return the draws
instead of a summary, and a positional table reported draws 1, 2 and 3 as an
estimate and its credible interval. The message points at
lapply(x$fits, nec, posterior = TRUE), where the posteriors are exactly whatthey already were.
This changes the default column names from
Estimate, Q2.5, Q97.5toQ50, Q2.5, Q97.5, which is whatnec()itself returns per level.Tests
tests/testthat/test-bnec_group.R, 66 assertions. Real fits are deliberatelynot in the test file — two per test would dominate the suite — so a
bayesnecgroupfitis built frommanec_exampleas a stand-in level, whichtests the arithmetic and dispatch where the bugs actually are. The fitting path
is exercised by hand, above.
Covers: validation, including the numeric and
NAgrouping columns; crossedweights as the outer product; the best combination and its weight; the diagonal
summing to 1, sorted, and restricted to common models; a single-model level
contributing exactly 1; the pseudo-BMA refusal, the method read off the fits
overriding the recorded request, and mixed methods across levels; the pooled
comparison, its pointwise pairing on data whose grouping column is deliberately
not in level order, and its
NAstandard error both against a model-averagedfit and against a pooled fit scored on a different number of observations;
per-level estimates as one row per level with identical fits giving identical
numbers;
posterior = TRUErefused on all three estimators; a five-elementprob_valscarried through rather than truncated;compare_posteriordispatchwith the default path unchanged; and the print method reporting the shared
family once.
Also in this PR
\link{set_distribution}did not resolve — it is not exported — soR CMD checkwarned on every platform. Demoted to\code{}, andnotes/scripts/check_rd_links.Rwas added to catch the class of error — it scans every generated Rd for\linktargets that are not object names and reports them.