Add tidypredict_combine_trees() and the last extractors - #436
Open
EmilHvitfeldt wants to merge 1 commit into
Open
Add tidypredict_combine_trees() and the last extractors#436EmilHvitfeldt wants to merge 1 commit into
EmilHvitfeldt wants to merge 1 commit into
Conversation
tidypredict_trees() returns per-tree expressions, but putting them back together is not summing or averaging them, and the rule differs by backend: blackboost offset + nu * sum(trees) aorsf mean(trees), inside a guard returning NA for a partial row catboost scale * sum(trees) + bias, then an inverse link xgboost base_score + sum(trees), then the objective's inverse link lightgbm sum(trees), divided by n for boosting = "rf", then the link randomForest mean(trees), plus corr.bias rescaling when present cforest mean(trees) So exposing the trees alone was not enough. A caller doing the obvious thing would drop blackboost's offset and shrinkage, and aorsf's missing- row guard, and get plausible wrong numbers rather than an error. Add tidypredict_combine_trees(x, trees), which holds that rule. `trees` may be expressions or symbols naming columns the trees were written to, since computing each tree into its own column and then combining references to them is the case this exists for. Every ensemble's tidypredict_fit() now routes through the two generics, so the identity tidypredict_combine_trees(x, tidypredict_trees(x)) == tidypredict_fit(x) holds by construction rather than by coincidence. The tests assert it at the value level for all nine backends, and again via symbols. Also adds tidypredict_trees() and tidypredict_n_trees() for cforest, blackboost and aorsf. Two backends deliberately have no trees exposed. Boosted C5.0 combines by a confidence-weighted vote over class labels, so there are no per-tree numbers to combine at all; tidypredict_combine_trees() refuses with an explanation rather than a bare "no method". And xrf is not a tree ensemble: it is a penalized linear model whose predictors are rule indicators, already handled as a pm_regression. Multiclass lightgbm and catboost also refuse: their tidypredict_trees() output is a flat round-robin list whose adjacent trees belong to different classes, so it is not summable as one ensemble. Corrects a stale comment on tidypredict_n_trees.lgb.Booster() claiming single-leaf trees are dropped by the extractor. add_lgb_stump_trees() deliberately restores them, because multiclass class assignment is positional and a gap shifts every later class (#419).
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
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.
T4 / phase 1d, and the last piece of the orbital new-backends plan's tidypredict-side work.
The plan for this PR was wrong, in a way the code made obvious
Phase 1a claimed that "any backend that gains a
tidypredict_trees()method automatically gainsseparate_treesin orbital with no orbital change at all." That only holds when a model combines its trees by a plain sum or mean. Checking the five backends this PR was supposed to cover, it holds for one:NAfor a partial row ✗offset + nu * sum(trees)✗Adding the methods as specified would have let a caller drop blackboost's offset and shrinkage, and aorsf's missing-row guard: plausible wrong numbers rather than an error.
tidypredict_combine_trees(x, trees)Holds the combination rule alongside the trees.
treesmay be expressions or symbols naming columns the trees were written to, because computing each tree into its own column and then combining references to those columns is the case this exists for.The rules, all of which a caller would otherwise have to know:
The identity is enforced structurally. Every ensemble's
tidypredict_fit()now routes through the two generics, soholds by construction rather than by coincidence. The arithmetic was factored into one helper per backend (
rf_combine(),ranger_combine(),xgb_combine(),lgb_combine(),catboost_combine(),mboost_combine()) called from both paths, so it cannot drift. Tests assert the identity at the value level for all nine backends, and again withrlang::syms()standing in for tree columns. It held on the first attempt for every backend, including DART andbinary:logistic.The xgboost helper is arranged so double-applying DART weights is structurally impossible:
assemble_xgb_formula()is nowxgb_combine(apply_dart_weights(...)).Two backends deliberately expose nothing
Boosted C5.0 combines by a confidence-weighted vote where each trial contributes a class label plus a confidence, with
SelectClassGen()breaking ties on that trial's own root class. There are no per-tree numbers, so splitting the trees apart is not an approximation of the model, it is a different model.tidypredict_combine_trees()refuses with an explanation rather than a bare "no method".xrf is not a tree ensemble at all. It is a penalized linear model whose predictors are rule indicators, built as a
termslist of coefficient times indicator, and it already works as apm_regression. The plan listed it here on a false premise.Multiclass lightgbm and catboost also refuse: their
tidypredict_trees()output is a flat round-robin list whose adjacent trees belong to different classes, so it is not summable as a single ensemble.A comment of mine from #433 was wrong
tidypredict_n_trees.lgb.Booster()claimed single-leaf trees are dropped by the extractor. They are not:lgb.model.dt.tree()omits them butadd_lgb_stump_trees()deliberately puts them back, because multiclass class assignment is positional and a gap shifts every later class (#419). Comment corrected.Verification
Suite
FAIL 0 | WARN 6 | SKIP 1 | PASS 2703, up from 2675.R CMD checkwith vignettes: 0 errors, 0 warnings.One thing worth flagging about the tests. The new ones initially passed under
devtools::test()and failed underR CMD checkwithcould not find function "case_when". Tree expressions callcase_when()unqualified, andtests/testthat.Rattaches only testthat and tidypredict — they were passing only because an earlier test file had attached dplyr, which check does not reproduce. Fixed by evaluating against dplyr's namespace explicitly, so the tests no longer depend on run order.Four pre-existing snapshots changed only in the reported call label, since the objective checks moved into the extracted helpers. Messages are byte-identical and the new labels are accurate.
Noted, not fixed
apply_xgb_objective()testsbase_score != 0on a valueget_xgb_json_params()can parse as length > 1, since its regex splits on commas. Unreachable today because multiclass objectives are rejected, but a latent trap.