Refactor/projection and validation gaps - #72
Merged
Conversation
The numerical optimizer's choice among its restarts was written inline inside a function nested in optimize_cost_nlcoptim(). Two of its steps, projecting the chosen restart onto the intervention bounds and recomputing its cost there, only run when every restart has left the box, which no two-component problem does. The regression suite had to reach them through a three-component configuration whose eleven restarts each miss the bounds by a unit or so in the last place, so a solver converging more tightly would have stopped the fallback firing and those tests would have kept passing while covering nothing. That could not be guarded from the outside: the projection leaves a component exactly on a bound and so does a solver that converged there, so the returned value cannot distinguish them. The filter, the fallback, the choice, the projection and the cost recomputation are one decision, so they move together into select_restart_within_bounds(), which takes the restart results and the bounds and needs no solver, no model and no data. The body is unchanged. Its unit tests cover all restarts in the box, some, none, a tie in cost, a failed restart among good ones, every restart failed, a single restart, and a winner genuinely outside the bounds. Removing the projection now fails thirteen of them, where before it was caught only by the fragile fixture, which is kept for the integration path. Two smaller gaps found while reviewing the same code: rec_int_processor() passed character(0) as the set of coefficient names already claimed by other terms, so at that call site the anchored-name fallback excluded nothing and the guard against a covariate being read as a fixed effect could not fire. It now builds the list the same way get_confidence_set() does, which meant threading additional_covariates through from lago_optimization(). The fallback turns out not to be reachable from any model the package fits, since the term mapping is only unavailable for an intercept-only fit, but the argument was wrong and a mutation to it survived the whole suite. Center weights within a thousandth of summing to 1 were accepted and then used as they were. They multiply the per-center outcomes and are summed, so weights summing to 0.999 scaled every reported outcome by 0.999, including the outcome the recommendation is chosen against. They are now renormalised where they are validated. The tolerance is unchanged, since it is documented and states that the input was meant to be a set of weights; renormalising is exact for a caller whose weights already sum to 1, and silent, because warning would fire on rounded input the documentation invites. The suite goes from 629 expectations to 712.
The new internal's documentation said the projection can only raise the cost of a monotone cost function, so that reporting the solver's cost understates what the recommendation costs. That is wrong in both halves. Projection moves a component up to a lower bound or down to an upper one, so on an increasing cost function it can move the cost either way, and a test in the same file demonstrates it: c(0.25, 7) projects to c(1, 5) and costs 27 against the 35.5 the solver reported. The cost function need not be monotone at all, since create_cost_function() builds whatever polynomial the caller's coefficients describe. The claim appeared in the roxygen, in the generated manual page and in two test comments. All four now say the recomputed cost can be on either side of the solver's, which is the reason it is recomputed rather than adjusted. Two smaller corrections in the same files. The note on the three-component fixture said two restarts return an exactly-zero bound violation under tighter solver tolerances; the count depends on how the objective is built, so it now says several, which is what the argument needs. And an assertion on a recorded outcome value was described as verified by hand below, with no derivation following: it is a fixture recorded from the fixed tree, and says so.
…claims Review found that deleting the single line in lago_optimization() that forwards additional_covariates to rec_int_processor() left the whole suite green. The existing test drives rec_int_processor() directly, so it pins what the callee does with the argument and never runs the caller, which is the same structural gap one level up from the one this branch set out to close. The fallback the argument protects needs a model whose term mapping is missing, and lago_optimization() never fits one, so it cannot be reached by running an optimization: the test instead replaces rec_int_processor() for one call and asserts what it received. Deleting the forwarding line now fails it. Two claims in comments were wrong. The note on renormalising center weights said it is exactly a no-op for a compliant caller, which does not hold on the default weights: a vector of center sizes divided by its own total need not sum to exactly 1, and about one such vector in four hundred is a unit in the last place away, so those runs move by that much. It now says so, and says the movement is the correction working rather than an error introduced. The note in rec_int_processor() said the term mapping is always available for models this package fits, making the fallback unreachable. A model fitted with model = FALSE whose data has left scope cannot rebuild its mapping, and the exported get_confidence_set() takes whatever model the caller passes, so the fallback is reachable and the guard on it is not decoration.
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.
1. Lift the restart selection out of the solver
The numerical optimizer's choice among its restarts — filter to the ones inside
the bounds, keep all of them if none is, take the cheapest, project it onto the
bounds, recompute its cost there — was inline inside a function nested in
optimize_cost_nlcoptim(). The last two steps only run when every restart hasleft the box, which no two-component problem does, so #71 had to reach them
through a three-component fixture whose eleven restarts each miss the bounds by
about a unit in the last place. A solver converging more tightly would have
stopped the fallback firing and those tests would have kept passing while
covering nothing, and that could not be detected from outside: the projection
leaves a component exactly on a bound, and so does a solver that converged there.
It is now
select_restart_within_bounds(), callable with a hand-built restartmatrix and no solver, model or data. The body is unchanged. Unit tests cover all
restarts in the box, some, none, a cost tie, a failed restart among good ones,
every restart failed, a single restart, and a winner genuinely outside the
bounds. Removing the projection now fails 13 of them, under the exact tighter
solver tolerances that previously disarmed the fixture, with the fixture
contributing zero failures. The fixture is kept for the integration path.
2.
rec_int_processor()was defeating a guard at its own call siteIt passed
character(0)as the set of coefficient names already claimed by otherterms, so the anchored-name fallback excluded nothing and the guard against a
covariate being read as a fixed effect could not fire. It now builds the list the
way
get_confidence_set()does, which meant threadingadditional_covariatesthrough from
lago_optimization().3. Center weights were validated and then used unnormalised
Weights within a thousandth of summing to 1 were accepted as-is. They multiply
the per-centre outcomes and are summed, so weights summing to 0.999 scaled every
reported outcome by 0.999 — including the outcome the recommendation is chosen
against. They are renormalised where they are validated. The tolerance is
unchanged, since it states the input was meant to be a set of weights, and the
correction is silent because warning would fire on rounded input the
documentation invites.