Fix/exported guards and identity ci - #74
Merged
Merged
Conversation
…n link Three defects the reviews of the previous change disclosed and left. The message refusing an all-failed numerical optimization recommended grid search and blamed having more than three intervention components. Neither is right for the case that actually reaches it. A model whose terms are not all estimable returns NA coefficients, every outcome computed from them is NA, and every restart fails, so the path is reached through rank deficiency rather than through a hard optimization, and grid search fails on the same input for the same reason. The refusal now names the terms that could not be estimated and says to drop or combine them, keeping the old wording only for the case it was written for. Grid search no longer walks into its own unguarded comparison on the same input: it refuses it with the same message, which fires exactly when the comparison it precedes would have failed. get_confidence_set() is exported and does not go through validate_inputs(), so the weight checks added last time did not protect it. A negative center weight produced an upper confidence bound above 1 for a binary outcome, silently. The checks are now one function that both entry points call, so the two cannot disagree, and the argument documentation that already claimed weights must be non-negative and finite is true for both. The sum-to-one check stays out of the exported path deliberately: validate_inputs() renormalises, and the confidence set has nothing to renormalise into. A binary outcome's interval was built on the logit scale whatever link the model was fitted on, because that branch keyed on the outcome type and applied expit() and the logit delta-method factor unconditionally. A binomial model fitted with an identity link therefore reported an interval on the wrong scale: for one such fit 0.681 to 0.685, where the interval on the model's own scale is 0.759 to 0.779, which does not even contain the estimate. The point estimate was always right; only the interval was wrong. The branch now follows the link, and the logit arithmetic is unchanged term for term, since reassociating it moves the result in the last few digits.
Two points from review, both about a guard saying something a caller can act on. is.finite() is TRUE for every level of a factor, so a factor whose levels look like numbers passed the finiteness check, and the comparison after it then gave NA and raised the base-R "missing value where TRUE/FALSE needed" that the guard exists to replace. A non-numeric weight vector is now refused for what it is, with the same treatment for a character vector. This was inherited from the change that introduced the guard rather than new here. The rank-deficient refusal told the caller to drop or combine the collinear predictors. That is unfollowable on the route this branch newly names, where center-level data with one row per center is saturated rather than collinear: dropping a predictor leaves nothing to estimate the others from either. The message now covers that case and says what does help, which is more rows per center or fitting without the center effects.
The branch that reports a rank-deficient fit is entered when at least one coefficient could not be estimated, and the tests only ever passed it two. A condition of more than one aliased term therefore satisfied them while restoring the whole defect for a single aliased coefficient: grid search recommended, and more than three intervention components blamed, on a fit with two. One aliased term is not a corner case, it is what the message's own example produces, since two intervention components that are rescalings of one another alias exactly one. Both guards are pinned at that boundary now. Verified by mutating the condition in a throwaway tree and running the checks there: the assertion fails, naming the wrong message it received.
Every assertion on the rank-deficient message matched a substring, so a message listing every coefficient in the model satisfied all of them. That message tells the caller to drop the intercept, both intervention components and every center, which is advice they can follow and act on, and worse than naming nothing. The terms glm() did estimate are now asserted absent. The assertions are at the call site that decides which names are passed on, because that is the only place the difference shows: passing a hand-built list straight to the guard cannot see a caller that builds the wrong list. Verified by making that caller name every coefficient in a throwaway tree, where the checks then fail.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A binary outcome's interval was always built on the logit scale
The branch keyed on
outcome_typeand appliedexpit()and the logitdelta-method factor unconditionally, ignoring
link. A binomial model fittedwith
link = "identity"got an interval on the wrong scale:0.681–0.685where the interval on the model's own scale is
0.759–0.779, which does notcontain the point estimate.
This turned out bigger than the interval. Membership in the confidence set is
decided by whether an interval brackets the goal, so the wrong scale discarded
the set: on one fixture the released version reports an empty set where the
answer is 6 of 12 grid points, and on another a disjoint set of 10 wrong
interventions. The point estimate was always right.
The logit path is unchanged term for term — reassociating
se * p * (1 - p)moves the result at ~2e-16, so the order is preserved deliberately.
The exported entry point bypassed weight validation
get_confidence_set()is exported and does not go throughvalidate_inputs(),so #73's weight guards did not protect it: a negative centre weight gave a
confidence bound above 1 for a binary outcome, silently. The checks are now one
function with two call sites, so the paths cannot diverge.
The all-failed message recommended a method that cannot help
It suggested grid search and blamed ">3 intervention components". The path is
reached through rank deficiency, where grid search fails on the same input for
the same reason — it now names the terms that could not be estimated. Grid
search gained the same guard rather than walking into its own unguarded
comparison.