Fix cforest models under partykit 1.3-0 - #434
Merged
Merged
Conversation
partykit 1.3-0 added a shim to its S3 methods that warns when a method is called directly rather than through the generic. It identifies the caller with as.name(as.list(sys.call(-1))[[1L]]) which errors on any call whose first element is not a symbol. The generic's own frame is the method's caller, so calling `partykit::gettree(model, tree_no)` makes that first element the `::` call, and every cforest model fails with 'language' object cannot be coerced to type 'symbol' This breaks tidypredict_fit(), parse_model() and the partykit vignette, so R CMD check fails at vignette building. Bind the generic to a local name and call it through that symbol. The name has to be `gettree`, since the shim compares the symbol it recovers against the generic's own name and only stays quiet when they match; any other name works but emits a spurious deprecation warning per call. Only gettree is affected. nodeids, nodeapply, as.party, id_node, is.terminal and kids_node were each checked and are fine with the `::` prefix.
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.
mainis currently red, and #433 inherited it. The cause is partykit 1.3-0, released in the last day or so, not anything in either PR.Evidence it is pre-existing
mainat 3bfa3c7 already fails: run 32607353890 reportsprocessing vignette 'partykit.Rmd' failed.mainlocally and installing partykit 1.3-0 reproduces the identical error. With partykit 1.2.27 it passes.The bug
partykit 1.3-0 added a shim to its S3 methods that warns when a method is called directly instead of through the generic. It identifies the caller with:
as.name()errors on any call whose first element is not a symbol. The generic's own frame is the method's caller, so writingpartykit::gettree(...)makes that first element the::call, and every cforest model dies with:Note
is.null(caller) ||short-circuits, which is why a bare top-level call looks fine and this only shows up from inside another function. It takes outtidypredict_fit(),parse_model(), and the partykit vignette, soR CMD checkfails at vignette building rather than at tests.Calling
partykit::gettree()is correct usage, so this is an upstream regression. Worth reporting to partykit; this PR just unblocks us.The fix
Bind the generic to a local name and call it through that symbol.
The name has to be
gettreespecifically. The shim compares the symbol it recovers against the generic's own name, sogettreestays silent while any other name works but emits a spurious "calling partykit methods directly is deprecated" warning on every call. I verified both branches of that.Only
gettreeis affected. I checked every partykit function tidypredict calls with a::prefix —nodeids,nodeapply,as.party,id_node,is.terminal,kids_node— and they are all fine.Verification
Under partykit 1.3-0: suite
FAIL 0 | WARN 6 | SKIP 1 | PASS 2475, andR CMD checkwith vignettes gives 0 errors, 0 warnings. The remaining notes are a worktree.gitand the pre-existing unstatedcatboostin vignettes.The new test asserts both halves of the fix: no error, and no warning, so a future refactor that renames the binding gets caught rather than silently reintroducing warning noise.
Worth merging ahead of #433, which should then go green on rebase.