Skip to content

[R-package] factor out {ggplot2}#3224

Merged
guolinke merged 6 commits intolightgbm-org:masterfrom
jameslamb:fix/remove-ggplot2
Jul 20, 2020
Merged

[R-package] factor out {ggplot2}#3224
guolinke merged 6 commits intolightgbm-org:masterfrom
jameslamb:fix/remove-ggplot2

Conversation

@jameslamb
Copy link
Copy Markdown
Member

@jameslamb jameslamb commented Jul 13, 2020

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 with base R code.

This will allow us to cut out this R CMD check NOTE:

  • checking package dependencies ... NOTE
    Package suggested but not available for checking: ‘ggplot2’

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
image

after

image

plot 2

before

image

after

image

plot 3

before

image

after

image

plot 4

before

image

after

image

plot 5

before

image

after

image

plot 6

before

image

after

image

plot 7

before

image

after

image

plot 8

before

image

after

image

@jameslamb jameslamb requested review from Laurae2 and guolinke July 13, 2020 03:47
@jameslamb jameslamb requested a review from StrikerRUS as a code owner July 13, 2020 03:47
@StrikerRUS StrikerRUS removed their request for review July 13, 2020 13:19
Copy link
Copy Markdown
Collaborator

@StrikerRUS StrikerRUS left a 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!

@guolinke guolinke merged commit 9f52282 into lightgbm-org:master Jul 20, 2020
@jameslamb
Copy link
Copy Markdown
Member Author

oooooooooooooooo yes we can! I forgot about those. Yes I'll remove them right now. We can always add back if we introduce new Suggests dependencies (like whenever #1944 is picked up, for rmarkdown) but for now it would be better to simplify those files.

I'll open a PR right now, thanks for the suggestion!

@github-actions
Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants