search

daattali / ggExtra

📊 Add marginal histograms to ggplot2, and more ggplot2 enhancements
http://daattali.com/shiny/ggExtra-ggMarginal-demo/
Other
383 stars 45 forks source link

bug in v0.8: ggMarginal with geom_line #109

Closed giuseppec closed 6 years ago

giuseppec commented 6 years ago

I think you introduced a bug:

p = ggplot(mtcars, aes_string(x = "wt", y = "mpg")) + geom_line() + geom_point() 
ggMarginal(p)
# Error in `[.data.frame`(scatDF, , c("var", "fill", "colour", "group")) : 
#   undefined columns selected

It used to work in v0.7. Also, switching geom_point() with geom_line() will make it work again. However, this still seems to be a bug that should be fixed for the next version. I guess people already use ggExtra without being aware that the order of geom_point and geom_line matters. Would be great if you could fix this soon.

traceback()
7: stop("undefined columns selected")
6: `[.data.frame`(scatDF, , c("var", "fill", "colour", "group"))
5: scatDF[, c("var", "fill", "colour", "group")]
4: getVarDf(marg = marg, scatPbuilt = scatPbuilt)
3: genRawMargPlot(marg, type = type, scatPbuilt = scatPbuilt, prmL = prmL, 
       groupColour = groupColour, groupFill = groupFill)
2: genFinalMargPlot(marg = "x", type = type, scatPbuilt = scatPbuilt, 
       prmL = prmL, groupColour = groupColour, groupFill = groupFill)
1: ggMarginal(p)
giuseppec commented 6 years ago

I've looked at scatDF and basically the fill column was missing. Maybe you could add some more unit tests for several preprocessing stuff to avoid such (critical) bugs in the future?

daattali commented 6 years ago

Thanks for the report - I agree that's a bad bug to introduce! We do make every attempt possible to not introduce any regressions. Every time a bug is found, a unit test is added to ensure similar things won't happen. Will let you know when this is addressed.

daattali commented 6 years ago

Looks like this is happening because these lines:

https://github.com/daattali/ggExtra/blob/e97e2c01a7b132604e9c23fc1dbdf5a0f05769e4/R/ggMarginal-helpers-genFinalMargPlot.R#L77-L87

don't correctly pick out the geom_point layer. It only looks to see if there is both an x and a y. In the code provided by OP, the geom_line() does have x and y, so the code incorrectly treats it as the geom_point just because it's the first layer that has an x and a y.

@crew102

daattali commented 6 years ago

Perhaps changing the vapply to scatPbuilt$plot$layers, function(x) class(x$geom)[1] == "GeomPoint" would solve it

crew102 commented 6 years ago

yeah that's the chunk responsible for the problem. I put together a similar fix last night but found some problems with our tests, so I need to update the test figs/code before opening a pr for this issue.

daattali commented 6 years ago

this issue can be fixed now

Victor-GY-Yu commented 6 years ago

I got this error too - my plot (which is in a very complex function), reducing the code to isolate the problem:

df %>% ggplot(mapping = aes(x, y)) +
    stat_function(fun = function(x) any_function_here) +
    geom_point()

Swapping the stat_function and geom_point calls around fixed it.

crew102 commented 6 years ago

This issue is fixed in the development version of ggExtra (see 97e5036dab8b0b19a9291bfbf37b09272f7fba2b). Try installing it with devtools::install_github("daattali/ggExtra").

library(ggplot2)
library(ggExtra) # dev version

p <- ggplot(mtcars, aes(wt, mpg)) +
  stat_function(fun = exp) +
  geom_point()

ggMarginal(p)

Created on 2018-11-30 by the reprex package (v0.2.0.9000).