Update indexing for dplyr_cummean (tidyverse#5287) (tidyverse#5288)

* Update indexing for dplyr_cummean (tidyverse#5287) * Add unit test for cummean Check that cummean is consistent with the alternate method of cummean(x) = cumsum(x) / seq_along(x). Make sure future changes don't break the relationship above. See also: tidyverse#5287 * Update unit test for cummean Add inline comments with exact values we expect to see, so in the future it is easier to compare expected vs actual results. * Update unit test for cummean Add another check that we get expected values, independent of cumsum and seq_along (in case of other bugs). That is, cummean(1:5) = [1, 1.5, 2, 2.5, 3]. * Not assuming there is at least one element, +test * + NEWS Co-authored-by: Romain Francois <[email protected]>
lucassxs · Jun 9, 2020 · 76f61d0 · 76f61d0
1 parent f089919
commit 76f61d0
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 2 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -1,5 +1,9 @@
 # dplyr (development version)
 
+## Minor improvements and bug fixes
+
+* cummean() no longer has off-by-one indexing problem (#5287).
+
 # dplyr 1.0.0
 
 ## Breaking changes

diff --git a/src/funs.cpp b/src/funs.cpp
@@ -105,8 +105,8 @@ SEXP dplyr_cummean(SEXP x) {
   double* p_out = REAL(out);
   double* p_x = REAL(x);
 
-  double sum = *p_out++ = *p_x;
-  for (R_xlen_t i = 1; i < n; i++, ++p_x, ++p_out) {
+  double sum = 0.0;
+  for (R_xlen_t i = 0; i < n; i++, ++p_x, ++p_out) {
     sum += *p_x;
     *p_out = sum / (i + 1.0);
   }

diff --git a/tests/testthat/test-window.R b/tests/testthat/test-window.R
@@ -87,6 +87,14 @@ test_that("cummean is not confused by FP error (#1387)", {
   expect_true(all(cummean(a) == a))
 })
 
+test_that("cummean is consistent with cumsum() and seq_along() (#5287)", {
+  x <- 1:5
+  expect_equal(cummean(x), c(1, 1.5, 2, 2.5, 3))
+  expect_equal(cummean(x), cumsum(x) / seq_along(x))
+
+  expect_equal(cummean(numeric()), numeric())
+})
+
 test_that("order_by() returns correct value", {
   expected <- int(15, 14, 12, 9, 5)
   expect_identical(order_by(5:1, cumsum(1:5)), expected)