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

Move {mgcv} to suggests #5987

Merged
merged 12 commits into from
Aug 26, 2024
Merged

Move {mgcv} to suggests #5987

merged 12 commits into from
Aug 26, 2024

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Jul 9, 2024

This PR fixes #5986 and closes #5985.

Briefly, when method = "gam" and {mgcv} is not installed, we fallback to method = "lm" with a warning.
Needed a mock test to verify correct message is thrown and add one skip step.

Please note that downstream dependencies that use geom_smooth(method = "gam") will need to add {mgcv} to their dependencies.

@clauswilke
Copy link
Member

@teunbrand I believe this is a reasonable solution but just wanted to raise one potential issue: I believe once mgcv is in suggests it no longer gets installed automatically when people install ggplot2. This means most people will likely not have it installed going forward. I don't know what to do about this, just one of the annoyances of the R ecosystem that I've run into a couple of times. (I tell my students to install packages X, Y, and Z, and then key functionality is missing because some dependencies have been moved into suggests and no longer are installed automatically.)

@clauswilke clauswilke mentioned this pull request Jul 9, 2024
@yutannihilation
Copy link
Member

This means most people will likely not have it installed going forward.

I think it's a good point. I believe rlang::check_installed(), which interactively asks the user to install the package, should solve this as we do in several places.

https://github.com/search?q=repo%3Atidyverse%2Fggplot2%20check_installed&type=code

@clauswilke
Copy link
Member

clauswilke commented Jul 9, 2024

@yutannihilation Ah, good point. Yes, this seems like the way to go.

Edit: Oh, but then there's no fallback in case the user doesn't want to install the package, right? It's just a hard check.

@teunbrand
Copy link
Collaborator Author

Yeah the lack of fallback is why I didn't use that initially, but giving it a 2nd thought we could probably wrap it in a try_fetch() block to set a fallback.

@clauswilke
Copy link
Member

Thinking some more about it, this can get rather cumbersome quickly if somebody can't install mgcv for some reason and gets prompted to do it every time they try to make a plot.

Here is an alternative to consider: ggplot could check on startup (library load) what packages are installed/missing and on occasion (once per session, once per week, whatever) print a message that lists suggested but not installed packages that would enable additional functionality.

Such a function could also not be run automatically but be available for manual checking.

@teunbrand
Copy link
Collaborator Author

this can get rather cumbersome quickly if somebody can't install mgcv for some reason and gets prompted to do it every time they try to make a plot.

This would only trigger when method = "gam", either explicitly or by default when nrow(data) > 1000. If they don't want to be prompted about this, they could set the method argument to something else in order to opt-out these prompts.

@clauswilke
Copy link
Member

I see. Maybe mention this in the message that is generated?

@teunbrand
Copy link
Collaborator Author

wrap it in a try_fetch() block

This turns out trickier than anticipated since check_installed() has some restart condition that is too slippery for me to catch in a try_fetch() block.

ggplot could check on startup (library load) what packages are installed/missing

It would be handy to have a ggplot2::install_suggested() function to install the functional dependencies.

@teunbrand
Copy link
Collaborator Author

This is what happens now if {mgcv} is not installed in non-interactive versions.

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

rlang::is_installed("mgcv")
#> [1] FALSE

ggplot(mpg, aes(displ, hwy)) +
  geom_smooth(method = "gam")
#> The `method` was set to "gam", but mgcv is not installed.
#> ! Falling back to `method = "lm"`.
#> ℹ Change the `method` argument to silence this message.
#> `geom_smooth()` using formula = 'y ~ x'

Created on 2024-07-11 with reprex v2.1.0

In interactive versions, you'll be prompted to install.
If you refuse, you get the same as the example above.
If you install {mgcv}, you continue with method = "gam" as per usual.

@clauswilke
Copy link
Member

Does the message also appear when people don't set the method explicitly but just use a large dataset with >1000 points? If yes then I'd suggest to say something like "Install mgcv or change the method argument ..."

@teunbrand
Copy link
Collaborator Author

Thanks for the suggestion Claus, it now reads:

The method was set to "gam", but mgcv is not installed.
! Falling back to method = "lm".
ℹ Install mgcv or change the method argument to resolve this issue.

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@teunbrand teunbrand merged commit bcb87fc into tidyverse:main Aug 26, 2024
13 checks passed
@teunbrand teunbrand deleted the mgcv_suggests branch August 26, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review dependencies
4 participants