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

weights and range calculation #59

Closed
wants to merge 1 commit into from

Conversation

mkoohafkan
Copy link

This gets the job done, but I wonder if it would be more effective to turn geom_density_ridges into a geom_ridgeline() + ggplot2::stat_density() combo.

- replace `bw.nrd0` with `stats::density` and only use finite values for computing `from`/`to`
- add support for `weight` aesthetic
@clauswilke
Copy link
Collaborator

A couple of comments:

  1. Please don't change the bandwidth calculation if weight = NULL, and please don't call stats::density() just to calculate bandwidth. Actually, I think stats::density() doesn't consider weights for bandwidth calculation (see below), so you can just leave it as is.

  2. Please use tidyverse styling and indenting.

  3. I don't see the need to create a data$weigth column if the weight aesthetic isn't set. You can just write something like the following before the stats::density() call:

if (is.null(data$weight)) {
  weights <- NULL
} else {
  weights <- data$weight / sum(data$weight)
}
  1. All new features will have to be documented. Also consider adding a unit test for unweighted and weighted density calculations (neither currently exists).

  2. Currently a few unit tests are failing. I believe this is unrelated to the current PR and will have to be addressed separately.

Demonstration that density() doesn't use the weights for bandwidth calculations. This can be verified by looking at the source code.

x <- rnorm(100)
weights <- 1:100

density(x)$bw
#> [1] 0.3789782
density(x, weights = weights/sum(weights))$bw
#> [1] 0.3789782

Created on 2020-06-21 by the reprex package (v0.3.0)

@clauswilke
Copy link
Collaborator

This is now resolved. #90

@clauswilke clauswilke closed this Feb 7, 2024
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.

Weighted densities
2 participants