wilkelab / ggridges

Ridgeline plots in ggplot2
https://wilkelab.org/ggridges
GNU General Public License v2.0
411 stars 31 forks source link

remove inf for calc_panel_params #21

Open wfondrie opened 6 years ago

wfondrie commented 6 years ago

The suggested changes fix problems I was having when handling infinite values with stat_density_ridges(). Here's a MWE:

library(ggplot2)
library(ggridges)
set.seed(1234)

# works
dat2 <- data.frame(x = c(rnorm(20)),
                   y = c(rep("a", 10), rep("b", 10)))

ggplot(dat2, aes(x = x, y = y)) + geom_density_ridges()

# doesn't work
dat <- data.frame(x = c(rnorm(19), Inf),
                  y = c(rep("a", 10), rep("b", 10)))

ggplot(dat, aes(x = x, y = y)) + geom_density_ridges()

# Error in if (!(lo <- min(hi, IQR(x)/1.34))) (lo <- hi) || (lo <- abs(x[1L])) ||  : 
# missing value where TRUE/FALSE needed

I think that the changes should minimally impact other functions. Also, this is my first pull request so let me know if I did anything wrong.

clauswilke commented 6 years ago

Thanks for the PR.

Just wondering: Does the regular stat_density() have issues with Inf? And if not, have you looked into how it addresses this problem?

codecov-io commented 6 years ago

Codecov Report

Merging #21 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #21   +/-   ##
=======================================
  Coverage   58.33%   58.33%           
=======================================
  Files          13       13           
  Lines         324      324           
=======================================
  Hits          189      189           
  Misses        135      135
Impacted Files Coverage Δ
R/stats.R 100% <ø> (ø) :arrow_up:
R/position.R 0% <0%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4e94e9a...656bd12. Read the comment docs.

wfondrie commented 6 years ago

I just looked through the ggplot2::stat_density() code. It looks like it uses the stats::density() function for density calculations, instead of calling bw.nrd0 directly. stats::density() has a line that filters for only finite values of x.

clauswilke commented 6 years ago

Then switching over to stats::density() sounds like the better approach.

clauswilke commented 6 years ago

Oh, please also add a little test to make sure that the stat can handle infinite values. It should be added here: https://github.com/clauswilke/ggridges/blob/master/tests/testthat/test_stat_density_ridges.R and be as minimal as possible, so it's fast.

wfondrie commented 6 years ago

Will do. I'll tackle it soon.