fix: plot.gg_variable default multi-class render error (yvar not found)#93
Merged
Conversation
…not predictors) The default-xvar selection in plot.gg_variable stripped only yhat/event/time columns, leaving yvar (the response factor added by gg_variable for classification) and outcome (the facet column added by the multi-class pivot) to be treated as predictors. They were then pivoted into `var`, dropping the column the downstream geom_jitter aes referenced, and the patchwork errored when actually rendered with "Column `yvar` not found in `.data`". Invisible to CI: the existing test only asserted patchwork class (lazy), and vdiffr snapshots run with VDIFFR_RUN_TESTS=false. New test exercises a real ggplot_build on every patchwork sub-plot. Add yvar and outcome to the exclusion list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 87.31% 87.31% -0.01%
==========================================
Files 38 38
Lines 3098 3097 -1
==========================================
- Hits 2705 2704 -1
Misses 393 393
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a render-time failure when plotting multi-class gg_variable outputs with default xvar selection, by ensuring response/structural columns aren’t accidentally treated as predictors and by adding a regression test that forces patchwork subplots to build.
Changes:
- Exclude
yvarandoutcomefrom the defaultxvarcandidate set inplot.gg_variable(). - Add a regression test that calls
ggplot_build()on each patchwork subplot for a multi-classrandomForestclassification. - Document the fix in
NEWS.mdand bump package version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/testthat/test_gg_variable.R |
Adds a regression test that forces patchwork panels to build to catch render-time aesthetic errors. |
R/plot.gg_variable.R |
Adjusts default xvar exclusion list to avoid pivoting away columns needed for downstream aesthetics in multi-class plots. |
NEWS.md |
Adds a changelog entry describing the bug and the new test coverage. |
DESCRIPTION |
Bumps the development version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…grep)
Copilot review caught a latent bug I inherited when fixing the yvar/outcome
issue: grep("time", colnames(gg_dta)) silently excludes any predictor whose
name contains the substring "time" -- the documented veteran-data survival
predictor `diagtime` was being dropped from the default xvar list. Same for
grep("event", ...) on any predictor containing "event".
Switch the exclusion to exact name matching for event/time/yvar/outcome via
%in%, and anchor the yhat match as ^yhat(\\.|$) so it still catches both the
bare yhat column (regression / single-class survival) and the yhat.<class>
columns (classification gg_variable). New test on the veteran survival
forest confirms diagtime is included as a panel.
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.
Summary
plot(gg_variable(rf_classification))(default — noxvar) errored at render time on a multi-class randomForest classification forest:Root cause: the default-xvar selection in
plot.gg_variableexcludedyhat/event/timecolumns but notyvar(the observed-class response column added bygg_variablefor classification) oroutcome(the facet column added by the multi-class pivot). Both got iterated as predictors and pivoted intovar, dropping the column the downstreamgeom_jitter(aes(color = yvar))needed.Invisible to CI because the existing test only asserted patchwork class (lazy — never builds the sub-plots) and vdiffr snapshots run with
VDIFFR_RUN_TESTS = false.Fix
yvarandoutcometo the default-xvar exclusion list inR/plot.gg_variable.R.ggplot_buildon every patchwork sub-plot — this is what catches it.Test plan
devtools::test(filter = 'gg_variable')— 0 failuresdevtools::check(args = "--as-cran")— 0 errors, 0 warnings, 0 notes🤖 Generated with Claude Code