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

Should ggMarginal suppress warnings when type = boxplot or density #52

Closed crew102 closed 7 years ago

crew102 commented 7 years ago

Depending on the type of marginal plot, ggMarginal will pass along one of these warnings:

  1. type = histogram - i.e., ggMarginal(data = mtcars, x = "mpg", y = "wt", type = "histogram")
    `stat_bin()` using `bins = 30`.
    Pick better value with
    `binwidth`.
  2. type = boxplot - i.e., ggMarginal(data = mtcars, x = "mpg", y = "wt", type = "boxplot")
    Warning messages:
    1: Continuous x aesthetic -- did you forget aes(group=...)? 
    2: Continuous x aesthetic -- did you forget aes(group=...)? 
    3: Continuous x aesthetic -- did you forget aes(group=...)? 
  3. type = density - i.e., ggMarginal(data = mtcars, x = "mpg", y = "wt", type = "density")
    Warning: Ignoring unknown parameters: fill
    Warning: Ignoring unknown parameters: fill

In my opinion we should only pass along the warning in case number 1 (i.e., the binwidth warning when type = histogram). The other two warnings are not helpful and are the result of expected behavior. What do you think - should we suppress these warnings?

daattali commented 7 years ago

I agree that the boxplot message should not appear, good point, it should be suppressed. The density warning should not be happening at all though, is it possible that the refactor introduced it? I had these few lines added a while back that explicitly remove the 'fill' parameter when using a density plot to avoid this message https://github.com/daattali/ggExtra/blob/master/R/ggMarginal.R#L96-L99

If possible, it'd be good to re-introduce that logic so that the warning doesn't happen


Dean Attali President & CEO AttaliTech Ltd http://AttaliTech.com http://attalitech.com

On 2 May 2017 at 21:10, Chris Baker notifications@github.com wrote:

Depending on the type of marginal plot, ggMarginal will pass along one of these warnings:

  1. type = histogram - i.e., ggMarginal(data = mtcars, x = "mpg", y = "wt", type = "histogram")

stat_bin() using bins = 30. Pick better value with binwidth.

  1. type = boxplot - i.e., ggMarginal(data = mtcars, x = "mpg", y = "wt", type = "boxplot")

Warning messages: 1: Continuous x aesthetic -- did you forget aes(group=...)? 2: Continuous x aesthetic -- did you forget aes(group=...)? 3: Continuous x aesthetic -- did you forget aes(group=...)?

  1. type = density - i.e., ggMarginal(data = mtcars, x = "mpg", y = "wt", type = "density")

Warning: Ignoring unknown parameters: fill Warning: Ignoring unknown parameters: fill

In my opinion we should only pass along the warning in case number 1 (i.e., the binwidth warning when type = histogram). The other two warnings are not helpful and are the result of expected behavior. What do you think - should we suppress these warnings?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/daattali/ggExtra/issues/52, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFNW7bfEmT9qcMCirlwTdttQyo2Poks5r19P9gaJpZM4NO3aC .

crew102 commented 7 years ago

Ah yes, I removed extraParams[['fill']] <- NULL b/c I thought it was intended to prevent failure of the function when there was a non-null fill (which maybe it was), and since I wasn't seeing it fail in those cases, I removed it. So the warning message for density is new to the refactor. I will add that logic back.

daattali commented 7 years ago

Point 3 should be fixed with #40

Point 1 we agreed should remain

Point 2 I don't think was addressed in #40 ?

crew102 commented 7 years ago

I was going to address in separate PR

daattali commented 7 years ago

Sounds good!