Skip to content
New issue

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

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

Already on GitHub? # to your account

remove inf for calc_panel_params #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wfondrie
Copy link

@wfondrie wfondrie commented May 3, 2018

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
Copy link
Collaborator

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
Copy link

codecov-io commented May 3, 2018

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% <ø> (ø) ⬆️
R/position.R 0% <0%> (ø) ⬆️

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
Copy link
Author

wfondrie commented May 3, 2018

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
Copy link
Collaborator

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

@clauswilke
Copy link
Collaborator

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
Copy link
Author

wfondrie commented May 3, 2018

Will do. I'll tackle it soon.

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

Successfully merging this pull request may close these issues.

3 participants