-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[R-package] factor out {ggplot2} #3224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the idea to drop heavy dependency!
f884a2c
to
69bcdcf
Compare
@jameslamb Can we now remove these lines? LightGBM/.ci/test_r_package.sh Lines 117 to 118 in 5056057
LightGBM/.ci/test_r_package_windows.ps1 Line 141 in 5056057
|
oooooooooooooooo yes we can! I forgot about those. Yes I'll remove them right now. We can always add back if we introduce new I'll open a PR right now, thanks for the suggestion! |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Today the R package has a
Suggests
dependency on{ggplot2}
. This is only for one demo,demo/leaf_stability.R
. This pull request proposes factoring it out and replacing it withbase
R code.This will allow us to cut out this
R CMD check
NOTE:More importantly, it's an expensive dependency that anyone installing the
Suggests
dependencies of{lightgbm}
has to pay for.Some of these plots look different from the comments around them, but this PR does not address that...it can be addressed in #1944 . This PR is limited to just factoring out
{ggplot2}
, to make the package as lightweight as possible when it goes to CRAN (work-in-progress, #629 ).Comparisons
I've included before and after screenshots for each plot below. The "Depth Density" plots look noticeably different, I think because of changes in how
geom_density()
works in{ggplot2}
.demo/leaf_stability.R
was written more than 3 years ago.Thanks to @alistaire47 for helping me with
{ggplot2}
stuff 😀plot 1
before
after
plot 2
before
after
plot 3
before
after
plot 4
before
after
plot 5
before
after
plot 6
before
after
plot 7
before
after
plot 8
before
after