Weighted moving average #54
Conversation
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 88.65% 88.68% +0.03%
==========================================
Files 80 80
Lines 1613 1618 +5
==========================================
+ Hits 1430 1435 +5
Misses 183 183
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for this great PR. I really appreciate it (and I am very happy that you already included unit tests).
In fact .movingAverage and .movingWeightedAverage just differ in the calculation of their weights:
1/length(windowSize) vs myweigths<-1/2**abs(-halfWindowSize:halfWindowSize); myweights/sum(myweights).
What do you think about changing .movingAverage into?:
.movingAverage <- function(y, halfWindowSize=2L, weighted=FALSE) {
.stopIfNotIsValidHalfWindowSize(halfWindowSize, n=length(y))
windowSize <- 2L * halfWindowSize + 1L
if (weighted) {
weights <- 1 / 2^abs(-halfWindowSize:halfWindowSize)
} else {
weights <- rep.int(1L, windowSize)
}
.filter(y, hws=halfWindowSize,
coef=matrix(weights/sum(weights), nrow=windowSize, ncol=windowSize, byrow=TRUE))
}Additional I just have some comments about the coding style (everyone has his own, sorry).
R/smoothingFilters-functions.R
Outdated
| ## | ||
| .movingWeightedAverage<-function(y, halfWindowSize=2L) { | ||
| windowSize <- 2L * halfWindowSize + 1L | ||
| myweigths<-1/2**abs(-halfWindowSize:halfWindowSize) |
There was a problem hiding this comment.
Could you please use ^ instead of ** (sorry but I never used ** in R before). Please put spaces around binary operators (/) and <- but not around = (three lines below). In general I try to follow the Bioconductor coding style (except I never used 4 spaces for indentation): https://bioconductor.org/developers/how-to/coding-style/
R/smoothingFilters-functions.R
Outdated
| myweigths<-1/2**abs(-halfWindowSize:halfWindowSize) | ||
| myweigths<-myweigths/sum(myweigths) | ||
| .filter(y, hws=halfWindowSize, | ||
| coef=matrix(myweigths, nrow=windowSize, ncol=windowSize, byrow = TRUE)) |
There was a problem hiding this comment.
Please use byrow=TRUE instead of byrow = TRUE.
|
No problem with the coding style. I shall try to be more careful in the future. I think your suggestion of incorporating the weights into the MovingAverage is a very good one. This also lessens the changes to be added to packages that rely on this function. I've made the changes, also to the unit tests and man pages. |
There was a problem hiding this comment.
Great! Thanks for applying these changes. The PR is in a pretty good shape. I just have a few minor comments left.
Please feel free to add an entry to the NEWS file (with version 1.17, I will bump it in the DESCRIPTION myself after merging this PR) and crediting yourself.
man/smoothIntensity-methods.Rd
Outdated
| @@ -26,7 +27,12 @@ This method smoothes the intensity values of a | |||
| selected \code{method}.} | |||
| \item{\dots}{arguments to be passed to \code{method}. \code{SavitzkyGolay} has | |||
| an additional \code{polynomialOrder} argument (default: \code{3}) to control | |||
| the order of the filter. Unused for \code{MovingAverage}} | |||
| the order of the filter. Unused for \code{MovingAverage}. | |||
There was a problem hiding this comment.
\dots isn't "unused" in MovingAverage anymore. This sentence could be removed.
There was a problem hiding this comment.
Sorry.. missed that one. Done.
R/smoothingFilters-functions.R
Outdated
| .filter(y, hws=halfWindowSize, | ||
| coef=matrix(1L / windowSize, nrow=windowSize, ncol=windowSize)) | ||
| coef=matrix(weights/sum(weights), nrow=windowSize, ncol=windowSize, byrow=TRUE)) |
There was a problem hiding this comment.
Could you please add spaces around / (I know I haven't done it myself in my last comment, sry).
There was a problem hiding this comment.
No problem. Done.
|
|
||
| values <- c(rep(4, 4), 5:6, rep(7, 4)) | ||
| expect_equal(MALDIquant:::.movingAverage(1:10, 3), values) | ||
| expect_equal(MALDIquant:::.movingAverage(1:10, 3, weighted=TRUE), values) |
There was a problem hiding this comment.
Could you please add an unit test where the default and the weighted version differ?
code tyding in the function itself and in the man page
|
Thanks a lot! I will run a few checks and tests and submit to CRAN soon. |
Preemptively creating the pull request for the weighted moving average. More explanation in #53