Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set outline type for geom_density as geom_area #3708

Merged
merged 2 commits into from Jan 8, 2020

Conversation

thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Jan 8, 2020

The new stroking of geom_ribbon() and geom_area() didn't make it to geom_density() This PR adds it and make it behave like geom_area()

@thomasp85 thomasp85 added this to the ggplot2 3.3.0 milestone Jan 8, 2020
@thomasp85 thomasp85 requested a review from yutannihilation Jan 8, 2020
Copy link
Member

@yutannihilation yutannihilation left a comment

Looks good, but..., I'm wondering if we can safely make this change.

Different from the case of GeomRibbon, I guess there are already many usages of full-stroking outlines on GeomDensity. That's why I didn't include GeomDensity in #3529.

Probably I'm just too timid because changing the outline of GeomDensity is one of my trauma around ggplot2. Let me think a bit more after drinking some coffee...

R/geom-density.r Outdated
@@ -56,7 +57,9 @@ geom_density <- function(mapping = NULL, data = NULL,
na.rm = FALSE,
orientation = NA,
show.legend = NA,
inherit.aes = TRUE) {
inherit.aes = TRUE,
outline.type = 'upper') {
Copy link
Member

@yutannihilation yutannihilation Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
outline.type = 'upper') {
outline.type = "upper") {

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Jan 8, 2020

You need to add geom_density here to avoid argument checks:

9c21ed7

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 8, 2020

You are already breaking peoples code as geom_density now uses "both" (the default for geom_ribbon()). This will just break the code in a more. beautiful way... 🙂

@hadley do you have any objections to this visual change?

@thomasp85 thomasp85 merged commit 89120e8 into tidyverse:v3.3.0-rc Jan 8, 2020
2 checks passed
@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Jan 8, 2020

Hmm, you are right. Hope this is OK this time...

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Jan 8, 2020

@thomasp85 @hadley
Now I feel this is not OK after looking at the examples on geom_density(), a geom with no fill by default. Note that these figures are drawn against master and doesn't reflect this pull request, so it's just my fault in #3529. Sorry I didn't aware of this visual breakage.

I want to propose renaming option "legacy" to "full" and making it default for geom_density() to restore the previous full-stroking outline. What do you think?

Screenshot from 2020-01-09 08-42-24

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 9, 2020

I am honestly against. The full stroking of density plots has always looked ugly to my eye. I’m for the renaming, though

The sides and lower bound in a density plot is not really data, and stroking them just creates visual noise IMO

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Jan 9, 2020

The sides and lower bound in a density plot is not really data, and stroking them just creates visual noise IMO

Thanks, agreed. After some investigation, I now agree that drawing the upper lines only is the right thing as base R does so.

plot(density(diamonds$carat, adjust = 5))

What makes me uneasy with upper-only geom_density() is that the line doesn't start from 0 by default, but it seems because geom_density() provides a narrower plot range than plot(density()) (#3387 might be related?). So, you are definitely right in that the side strokes are "visual noise" that have hided this problem. I feel the right one below should be drawn by default. I'll file a new issue for this.

p <- ggplot(diamonds, aes(carat)) +
  geom_density(adjust = 5)

patchwork::wrap_plots(
  p + ggtitle("(default)"),
  p + ggtitle("(expanded)") + expand_limits(x = -0.5)
)

I’m for the renaming, though

Sure.

@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Jan 9, 2020

I agree that the rightmost is more pleasing, but I think that ggplot2 has historically erred to the side of keeping the data-range. This is also done by default in violin plots. Extrapolating is something you have to do wilfully, as it creates data from thin air

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Jan 9, 2020

Sounds fair, but let me dig into the history...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants