Closed mkoohafkan closed 9 months ago
A couple of comments:
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.
Please use tidyverse styling and indenting.
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)
}
All new features will have to be documented. Also consider adding a unit test for unweighted and weighted density calculations (neither currently exists).
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)
This is now resolved. #90
bw.nrd0
withstats::density
and only use finite values for computingfrom
/to
(addresses issue and conversaation from pull #21weight
aesthetic, fixes #5This gets the job done, but I wonder if it would be more effective to turn
geom_density_ridges
into ageom_ridgeline() + ggplot2::stat_density()
combo.