Skip to content

Replace the .extract_* functions with generics - #433

Merged
EmilHvitfeldt merged 1 commit into
mainfrom
extractor-generics
Aug 24, 2026
Merged

Replace the .extract_* functions with generics#433
EmilHvitfeldt merged 1 commit into
mainfrom
extractor-generics

Conversation

@EmilHvitfeldt

Copy link
Copy Markdown
Member

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 gets separate_trees support 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 _classprob suffix covers two of them:

Functions Returns
A xgb, lgb, catboost, rf, ranger _trees flat list of per-tree expressions
B rf, ranger _classprob level-keyed list of per-tree expression lists
C rpart, partykit _classprob one whole-tree expression per level
D earth, glmnet _multiclass one deparsed string per level

So two functions sharing the _classprob name return structurally different things, and two others return strings where everything else returns expressions.

The generics

  • tidypredict_trees() — A, 5 methods
  • tidypredict_class_trees() — B, 2 methods
  • tidypredict_class_exprs() — C and D, 4 methods
  • tidypredict_n_trees() — 5 methods

Documented 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, or sum(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(), not deprecate_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 plain library(orbital) session emits zero warnings and still produces correct output, while orbital's own test run shows the warnings because testthat forces lifecycle_verbosity. A direct caller gets warned, which is who can act on it.

Adds lifecycle to Imports.

Also worth knowing

catboost's niter counts boosting rounds, not trees. For multiclass models they differ by the number of classes, so tidypredict_n_trees.catboost.Model() counts trees rather than trusting niter.

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 new test-deprec-extractors.R covers 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 a glmnet class 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.

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
EmilHvitfeldt merged commit 0dfeafd into main Aug 24, 2026
9 checks passed
@EmilHvitfeldt
EmilHvitfeldt deleted the extractor-generics branch August 24, 2026 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant