YuLab-SMU / scatterpie

:art: scatter pie plot
https://cran.r-project.org/package=scatterpie
60 stars 15 forks source link

Some identified bugs #9

Closed larmarange closed 4 years ago

larmarange commented 6 years ago

Hello,

thank you for your package. When trying to use it, I have identified some bugs:

https://github.com/GuangchuangYu/scatterpie/blob/8a42783947a02ea45c55ac956f70dae559a27788/R/geom_scatterpie.R#L40 You should write tidyr::gather_ instead of just gather. Otherwise, you will have an error if the user did not charge tidyr by himself.

Similarly, the calls to geom_arc_bar should also be prefixed by ggforce.

Finally, I had an issue with that line

https://github.com/GuangchuangYu/scatterpie/blob/9c8d2b262d16bd5f3352a5fd7e2c16229cf01611/R/geom_scatterpie.R#L31

xvar was remaining empty.

But, that alternative code was working:

xvar <- as.character(mapping$x)[2]

Best regards

statnmap commented 6 years ago

Yes. Thank you for this package.
I had the same issues here. I think linked to modifications in {rlang} and {tidyr}.

gather_ is now deprecated so that you need to use more tidyeval with gather. I propose:

  cols2 <- enquo(cols)
  df <- tidyr::gather(data, "type", "value", !!cols2)

Concerning xvar, I agree. I would do the same proposition:

xvar <- as.character(mapping$x)[2]
GuangchuangYu commented 6 years ago

for changing from xvar <- as.character(mapping)['x'] to xvar <- as.character(mapping$x)[2], do you have any reproducible example?

> require(ggplot2)
Loading required package: ggplot2
> as.character(aes(x=abc))['x']
    x
"abc"
> as.character(aes(x=abc)$x)
[1] "abc"
> as.character(aes(x=abc)$x)[2]
[1] NA

thanks @statnmap for the rlang part.

statnmap commented 6 years ago

I am using the dev version of {ggplot2}, this maybe a reason. Here are my outputs:

> as.character(aes(x=abc))['x']
[1] NA
> as.character(aes(x=abc)$x)
[1] "~"   "abc"
> as.character(aes(x=abc)$x)[2]
[1] "abc"
> 

To allow your function to work for both versions, you can add a test on xvar like:

xvar <- as.character(aes(x=abc))['x']
if (is.na(xvar)) {
xvar <- as.character(mapping$x)[2]
}
GuangchuangYu commented 6 years ago

thanks @statnmap and great to know the change.