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

Fix ggplot2 update #252

Merged
merged 5 commits into from
Nov 17, 2022
Merged

Fix ggplot2 update #252

merged 5 commits into from
Nov 17, 2022

Conversation

nikosbosse
Copy link
Contributor

version 3.4.0 of ggplot2 deprecated the use of "size" in geom_line() and similar.
The problem is that I think in older version you can't use linewidth and it will error. So not sure when the best moment is to switch.
Also I think the tests generate lots of warnings due to the same issue in the ggdist pacakge. @Bisaloo do you have an idea about what the best thing forward would be?

@Bisaloo
Copy link
Contributor

Bisaloo commented Nov 7, 2022

You should be fine if you set 3.4.0 as the minimum required version for ggplot2 in DESCRIPTION. This is what we have done for ggdist for example:

ggdist (>= 3.1.0),

@nikosbosse
Copy link
Contributor Author

My concern would be that it forces people to update to the newest version without a fundamental need. Do you think that is a problem?

1 similar comment
@nikosbosse
Copy link
Contributor Author

My concern would be that it forces people to update to the newest version without a fundamental need. Do you think that is a problem?

@Bisaloo
Copy link
Contributor

Bisaloo commented Nov 10, 2022

No, I think it's perfectly fine. There is a fundamental need to update: to stop using deprecated options.

This is very different from the case where we require a minimum R version. Updating an R package shouldn't be problem for most users while updating R is almost impossible in some very restricted environments.

@nikosbosse
Copy link
Contributor Author

Makes sense.
I'm not entirely sure we can currently prevent the tests from failing.
When running devtools::test() I'm getting the following warning:

Warning (test-plot_predictions.R:93): plot_predictions() works without median
Using the size aesthietic with geom_ribbon was deprecated in ggplot2 3.4.0.
i Please use the linewidth aesthetic instead.

But I think that is an issue with ggdist and I guess we just have to wait for them to address it.

This is our call to geom_lineribbon, and I don't see any usage of size

ggdist::geom_lineribbon(
        data = intervals,
        aes(
          ymin = lower, ymax = upper,
          # We use the fill_ramp aesthetic for this instead of the default fill
          # because we want to keep fill to be able to use it for other
          # variables
          fill_ramp = factor(range, levels = sort(unique(range), decreasing = TRUE))
        ),
        lwd = 0.4
      )

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #252 (b7f896a) into master (5b6cd5f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   91.32%   91.32%           
=======================================
  Files          22       22           
  Lines        1361     1361           
=======================================
  Hits         1243     1243           
  Misses        118      118           
Impacted Files Coverage Δ
R/plot.R 97.96% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nikosbosse
Copy link
Contributor Author

huh, interesting - they fail on my machine, but not in CI. well then.. Merging!

@nikosbosse nikosbosse merged commit 1bffc42 into master Nov 17, 2022
@nikosbosse nikosbosse deleted the fix-ggplot2-update branch November 17, 2022 18:16
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.

2 participants