jtlandis / ggside

ggplot2 extension allowing for plotting various geometries as side panels using the ggplot2 API
Other
339 stars 16 forks source link

not be compatible with github version of ggplot2 #36

Closed xiangpin closed 1 year ago

xiangpin commented 2 years ago

Thanks for the great package. But the development version of ggplot2 has some big (changes)[https://github.com/tidyverse/ggplot2/blob/main/NEWS.md#ggplot2-development-version]. the current version of ggside is not compatible with it.

> library(ggplot2)
> packageVersion('ggplot2')
[1] ‘3.3.6.9000’
> library('ggside')
Registered S3 method overwritten by 'ggside':
  method from
  +.gg   ggplot2
> packageVersion('ggside')
[1] ‘0.2.1’
> example(geom_xsideboxplot)

gm_xsd> df <- expand.grid(UpperCase = LETTERS, LowerCase = letters)

gm_xsd> df$Combo_Index <- as.integer(df$UpperCase)*as.integer(df$LowerCase)

gm_xsd> p1 <- ggplot(df, aes(UpperCase, LowerCase)) +
gm_xsd  geom_tile(aes(fill = Combo_Index))

gm_xsd> #sideboxplots
gm_xsd> #Note - Mixing discrete and continuous axis scales
gm_xsd> #using xsideboxplots when the y aesthetic was previously
gm_xsd> #mapped with a continuous varialbe will prevent
gm_xsd> #any labels from being plotted. This is a feature that
gm_xsd> #will hopefully be added to ggside in the future.
gm_xsd>
gm_xsd> p1 + geom_xsideboxplot(aes(y = Combo_Index)) +
gm_xsd     geom_ysideboxplot(aes(x = Combo_Index))
Hit <Return> to see next plot:
Error in `geom_xsideboxplot(aes(y = Combo_Index))` at tmp/Rtmpnv82lq/Rex6e5c441e74f5:21:0:
! Problem while mapping stat to aesthetics.
ℹ Error occurred in the 2nd layer.
Caused by error in `switch()`:
! EXPR must be a length 1 vector
Run `rlang::last_error()` to see where the error occurred.
Warning messages:
1: Computation failed in `stat_boxplot()`
Caused by error in `stats::quantile()`:
! `quantile.ggplot2_mapped_discrete()` not implemented.
2: Computation failed in `stat_boxplot()`
Caused by error in `stats::quantile()`:
! `quantile.ggplot2_mapped_discrete()` not implemented.
> example(geom_xsidedensity)

gm_xsd> ggplot(mpg, aes(displ, hwy, colour = class)) +
gm_xsd   geom_point(size = 2) +
gm_xsd   geom_xsidedensity() +
gm_xsd   geom_ysidedensity() +
gm_xsd   theme(axis.text.x = element_text(angle = 90, vjust = .5))
Hit <Return> to see next plot:
Error in `geom_xsidedensity()` at tmp/Rtmpnv82lq/Rex6e5c5b0f51b8:8:0:
! Problem while converting geom to grob.
ℹ Error occurred in the 2nd layer.
Caused by error in `[.data.frame`:
! undefined columns selected
Run `rlang::last_error()` to see where the error occurred.
jtlandis commented 2 years ago

Thanks for bringing this to my attention. I'll have a look soon

jtlandis commented 2 years ago

I found the issue to this specific issue and it should be fixed on the dev branch. Unfortunately it seems that there are a few more changes in ggplot2 that may affect certain features of ggside, such as mixing continuous and discrete axises.

xiangpin commented 2 years ago

I also found the issue. Moreover, when the order between geom layer of ggplot2 and geom layer of ggside was changed, the issue would be missed.

geom of ggplot2 + geom of ggside

> library(ggside)
Loading required package: ggplot2
Registered S3 method overwritten by 'ggside':
  method from
  +.gg   ggplot2
> ggplot(mpg, aes(displ, hwy, colour = class)) -> p
> f <- p + geom_point()
> f + geom_xsideboxplot(aes(y = class), orientation = 'y')
Error: Discrete value supplied to continuous scale

geom of ggside + geom of ggplot2

> f2 <- p + geom_xsideboxplot(aes(y = class), orientation = 'y')
> f2 + geom_point()

xx

PS: I also add the S3 method to solve the issue of stats::quantile in stat_boxplot. And I hope this helps you to solve the problem.

#' @method quantile ggplot2_mapped_discrete
#' @importFrom stats quantile
#' @export
quantile.ggplot2_mapped_discrete <- function(x, ...){
  quantile(vctrs::vec_data(x))
}
jtlandis commented 2 years ago

I have already left a feature request with the maintainers of ggplot2. You can track it here. In this issue I have suggested a small change that would help not break ggside. At the very end I show how their new S3 class may have unintentional effects if it ever makes it to an unintended generic. I am going to forego troubleshooting for generics that break with the ggplot2_mapped_discrete class for a couple of reasons.

  1. ggplot2 v3.4.0 is still under development and things may change
  2. Attempting to find all possible hedge cases may take more effort than it's worth (and would be a massive undertaking to maintain)
  3. The solution provided to the ggplot2 maintainers should be sufficient to fix their switch to vctrs, if they choose to keep it. If they do not, I may be able to implement it myself as part of ggside, which is less than ideal as it is yet another ggproto object ggside would extend that could break down the line.

For now, we will wait. The current dev branch of ggside is a bit more compatible with the dev version of ggplot2 (except for when Positional scales are mixed between the side panel and main panel). When ggplot2 comes closer to releasing its new minor version, I will expedite development to be compatible with ggside.

There are other things in the works (not yet apart of the dev branch) that still yet make it into their final release that I have been keeping an eye on.

xiangpin commented 2 years ago

Thank you, I got it.

jtlandis commented 1 year ago

So as it stands ggplot2 3.4.0 has been released and the current dev version is able to pass all its tests locally. I am working on a release for ggside to go with what made it into ggplot2 3.4.0. Once I release the new version I may close this issue for the time being.

There are a lot of things that can change in ggplot2 and I haven't set up any automatic pipelines to notify me when ggplot2's dev branch is incompatible with my main and dev branches.

xiangpin commented 1 year ago

Ok, thank you