Skip to content

Expand ydensity range for nicer violin plots #1783

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

Merged
merged 5 commits into from
Sep 28, 2016

Conversation

thomasp85
Copy link
Member

Fixes #1700

@thomasp85
Copy link
Member Author

Continuing examples from #1700:

res <- data.frame(a = c(1:100),b=c(rep(0,50),rep(1,50)))
ggplot(res,aes(y=a,x=factor(b))) + geom_violin(trim=F)

rplot

ggplot(res,aes(y=a,x=factor(b))) + geom_violin(trim=T)

rplot01

One visual regression is in how zero-range data is displayed is this now remains as a "violin" rather than a single line:

dff <- data.frame(x = factor(rep(1, 10)), y = rep(0, 10))
ggplot(dff, aes(x, y)) + geom_violin()

rplot02

We can make a simple check for zero range condition and not expand in that case, but I'm not sure it is the right thing to do...

@thomasp85 thomasp85 self-assigned this Sep 27, 2016
@hadley
Copy link
Member

hadley commented Sep 27, 2016

I think that's the right behavior for zero range data

@thomasp85
Copy link
Member Author

The current or the proposed change (I personally prefer the current as it does not treat zero range differently)

@@ -69,7 +69,22 @@ StatYdensity <- ggproto("StatYdensity", Stat,
} else {
range <- scales$y$dimension()
}
dens <- compute_density(data$y, data$w, from = range[1], to = range[2],
if (is.character(bw)) {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind pulling this out into a separate function?

@hadley
Copy link
Member

hadley commented Sep 27, 2016

The plot that you included here seems like the correct behaviour to me.

@@ -69,6 +69,8 @@
* The `theme()` constructor now has named arguments rather than ellipsis. This
should make autocomplete substantially more useful.

* geom_violin now again has a nicer looking range that allow the density to
Copy link
Member

Choose a reason for hiding this comment

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

The range of each violin is now automatically extended 3 * bw for either end of the data range

@hadley
Copy link
Member

hadley commented Sep 27, 2016

LGTM

@thomasp85 thomasp85 merged commit 77343bd into tidyverse:master Sep 28, 2016
@wch
Copy link
Member

wch commented Sep 28, 2016

I don't think this is the right behavior for trim=T. It's supposed to trim to the range of the data, not until the tails have a near-zero width.

@hadley
Copy link
Member

hadley commented Sep 28, 2016

@wch you mean it should also set the bw multiplier to 0?

@wch
Copy link
Member

wch commented Sep 28, 2016

Yes, I think that's right. Here's what they looked like way back when they were first implemented:
https://github.com/wch/ggplot2/wiki/geom_violin

@thomasp85
Copy link
Member Author

Then there's really no need for a fix?

@wch
Copy link
Member

wch commented Sep 28, 2016

With trim=T, the violin for b=0 should go from 1 to 50, but right now it goes from about -15 to 70.

@wch
Copy link
Member

wch commented Sep 28, 2016

With 1.0.1, this is what it looked like.

res <- data.frame(a = c(1:100),b=c(rep(0,50),rep(1,50)))
ggplot(res,aes(y=a,x=factor(b))) + geom_violin(trim=T)

image

@wch
Copy link
Member

wch commented Sep 28, 2016

And with trim=F, it looked like this:

image

I don't see why it would be useful to have the tails running all the way to the top or bottom.

@lock
Copy link

lock bot commented Jan 18, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants