corybrunson / ggalluvial

ggplot2 extension for alluvial plots
http://corybrunson.github.io/ggalluvial/
GNU General Public License v3.0
499 stars 34 forks source link

No supporting the ggplot2 github version (>3.3.6) #103

Closed xiangpin closed 1 year ago

xiangpin commented 2 years ago

Description of the issue

The next version of ggplot2 will have some changes. But now the ggalluvial is not compatible with the version. You can see the following example.

Reproducible example (preferably using reprex::reprex())

> library(ggplot2)
> packageVersion('ggplot2')
[1] ‘3.3.6.9000’
> library(ggalluvial)
> example(geom_alluvium)

gm_llv> # basic
gm_llv> ggplot(as.data.frame(Titanic),
gm_llv         aes(y = Freq,
gm_llv             axis1 = Class, axis2 = Sex, axis3 = Age,
gm_llv             fill = Survived)) +
gm_llv    geom_alluvium() +
gm_llv    scale_x_discrete(limits = c("Class", "Sex", "Age"))
Hit <Return> to see next plot:
Error in `geom_alluvium()` at tmp/RtmpRpKSMn/Rex6e056fb07094:8:0:
! Problem while converting geom to grob.
ℹ Error occurred in the 1st layer.
Caused by error in `vec_slice()`:
! Must subset elements with a valid subscript vector.
ℹ Logical subscripts must match the size of the indexed input.
✖ Input has size 3 but subscript `i` has size 2.
Run `rlang::last_error()` to see where the error occurred.
> example(geom_stratum)

gm_str> # full axis width
gm_str> ggplot(as.data.frame(Titanic),
gm_str         aes(y = Freq,
gm_str             axis1 = Class, axis2 = Sex, axis3 = Age, axis4 = Survived)) +
gm_str    geom_stratum(width = 1) +
gm_str    geom_text(stat = "stratum", aes(label = after_stat(stratum))) +
gm_str    scale_x_discrete(limits = c("Class", "Sex", "Age", "Survived"))
Hit <Return> to see next plot:
Error in `geom_stratum(width = 1)` at tmp/RtmpRpKSMn/Rex6e053a7f5804:8:0:
! Problem while converting geom to grob.
ℹ Error occurred in the 1st layer.
Caused by error in `check.length()`:
! 'gpar' element 'lwd' must not be length 0
Run `rlang::last_error()` to see where the error occurred.
corybrunson commented 2 years ago

Thank you @xiangpin for bringing this up! I will have a close look and do my best to resolve it this weekend at the latest.

corybrunson commented 2 years ago

I am thoroughly confused about this but still digging in.

First, a couple of issues due to the (i think?) newly introduced 'vctrs_vctr' classes, which do not tolerate slick base R behavior like letters[c(TRUE, FALSE)], have been identified and will be easy to resolve.

One remaining problem lies with the pairing of StatAlluvium and GeomPointrange. The issue does not arise with GeomLinerange and only arises when the legend is drawn. I'm trying to figure out whether this patch for this issue is the source, and i'll have to come back to it later.

corybrunson commented 2 years ago

The remaining problem arises here (introduced in this commit):

#' @export
#' @rdname draw_key
draw_key_pointrange <- function(data, params, size) {
  grobTree(
    draw_key_linerange(data, params, size),
    draw_key_point(transform(data, params, size = (data$size %||% 1.5) * 4))
  )
}

Some values in param, as passed from StatAlluvium, are NULL, which produces this error:

> transform(data.frame(a = 1, b = 2), list(c = NULL), d = 4)
Error in data.frame(list(a = 1, b = 2), list(c = NULL), d = 4) : 
  arguments imply differing number of rows: 1, 0

Maybe this was an oversight at ggplot2, but it may be good hygiene anyway for stats to not pass null parameter values to geoms. The following line in Stat*$setup_params() (which must be added if missing) solves the problem:

params[vapply(params, is.null, NA)] <- NULL

I plan to raise an issue at ggplot2 (if not already raised), either to remove the bug or to understand why it is a feature.

xiangpin commented 2 years ago

I think this issue might be introduced by the commit, which introduced vctrs internally in ggplot2

corybrunson commented 2 years ago

Wow, that is some commit. I think you're right that the original issue you flagged arose from the use of vctrs, and i think it's resolved in the ggplot2-upgrade branch.

Testing uncovered another issue, though, described in this issue. It seems like an accident, but i'll wait to see what the developers say (or until the development version is released, in case it's not addressed) before releasing the fix here.

xiangpin commented 2 years ago

Thank you, I got it.

corybrunson commented 1 year ago

This seems to have been resolved, as documented in the issue linked above.