fix: plot.gg_partial* return single patchwork figure instead of named list (#77)#78
Merged
Conversation
When both continuous and categorical predictors are present, plot.gg_partial(), plot.gg_partial_rfsrc(), and plot.gg_partialpro() previously returned list(continuous=<ggplot>, categorical=<ggplot>), which gave an inconsistent return type and made autoplot() dispatch ambiguous. They now call patchwork::wrap_plots(gg_cont, gg_cat, ncol = 1) so the return type is unconditionally a ggplot-inheriting object. patchwork moved from Suggests to Imports to reflect this unconditional usage. Tests updated: old list-structure assertions replaced with expect_s3_class(out, "ggplot"); autoplot test helper simplified to a plain expect_s3_class(). Closes #77. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
=======================================
Coverage 84.18% 84.18%
=======================================
Files 32 32
Lines 2403 2403
=======================================
Hits 2023 2023
Misses 380 380
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent return types for plot.gg_partial(), plot.gg_partial_rfsrc(), and plot.gg_partialpro() by ensuring they always return a single plot object (a ggplot or patchwork composition) rather than sometimes returning a named list of plots when both continuous and categorical predictors are present.
Changes:
- Updated
plot.gg_partial*methods to combine continuous + categorical panels withpatchwork::wrap_plots(..., ncol = 1)instead of returninglist(continuous=..., categorical=...). - Promoted
patchworkfromSuggeststoImports, and updatedNAMESPACE/roxygen docs accordingly. - Updated tests and documentation to reflect the new single-object return type.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
R/plot.gg_partial.R |
Switches combined output from a named list to a single patchwork plot; updates roxygen return docs and imports. |
DESCRIPTION |
Moves patchwork into Imports to support unconditional runtime usage. |
NAMESPACE |
Adds importFrom(patchwork, wrap_plots). |
NEWS.md |
Documents the behavior change for plot.gg_partial* return types. |
man/plot.gg_partial.Rd |
Updates return-value documentation to describe patchwork composition. |
man/plot.gg_partial_rfsrc.Rd |
Updates return-value documentation to describe patchwork composition. |
man/plot.gg_partialpro.Rd |
Updates return-value documentation to describe patchwork composition. |
tests/testthat/test_gg_partial.R |
Adjusts expectations for combined continuous+categorical plotting to be a single ggplot-inheriting object. |
tests/testthat/test_gg_partialpro.R |
Adjusts expectations for combined continuous+categorical plotting to be a single ggplot-inheriting object. |
tests/testthat/test_autoplot.R |
Simplifies autoplot assertions now that outputs are consistently ggplot-inheriting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use unqualified wrap_plots() (imported via @importFrom) instead of patchwork::wrap_plots() at all three call sites — makes the NAMESPACE import meaningful. - Remove stale result$continuous / result$categorical lines from plot.gg_partialpro() example; the return is now a single patchwork object, not a named list. - Regenerate man/plot.gg_partialpro.Rd. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 19, 2026
ehrlinger
added a commit
that referenced
this pull request
May 20, 2026
…#83) * docs: randomForest engine validation design spec (#82) Approved design for the v2.8-cycle randomForest validation & repair initiative (meta #82, groups #80/#81). Coverage-first/TDD: build the full randomForest test matrix red, fix plot.gg_variable (#80) and calc_roc.randomForest (#81, macro-average default), minor gg_rfsrc wart, Rd-doc the rfsrc-only families. Own branch off main@f3ffd38; not the varPro Phase plans. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: correct RF validation spec root causes against source Pre-plan source review found the initial spec mis-stated all three defects. Corrected: - #80: plot.gg_variable bare-list return is the *documented* multi-xvar contract (engine-agnostic, contract-tested at test_gg_variable.R:38-41) — fix is an intentional breaking list->single-object change like v2.7.3 #77/#78 (needs @return + NEWS + test rewrite), not an rfsrc-specific bug. - #81: degeneracy is a matrix mis-index prd[[k]] (calc_roc.R:166), not hard-labels; plus dropped oob plumbing. Macro-average lives ONLY in the randomForest path; shared .validate_which_outcome / calc_roc.rfsrc left byte-unchanged. - minor wart is plot.gg_error.R:244,248 (not plot.gg_rfsrc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: randomForest engine validation implementation plan (#82) 10-task coverage-first/TDD plan off the corrected design spec: dev bump, fresh baseline, RF structural matrix + ROC correctness (red), fix #80 (plot.gg_variable list->patchwork) / #81 (calc_roc.randomForest indexing+oob+macro-average, shared helpers untouched) / plot.gg_error wart, vdiffr snapshots, rfsrc-only Rd notes, final 0/0/0 gate -> PR (no merge). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: open 2.7.3.9001 dev increment for randomForest validation (#82) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: sync NEWS.md Version header to 2.7.3.9001 (#82) Phase 0 convention: NEWS.md line 2 Version: must mirror DESCRIPTION Version: (ggrandomforests.news() reads NEWS.md). RF1's first execution missed it because the plan's Step 3 under-specified. Plan corrected: Step 3 now requires the header sync, with a guard assertion in Step 4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: randomForest plot matrix — #80/#81/wart fail red (#82) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: structural gg_error wart assertion (warning-based was brittle) (#82) ggplot2 emits "Ignoring unknown labels" once per session (cli .frequency="once"), so expect_no_warning(ggplot_build(p)) is unreliable inside a long test_file — earlier tests can exhaust the once-per-session cap before the wart test runs. Replace with a structural check: if p$labels$colour is set, a colour aesthetic must be mapped on the plot or one of its layers. Deterministic, robust, tests the actual code defect at plot.gg_error.R:244,248. Plan Task 3 test code synced. Also clarified Step 2's expected red set: 3 tests fail (gg_variable cls+regr #80, gg_error wart), not 4 — gg_roc structural is correctly green pre-fix (the real #81 RED is in RF4 ROC numeric correctness). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: RF ROC correctness fails red; rfsrc characterization green (#81/#82) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: plot.gg_variable returns single ggplot/patchwork, not a list (#80) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: make expect_layer_nonempty patchwork-aware; tighten #80 iris assertion #80's fix made plot.gg_variable return a patchwork for default multi-xvar. expect_layer_nonempty's layer_data(p, 1) errored on the composite -> unwrap to first sub-plot via p[[1]] (patchwork sub-setting). For iris-classification specifically, the per-variable geom_smooth over multi-class yhat.* still can't compute stat_smooth (pre-existing classification gg_variable build quirk, independent of #80 — candidate follow-up under #82). The #80 contract is the structural invariant (a single composable patchwork with >1 sub- plot); the iris test now asserts that only. The mtcars regression test keeps the layer_data check (works fine on continuous yhat). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: correct randomForest ROC (probabilities, oob, macro-average) (#81) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: drop unconditional color='Outcome' label on gg_error regression plot (#82) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: vdiffr snapshots for the randomForest plot matrix (#82) Adds visual-regression baselines for randomForest classification (iris) and regression (mtcars) across gg_rfsrc, gg_error, gg_vimp, gg_roc, and gg_variable plot families (8 of 9 — gg_variable iris classification is blocked by the loess/geom_smooth defect logged under #82). Also commits the rfsrc baselines (previously unrecorded in this worktree). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: note gg_partial/gg_survival/gg_brier are randomForestSRC-only (#82) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: calc_roc.randomForest cross-platform — use table() not xtabs() (#81) The inner mclapply(...) closure called xtabs(~ res + (score > crit)) without a data= argument, relying on lexical scope for `res`/`score`. That works under Linux/mac (mclapply forks; child inherits parent env), but FAILS on Windows where mclapply degrades to serial and model.frame can't resolve the symbols — surfacing as "Error in model.frame.default(formula = ~res + (score > crit), ...) Execution halted" during R CMD check examples on windows-latest. Swap to base table(res, score > crit): no formula, no NSE, no model.frame, cross-platform. Numerical output identical (table() returns the same 2x2 contingency with the same positional indexing the downstream tbl[2,2]/rowSums(tbl)[2] uses); local devtools::test() still FAIL 0 | PASS 693; AUC on iris setosa stays 1.0000, default macro-average AUC 0.9939. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: resolve PR #83 review items + drop superfluous mclapply (#82) Resolves all three Copilot inline review items and one CI failure that slipped through, plus a small simplification raised in PR review. calc_roc.randomForest: - Validate which_outcome up front (Copilot): accept 0/0L/"all" as the overall case (was: identical(., 0) missed 0L); accept character class names ("setosa") via match()->index (was: passed through and produced NA via lvls[k]); error clearly on unknown class names and out-of-range numeric indices. The shared .validate_which_outcome and calc_roc.rfsrc remain byte-unchanged (per spec; rfsrc non-goal). - Drop parallel::mclapply for plain lapply: per-threshold work is one table() + arithmetic (microseconds), so fork overhead dominates on Linux/mac and mclapply degrades to serial on Windows anyway. Removing it also eliminates the closure-scope fragility that caused the earlier xtabs/Windows failure. - Fix the calc_roc roxygen example, which passed rf_iris$yvar (an rfsrc-style slot that is NULL on a randomForest object) — the new validation surfaces it as a clean error instead of silent wrong output. Use iris$Species directly. gg_roc roxygen (Copilot): qualify the which_outcome doc so the macro-average behaviour is correctly attributed to the randomForest method only; explicitly call out that the rfsrc method still warns + falls back to class 1 (#72 tracks rfsrc multi-class / CI separately). plot.gg_variable roxygen (Copilot): drop the inaccurate claim that the multi-xvar patchwork "works with ggplot2::layer_data()" — it does not operate on patchwork composites. Note callers should extract a panel first (e.g. layer_data(p[[1]])). plot.gg_error: the RF7 conditional labs() fix packed everything onto a 166-char line, tripping the line_length_linter (CI lint job). Factor into a single `err_labs` value built with a clear if/else; both geom branches use it. No behaviour change. R CMD check --as-cran: 0 errors / 0 warnings / 0 notes locally. devtools::test(): [ FAIL 0 | PASS 693 ]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: extract helpers from calc_roc.randomForest for cyclocomp (#82) The which_outcome validation block + inner one_class_roc nested function pushed cyclomatic complexity to 25 (lintr limit 20). Same pattern v2.7.0 used for gg_partial_rfsrc: extract top-level unexported helpers, leaving the dispatch function thin. New helpers: - .rf_prob_matrix(object, oob, lvls): votes-or-predict + row-normalise - .normalize_which_outcome(which_outcome, lvls): coerce/validate - .rf_one_class_roc(dta, prob, k, lvls): per-class sweep (plain lapply) - .rf_macro_average_roc(dta, prob, lvls): macro-average one-vs-rest Pure refactor — behaviour byte-identical: gg_roc(rf, which_outcome=1L) still AUC 1.0000 (16 rows); gg_roc(rf, "setosa") same; 0L/"all" both return the 200-row macro-average AUC 0.9939; error messages unchanged. devtools::test() [ FAIL 0 | PASS 693 ]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
What this does
Fixes the inconsistent return type from
plot.gg_partial(),plot.gg_partial_rfsrc(), andplot.gg_partialpro().Before: when both continuous and categorical predictors were present, these functions returned
list(continuous = <ggplot>, categorical = <ggplot>)— a surprise to users and ambiguous forautoplot().After: always returns a single
ggplot/patchworkobject. When both panel types are present they are combined vertically withpatchwork::wrap_plots(gg_cont, gg_cat, ncol = 1).inherits(p, "ggplot")isTRUEfor both single and combined cases.Changes
R/plot.gg_partial.R— replacelist(...)return withpatchwork::wrap_plots()at all 3 return sites; update@returndocs; add@importFrom patchwork wrap_plotsDESCRIPTION— patchwork moved fromSuggeststoImportsNAMESPACE—importFrom(patchwork, wrap_plots)regeneratedNEWS.md— entry added under v2.7.3expect_s3_class(out, "ggplot");expect_ggplot_result()helper removed from autoplot testsTest plan
devtools::test()— 677 pass, 0 failplot(gg_partial(...))returns a singleggplotwhen both variable types presentautoplot(gg_partial(...))returns a singleggplotCloses #77.
🤖 Generated with Claude Code