tidyverse / ggplot2

An implementation of the Grammar of Graphics in R
https://ggplot2.tidyverse.org
Other
6.46k stars 2.02k forks source link

Review dependencies #5986

Closed teunbrand closed 1 week ago

teunbrand commented 1 month ago

This issue is prompted by #5985.

Briefly, {mgcv} depends on {nlme} which requires R >= 4.0.0. We only use mgcv::gam() in geom_smooth() when there are >1000 points to fit, so it is a dependency with a very narrow application. We should probably find a way to make this a suggested package rather than an imported package.

teunbrand commented 1 month ago

This got me thinking about other dependencies as well.

In principle, the reasoning that 'mgcv applies to only a small number of functions and is not part of the core functioning of the rest of the package' also applies to {isobands}. It was reasoned in #3564 that {isobands} should be under 'Imports' to prevent revdep failures, but I think the appropriate place is in 'Suggests'.

Similarly, {MASS} is only required for a few functions so it might also be listed in 'Suggests'.

We also have a dependency on {glue}, which has largely (but not completely) become obsolete due to {cli}. We don't actually require glue as we don't expose it to users, it appears to only exist for the ease of developers. It might be worth the effort to wrangle out the residual pieces of {glue} so we can unlist it as a dependency.

In addition, it appears to me like {tibble} is only used twice in actual functions and further only exists in examples or for printing data prettily. This seems like it can also be moved from 'Imports' to 'Suggests'.

clauswilke commented 1 month ago

See my comment here: https://github.com/tidyverse/ggplot2/pull/5987#issuecomment-2217774943

Again, I don't know what the right solution is, as ggplot2 does have a wide range of weird dependencies, but the more you remove them the more you'll run into the issue that people can't do things they think of as core functionality.

yutannihilation commented 1 month ago

I'd add one more concern; moving to Suggests means we are accepting the related test might get skipped, which can make us overlook some unexpected degradation of the feature.

I don't think this point is so critical, but I feel the risk might not be worth just for saving R <= 3.6. We don't have the CI on the versions so we cannot confirm ggplot2 works soundly on the versions. I understand it's a good idea in general to remove a hard dependency if the package is used in a very limited way.

teunbrand commented 1 month ago

Yeah I agree that this is a concern. I also lament that there is no good distinction for packages that are suggested because they enable functionality (like {isobands}, {mgcv}, {MASS}) and packages that are needed for development (like {roxygen2}, {testthat}, {vdiffr}).

I guess the main reason one would want to limit the dependencies is because of ecosystem reasons: any dependency we could shave off ggplot2 can also be dropped in all downstream packages. For example, there is no reason why one must have {isobands} installed if you just want to make a plot with {ggridges}.

For interactive use, check_installed() is a good solution but developers might run into problems if their package depends on ggplot2's geom_contour() or geom_smooth(method = "gam").