Skip to content
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

Merged
merged 6 commits into from
Jul 20, 2020

Conversation

jameslamb
Copy link
Collaborator

@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
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!

R-package/demo/leaf_stability.R Show resolved Hide resolved
@guolinke guolinke merged commit 9f52282 into microsoft:master Jul 20, 2020
@StrikerRUS
Copy link
Collaborator

@jameslamb Can we now remove these lines?

# suppress R CMD check warning from Suggests dependencies not being available
export _R_CHECK_FORCE_SUGGESTS_=0

$env:_R_CHECK_FORCE_SUGGESTS_ = 0

@jameslamb
Copy link
Collaborator 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

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