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

Marg map retry #66

Closed crew102 closed 6 years ago

crew102 commented 7 years ago

This PR implements the feature mentioned in #61. Two notes:

  1. I had to hard-code the size of the lines that cover up the outline created by geom_density here:

https://github.com/crew102/ggExtra/blob/0d9579743824b4a6d23655a43a4275ed4d3fb7b9/R/ggMarginalHelpers.R#L167-L172

The size of the covering line(s) has to be a little bit bigger than the size of the line used by geom_density to actually cover the line on some graphics devices (e.g., svg and pdf in some cases).

  1. I will update the docs after this this PR is merged.
crew102 commented 6 years ago

Oh yeah, that does seem to be it. Nice catch.

In the example you gave I'm able to see the density colors in the plot, but they don't show up in the legend. Dropping the alpha param results in the colors showing up in both.

d

daattali commented 6 years ago

I came up with even simpler examples and reported the bug https://github.com/tidyverse/ggplot2/issues/2452

ggplot(mtcars, aes(wt, colour = factor(vs))) + geom_line(stat = "density", alpha = 0.9) +
  theme(
    axis.title.x = element_blank(),
    axis.text.x = element_blank(),
    plot.margin = unit(c(0, 0, 0, 0), "null"),
    axis.ticks.length = unit(0, "null")
  )

Is all we needed. I'd want to wait a few days to see if anything happens with this bug from ggplot's or rstudio's end. Until then, do you think it would be ok to drop the alpha from the geom_line or would that have an effect?

crew102 commented 6 years ago

Yeah, I say we drop alpha param for density plot lines...I don't image it's used very often and there wouldn't be any other way to resolve this issue for users using previous versions of ggplot2 (assuming issue is fixed in next release if ggplot2).

On Feb 13, 2018 7:04 PM, "Dean Attali" notifications@github.com wrote:

I came up with even simpler examples and reported the bug tidyverse/ggplot2#2452 https://github.com/tidyverse/ggplot2/issues/2452

ggplot(mtcars, aes(wt, colour = factor(vs))) + geom_line(stat = "density", alpha = 0.9) + theme( axis.title.x = element_blank(), axis.text.x = element_blank(), plot.margin = unit(c(0, 0, 0, 0), "null"), axis.ticks.length = unit(0, "null") )

Is all we needed. I'd want to wait a few days to see if anything happens with this bug from ggplot's or rstudio's end. Until then, do you think it would be ok to drop the alpha from the geom_line or would that have an effect?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daattali/ggExtra/pull/66#issuecomment-365448482, or mute the thread https://github.com/notifications/unsubscribe-auth/AKs3OWB0mrL9J0ln2rpn9sKjLfyE5uQnks5tUiLygaJpZM4PlFVw .

daattali commented 6 years ago

Yup, sounds good

On Feb 13, 2018 7:36 PM, "Chris Baker" notifications@github.com wrote:

Yeah, I say we drop alpha param for density plot lines...I don't image it's used very often and there wouldn't be any other way to resolve this issue for users using previous versions of ggplot2 (assuming issue is fixed in next release if ggplot2).

On Feb 13, 2018 7:04 PM, "Dean Attali" notifications@github.com wrote:

I came up with even simpler examples and reported the bug tidyverse/ggplot2#2452 https://github.com/tidyverse/ggplot2/issues/2452

ggplot(mtcars, aes(wt, colour = factor(vs))) + geom_line(stat = "density", alpha = 0.9) + theme( axis.title.x = element_blank(), axis.text.x = element_blank(), plot.margin = unit(c(0, 0, 0, 0), "null"), axis.ticks.length = unit(0, "null") )

Is all we needed. I'd want to wait a few days to see if anything happens with this bug from ggplot's or rstudio's end. Until then, do you think it would be ok to drop the alpha from the geom_line or would that have an effect?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daattali/ggExtra/pull/66#issuecomment-365448482, or mute the thread https://github.com/notifications/unsubscribe-auth/ AKs3OWB0mrL9J0ln2rpn9sKjLfyE5uQnks5tUiLygaJpZM4PlFVw .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/daattali/ggExtra/pull/66#issuecomment-365454939, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFBSyDUcVy8GzKd7BdSSm9eWFOHiDks5tUiqpgaJpZM4PlFVw .

crew102 commented 6 years ago

OK, I think this should be good now

daattali commented 6 years ago

Geez, that rlang thing seems like hell too. You're exposing yourself to a lot of misery. Thanks for removing alpha, I'll test everything out again soon , this should be good now though :)

daattali commented 6 years ago

Done! Congrats @crew102 that was a tough one :)