Fix/confidence bound range - #75
Merged
Merged
Conversation
Two ways a reported bound left the range of the quantity it describes, both reachable through the exported get_confidence_set() and both giving a probability outside 0 to 1. Center weights are supposed to form a weighted mean over centers, so they have to sum to 1. The exported entry point checked they were non-negative and finite but not that they summed to anything in particular, because the check that does that in validate_inputs() also renormalises, and the confidence set has nothing to renormalise into. Weights summing to 12 then produced an interval of 1.406 to 2.147 for a proportion outcome, and summing to 200, 18.259 to 30.398. They do not scale the outcome, which would at least be undoable: the weights multiply the center-dummy block while the intercept, the intervention contribution and the center characteristics are added once, so the result is a mixture of differently weighted pieces. Such a set is now refused rather than renormalised. The exported function cannot know which of the weights the caller got wrong, and renormalising would also perturb runs that are already correct, since the weights derived from center sample sizes do not always divide to exactly 1. Both entry points refuse the same vectors in the same words, from one shared check. The interval for a binary outcome is computed by the delta method on the probability scale, which is not confined to that scale: a small noisy fit returned a lower bound of -0.106 and an upper bound of 1.049 in the same set. Bounds are now reported within 0 and 1. This is a bound on the report and not a different interval: computing the interval on the linear predictor and transforming it back cannot leave the range at all, but it is a different interval and would move every binary interval the package has ever reported, which is not what a fix for an out-of-range report should do. Both bounds of a row have to be brought into range together. Clamping only the upper bound can leave it below the lower one, and findInterval() refuses a reversed interval outright rather than reporting something odd. Membership in the confidence set is unchanged, except for an outcome goal of exactly 1: findInterval() treats the interval as half-open, so a bound of exactly 1 was already excluded before this change while 1.049 was included. That is pre-existing and not reachable through lago_optimization(), where such a goal is unachievable and the confidence set is suppressed. The stored presentation snapshot contained two of these out-of-range values, a confidence interval ending at 1.005 and a set row with an upper bound of 1.132. It was pinning the defect, and is regenerated.
The clamp was applied to the matrix the confidence set is then computed from, so confining a bound changed which interventions qualify. findInterval() treats an interval as right-open, so for an outcome goal of exactly 1 a bound brought down from 1.049 to 1 puts the goal at the closed end and the row falls out: an intervention whose delta-method interval covers the goal stopped qualifying because of how its bound was reported. The claim in the previous commit that membership was unchanged held only for goals inside the range. Clamping is a statement about the report, so it is now applied to the report and nothing else. The bounds carried on the returned set and in the interval at the recommended intervention come from the confined matrix, while membership and the missing-bound filter read the interval as computed. Verified against the unclamped tree at goals of 0.5 and 1: the same rows qualify in both, while the reported bounds go from -0.106 and 1.049 to 0 and 1. A comment claimed findInterval() rejects an inverted interval outright. It does not, it reports against a vector it was told is sorted. Clamping both bounds together is still right, for that reason rather than the stated one.
Confining a bound to [0, 1] is only meaningful for an interval that has a part inside it. For one that lies entirely above 1 or entirely below 0 the intersection is empty, and clamping reported that emptiness as a non-empty interval: [1.02, 1.30] became [1, 1], a 95% interval of zero width, reported beside a point estimate it excludes. That is the invariant the previous release was fixed to restore, so clamping reintroduced it. It is reachable on an identity link, where a binomial model is a linear probability model and its prediction is the linear predictor, which is not confined to [0, 1] at all. Two reviewers found it independently, one by measuring 78 of 84 runs reporting a point estimate outside its own interval where the unclamped tree reports none, and the existing assertion that a point estimate lies inside its interval did not catch it because its fixture stays well inside the range and never reaches the clamp. Such rows now report no interval. NA is already what a bound that could not be computed carries here, and the consumers already handle it: the row is excluded from the confidence set by the same filter that drops an uncomputable bound, and the estimated outcome is reported with no interval rather than with an impossible one. Rows that straddle a boundary are still clamped, and rows inside the range are untouched.
Confining the reported bounds on both links was wrong on one of them. A binomial model fitted with an identity link is a linear probability model, and its point estimate is the linear predictor: extrapolate far enough along a positive slope and the estimate itself exceeds 1, which the package already reports as it stands. Confining only the interval around such an estimate reports an interval that excludes it, and where the whole interval is above 1 a zero-width one. On the logit link the estimate is expit of the linear predictor and is inside the range by construction, so confining the interval brings it into agreement with the estimate rather than out of it. Same operation, opposite effect, so it is applied only where it is right. Gating on the link also removes the case an earlier commit here handled by reporting no interval at all: an interval centred on an estimate inside the range always has a part inside it, so at most one of its bounds is ever confined and none is ever confined to nothing. The estimate leaving the range on an identity link is a defect of its own, and a larger one, since nothing constrains or flags it. It is not this one and is left for its own change. The test's oracle confined its bounds on both links, so its identity arm agreed with any implementation that did the same and could not observe the choice. The oracle now follows the link, the identity arm asserts that every reported interval contains the estimate it belongs to, which is the property confining them would break, and the logit arm keeps asserting the bounds are in range. The interval reported for the recommended intervention is asserted at a recommendation whose own interval leaves the range, since the previous fixture's was inside it and only a stored snapshot was holding that half of the fix.
Membership was asserted at an interior goal only, where no bound is confined and the two matrices cannot come apart. The case that distinguishes them is an outcome goal of exactly 1: findInterval() treats an interval as right-open, so a bound brought down to 1 puts the goal at the closed end and the row stops qualifying because of how it is reported. That is the defect fixed two commits ago, and nothing was holding it fixed, which is how it went in unnoticed in the first place. Verified by recombining the two matrices in a throwaway tree, deciding membership from the confined one again: the checks fail there.
The return documentation still promised that both bounds are confined to [0, 1] for a binary outcome, unconditionally. Gating the confinement on the link left that claim behind, so the documented contract said a reported bound is a probability while the function returns -0.169 and 1.011 on an identity-link fit. That is the same shape as the defect this branch is about, a stated guarantee about a reported probability that the value does not honour, and R CMD check cannot see it because it is prose. It now says which link confines and why the other does not. The identity arm's assertions that a reported interval contains its own estimate hold on that fixture whether or not the interval is confined, because no estimate there leaves [0, 1]: the largest is 0.594. What pins the choice is the hand oracle, which follows the link and so disagrees with a confined bound. The assertions are kept, because they state the property the choice exists to protect and would bite on a fixture that extrapolates far enough, and the comment now says which of the two is doing the work.
… can be The confidence-bound work stopped at confining what could be confined and left the estimate itself unaddressed on two paths. Both are now flagged, and neither changes a returned value: a caller who was getting a number still gets the same number, with a warning attached where that number is an extrapolation. A binary outcome on the identity link is a linear probability model, so its estimate is the linear predictor and is not confined to [0, 1]. Extrapolating along a positive slope past the fitted range gives an estimate above 1, which glm() does not object to because only the extrapolated grid predictions leave the range while the fitted values stay inside it. On the bundled shape at an unreachable goal the reported estimated outcome was 1.54, a probability that is not one. lago_optimization() now warns when a binary outcome's reported estimate leaves [0, 1]. The warning lives there and not in get_outcome(), which is the one function that computes the estimate but does not receive the outcome type, so it cannot know [0, 1] is the right range, and which runs on the negated scale under minimize, where every value it returns is negative regardless. Only lago_optimization() has both the outcome type and the estimate on the caller's own scale, and it fires once. The outcome is also predicted with any additional covariates held at 0. When a covariate's observed values never reach 0, that is a prediction for a covariate value that never occurred, and the reported outcome and interval are an extrapolation: a covariate observed in the fives, held at 0, shifted the reported interval. get_confidence_set() now warns, once, naming each numeric covariate whose observed range excludes 0 and that range. A factor or character covariate held at 0 is at its reference level, which is observed, so it does not warn. The value the covariate is held at is unchanged, so no returned number moves; the warning only says the number is an extrapolation. Neither the estimate leaving the range nor the covariate default is altered here. Both are reported as before and now flagged, which was the choice: the first because clamping the estimate would change which intervention the optimizer recommends, since get_outcome() drives the optimization and the goal comparison, and the second because rescaling or moving the held-at value would change every reported number for a run using additional covariates.
…tself Two points from review of the two diagnostics, both about the diagnostics themselves rather than any returned value. The covariate-support check called range() with na.rm on the observed values and then skipped a non-finite result, but an all-NA numeric column made range() itself warn about no non-missing arguments before that skip could run: a second, unrelated warning from something meant to emit one clear one. It now drops the non-finite values before range() and skips a column with none, so the base-R warning never fires. Not reachable through lago_optimization(), where the fit fails first, but reachable on a direct get_confidence_set() call. The out-of-range estimate message printed the value with signif() to six figures, so an estimate just past the boundary, 1.0000004, printed as 1 and the message read "the estimated outcome is 1, which is outside [0, 1]" -- a statement and its negation. It now prints at full precision, so the shown value is always one the reader can see is outside the range. The existing test pinned the rounded value and is updated to the fuller one, and a new test drives the message at a barely-out value to hold that it never rounds to look in range.
Two survivors from mutation review, both under-pinning rather than wrong code. The out-of-range check is strict: 0 and 1 are probabilities, so an estimate on either is in range and silent. Nothing pinned that, so loosening the test to <= or >= would warn on a legitimate probability, which a saturated fit reports, and the suite would stay green. An estimate at exactly 0 and exactly 1 is now asserted silent on both links, with just-outside each boundary asserted to warn so the silence is the closed boundary and not a dead check. The covariate warning lists the covariates held off their support. The multiple-offender test asserted the offenders are named but not that a non-offender is absent, so a message naming every covariate would have passed while telling the user a covariate held on its support is off it. A third covariate observed spanning 0 is added and asserted absent from the message. Both verified by mutation: loosening the boundary to non-strict fails the first, and dropping the spans-0 gate so every covariate is named fails the second.
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.
Keep a reported confidence bound inside the range it is compared against
Every way
get_confidence_set()could report a value outside the range of thequantity it describes, plus the two estimate-side defects underneath them that
earlier rounds kept deferring. Nine commits.
Weights not summing to 1 — the exported entry point accepted them and
produced an interval of 1.4-2.1 for a proportion. Refused, since the function
cannot know which weight the caller got wrong and renormalising would move
already-correct runs.
Binary interval outside [0, 1] — confined, on the logit link only. On
identity the estimate is the linear predictor and itself unbounded, so confining
its interval would report one excluding its own estimate. Confinement applies to
what is reported, not to what membership is decided from, so a goal of exactly 1
does not drop a qualifying row.
Identity estimate outside [0, 1] — the defect underneath the branch. A
binomial identity fit is a linear probability model; extrapolate far enough and
the estimate exceeds 1, silently. Now warned, once, at
lago_optimization()(theone site with the un-flipped estimate and the outcome type). Not clamped: the
estimate drives the optimizer, so clamping would change the recommendation.
Covariate held at 0 — additional covariates are predicted at 0 even when
their data never reaches 0. Warned, naming each numeric covariate whose observed
range excludes 0. The held value is unchanged, so no returned number moves.