Replace the .extract_* functions with generics - #433
Merged
Conversation
The eleven exported .extract_* functions had no shared contract. Their
names suggest two shapes; there are four, and the _classprob suffix
covers two of them:
A xgb, lgb, catboost, rf, ranger _trees flat list of tree exprs
B rf, ranger _classprob level-keyed list of A
C rpart, partykit _classprob one expr per level
D earth, glmnet _multiclass one deparsed string per level
Replace them with four generics whose return shapes are documented:
tidypredict_trees() for A, tidypredict_class_trees() for B,
tidypredict_class_exprs() for C and D, and tidypredict_n_trees(), which
consumers currently substitute for by reading $ntree, $num.trees, niter
or a JSON dump directly.
Two behaviour changes fall out of unifying C and D. earth and glmnet now
return language objects rather than deparsed strings, so that every
extractor returns expressions. And the partykit result is keyed by
outcome level; it was unnamed, which left callers assuming its order
matched levels() of the outcome. rpart was already keyed, so only
partykit changes here.
The old names remain as thin wrappers. They are exported and each had an
Rd, albeit with \keyword{internal}, so an unknown caller is possible.
The earth and glmnet wrappers deparse to preserve their return type, and
the partykit wrapper drops the new names.
deprecate_soft() rather than deprecate_warn(): orbital calls these from
its own namespace in sixteen places, and warning there would surface a
message to orbital users about a function they neither called nor can
avoid. Verified that a plain orbital session emits no warning while a
direct caller still gets one.
Also note catboost's niter counts boosting rounds, not trees: for
multiclass models the two differ by the number of classes, so
tidypredict_n_trees() counts trees rather than trusting it.
EmilHvitfeldt
force-pushed
the
extractor-generics
branch
from
August 23, 2026 22:53
08859a6 to
f7c1fd8
Compare
This was referenced Aug 24, 2026
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.
T1 / phase 1a of the orbital new-backends plan, and the highest-leverage change on either side: once a backend has a
tidypredict_trees()method, orbital getsseparate_treessupport for it with no orbital change at all.The problem is worse than "ad-hoc"
I expected eight functions needing two generics. There are eleven, implementing four mutually incompatible contracts, and the
_classprobsuffix covers two of them:_trees_classprob_classprob_multiclassSo two functions sharing the
_classprobname return structurally different things, and two others return strings where everything else returns expressions.The generics
tidypredict_trees()— A, 5 methodstidypredict_class_trees()— B, 2 methodstidypredict_class_exprs()— C and D, 4 methodstidypredict_n_trees()— 5 methodsDocumented as one topic,
?tidypredict_extractors, with each return shape stated explicitly rather than left to be inferred.tidypredict_n_trees()is new: consumers currently substitute for it by reading$ntree,$num.trees,niter, orsum(startsWith(dump, "booster"))off a raw model object, which orbital does in eleven places.Two deliberate behaviour changes
earth and glmnet now return language objects, not deparsed strings, so every extractor returns expressions.
The partykit result is keyed by outcome level. It was unnamed, which left callers assuming its position matched
levels()of the outcome — orbital does exactly that. Worth noting rpart was already keyed, so only partykit changes; my first pass through the code suggested both were unnamed and that was wrong.Old names
Kept as thin wrappers. They are exported and each had an Rd, so an unknown caller is possible even though the Rd carried
\keyword{internal}. The earth/glmnet wrappers deparse to preserve their return type and the partykit wrapper drops the new names, so all eleven behave exactly as before.deprecate_soft(), notdeprecate_warn(). orbital calls these from its own namespace in sixteen places, and warning there would show orbital users a message about a function they neither called nor can avoid. I verified this rather than trusting the semantics: a plainlibrary(orbital)session emits zero warnings and still produces correct output, while orbital's own test run shows the warnings because testthat forceslifecycle_verbosity. A direct caller gets warned, which is who can act on it.Adds
lifecycleto Imports.Also worth knowing
catboost'snitercounts boosting rounds, not trees. For multiclass models they differ by the number of classes, sotidypredict_n_trees.catboost.Model()counts trees rather than trustingniter.Verification
Full suite
FAIL 0 | WARN 6 | SKIP 1 | PASS 2537, up from 2469. The 6 warnings are pre-existing upstream fitting warnings (earth constant-y, glmnet small-class, nnet empty group). The newtest-deprec-extractors.Rcovers all eleven wrappers: the deprecation signal, the preserved return shape, and the unchanged wrong-class error text.Cross-checked against orbital: its suite passes on this build apart from one unrelated snapshot it deliberately has not accepted (the logit rewrite, which is waiting on a version bump).
Three things this shook out that I fixed rather than shipped: the rpart error was reporting against the generic's formal
x, a name no user typed;tidypredict_n_trees()returned doubles for the two forest methods while promising an integer; and aglmnetclass whose coefficients are all zero yields a bare numeric next to language siblings, so the documented contract now states that a constant may appear in place of an expression in any of these generics, and that element type is therefore not uniform within one result.