docs/test: smooth=TRUE is intentionally a no-op for factor predictors in plot.gg_variable#89
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
=======================================
Coverage 87.11% 87.11%
=======================================
Files 38 38
Lines 3043 3043
=======================================
Hits 2651 2651
Misses 392 392
🚀 New features to boost your workflow:
|
… in plot.gg_variable geom_smooth requires a continuous x-axis; for factor predictors the boxplot IQR already serves as the spread summary. Add inline comments to all three factor-predictor branches (binary classification, multi-class classification, regression) and three regression-test guard tests to lock in the no-op behaviour.
ehrlinger
force-pushed
the
fix/plot-gg-variable-smooth-factor-comment
branch
from
d714218 to
4f9dc48
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Documents and locks in the existing behavior that smooth=TRUE is intentionally a no-op when plot.gg_variable() is called with a factor predictor (since geom_smooth requires a continuous x-axis).
Changes:
- Added inline comments in
plot.gg_variable()explaining why smoothing is skipped for factor predictors across families. - Added test coverage for binary classification, multi-class classification, and regression ensuring
smooth=TRUEwith factor predictors does not error. - Added assertions (in some cases) that the resulting plot has two layers (boxplot + jitter) with no smooth layer.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/testthat/test_gg_variable.R | Adds tests asserting smooth=TRUE is a no-op with factor predictors and does not error. |
| R/plot.gg_variable.R | Adds inline documentation clarifying intentional smoothing skip for factor predictors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace brittle `length(p$layers) == 2` with a direct GeomSmooth-absence check (has_smooth_layer helper) — the actual contract is "no smooth layer for a factor predictor", insensitive to benign layer-composition changes. - Multi-class test now asserts no smooth layer too (was missing the check). - Call plot() once per test via `expect_no_error(p <- plot(...))` instead of twice with identical args. - Drop ntree 50 -> 25; model quality is irrelevant to these tests. 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
geom_smoothrequires a continuous x-axis; passingsmooth = TRUEwith a factor predictor is a silent no-op across all three plot families (binary classification, multi-class classification, regression). This was pre-existing behaviour that was consistent and correct, but undocumented.smooth=TRUEwith a factor predictor (a) does not error, (b) returns a ggplot with exactly 2 layers (boxplot + jitter, no smooth)Pre-existing gap first flagged during review of PR #88 (gg_variable classification fix).
Test plan
devtools::test(filter='gg_variable')— 47 tests, 0 failures🤖 Generated with Claude Code