Using my GitHub Action to observe the performance regression introduced by PR #4491 and fixed by PR #5463

NEWS.md

@@ -6,10 +6,21 @@
 
 ## NEW FEATURES
 
+1. `nafill()` now applies `fill=` to the front/back of the vector when `type="locf|nocb"`, [#3594](https://github.com/Rdatatable/data.table/issues/3594). Thanks to @ben519 for the feature request. It also now returns a named object based on the input names. Note that if you are considering joining and then using `nafill(...,type='locf|nocb')` afterwards, please review `roll=`/`rollends=` which should achieve the same result in one step more efficiently. `nafill()` is for when filling-while-joining (i.e. `roll=`/`rollends=`/`nomatch=`) cannot be applied.
+
 ## BUG FIXES
 
 ## NOTES
 
+1. New feature 29 in v1.12.4 (Oct 2019) introduced zero-copy coercion. Our thinking is that requiring you to get the type right in the case of `0` (type double) vs `0L` (type integer) is too inconvenient for you the user. So such coercions happen in `data.table` automatically without warning. Thanks to zero-copy coercion there is no speed penalty, even when calling `set()` many times in a loop, so there's no speed penalty to warn you about either. However, we believe that assigning a character value such as `"2"` into an integer column is more likely to be a user mistake that you would like to be warned about. The type difference (character vs integer) may be the only clue that you have selected the wrong column, or typed the wrong variable to be assigned to that column. For this reason we view character to numeric-like coercion differently and will warn about it. If it is correct, then the warning is intended to nudge you to wrap the RHS with `as.<type>()` so that it is clear to readers of your code that a coercion from character to that type is intended. For example :
+
+    ```R
+    x = c(2L,NA,4L,5L)
+    nafill(x, fill=3)                 # no warning; requiring 3L too inconvenient
+    nafill(x, fill="3")               # warns in case either x or "3" was a mistake
+    nafill(x, fill=3.14)              # warns that precision has been lost
+    nafill(x, fill=as.integer(3.14))  # no warning; the as.<type> conveys intent
+    ```
 
 # data.table [v1.14.0](https://github.com/Rdatatable/data.table/milestone/23?closed=1)  (submitted to CRAN on 20 Feb 2021)
 

R/data.table.R

@@ -141,7 +141,8 @@ replace_dot_alias = function(e) {
     return(ans)
   }
   if (!missing(verbose)) {
-    stopifnot(isTRUEorFALSE(verbose))
+    if (!is.integer(verbose) && !is.logical(verbose)) stop("verbose must be logical or integer")
+    if (length(verbose)!=1 || anyNA(verbose)) stop("verbose must be length 1 non-NA")
     # set the global verbose option because that is fetched from C code without having to pass it through
     oldverbose = options(datatable.verbose=verbose)
     on.exit(options(oldverbose))

R/shift.R

@@ -26,14 +26,10 @@ shift = function(x, n=1L, fill=NA, type=c("lag", "lead", "shift"), give.names=FA
 
 nafill = function(x, type=c("const","locf","nocb"), fill=NA, nan=NA) {
   type = match.arg(type)
-  if (type!="const" && !missing(fill))
-    warning("argument 'fill' ignored, only make sense for type='const'")
   .Call(CnafillR, x, type, fill, nan_is_na(nan), FALSE, NULL)
 }
 
 setnafill = function(x, type=c("const","locf","nocb"), fill=NA, nan=NA, cols=seq_along(x)) {
   type = match.arg(type)
-  if (type!="const" && !missing(fill))
-    warning("argument 'fill' ignored, only make sense for type='const'")
   invisible(.Call(CnafillR, x, type, fill, nan_is_na(nan), TRUE, cols))
 }

R/wrappers.R

@@ -9,6 +9,7 @@ fifelse = function(test, yes, no, na=NA) .Call(CfifelseR, test, yes, no, na)
 fcase   = function(..., default=NA) .Call(CfcaseR, default, parent.frame(), as.list(substitute(list(...)))[-1L])
 
 colnamesInt = function(x, cols, check_dups=FALSE) .Call(CcolnamesInt, x, cols, check_dups)
-coerceFill = function(x) .Call(CcoerceFillR, x)
 
 testMsg = function(status=0L, nx=2L, nk=2L) .Call(CtestMsgR, as.integer(status)[1L], as.integer(nx)[1L], as.integer(nk)[1L])
+
+coerceAs = function(x, as, copy=TRUE) .Call(CcoerceAs, x, as, copy)

inst/atime/tests.R

@@ -0,0 +1,68 @@
+pkg.edit.fun = quote(function(old.Package, new.Package, sha, new.pkg.path) {
+      pkg_find_replace <- function(glob, FIND, REPLACE) {
+        atime::glob_find_replace(file.path(new.pkg.path, glob), FIND, REPLACE)
+      }
+      Package_regex <- gsub(".", "_?", old.Package, fixed = TRUE)
+      Package_ <- gsub(".", "_", old.Package, fixed = TRUE)
+      new.Package_ <- paste0(Package_, "_", sha)
+      pkg_find_replace(
+        "DESCRIPTION", 
+        paste0("Package:\\s+", old.Package),
+        paste("Package:", new.Package))
+      pkg_find_replace(
+        file.path("src", "Makevars.*in"),
+        Package_regex,
+        new.Package_)
+      pkg_find_replace(
+        file.path("R", "onLoad.R"),
+        Package_regex,
+        new.Package_)
+      pkg_find_replace(
+        file.path("R", "onLoad.R"),
+        sprintf('packageVersion\\("%s"\\)', old.Package),
+        sprintf('packageVersion\\("%s"\\)', new.Package))
+      pkg_find_replace(
+        file.path("src", "init.c"),
+        paste0("R_init_", Package_regex),
+        paste0("R_init_", gsub("[.]", "_", new.Package_)))
+      pkg_find_replace(
+        "NAMESPACE",
+        sprintf('useDynLib\\("?%s"?', Package_regex),
+        paste0('useDynLib(', new.Package_))
+    })
+
+test.list <- list(
+  # Performance regression fixed in: https://github.com/Rdatatable/data.table/pull/4440
+  "Test regression fixed in #4440" = list(
+    pkg.edit.fun = pkg.edit.fun,
+    N = 10^seq(3,8),
+    setup = quote({
+      set.seed(1L)
+      dt <- data.table(a = sample(N, N))
+      setindex(dt, a)
+    }),
+    expr = quote(data.table:::shallow(dt)),
+    "Before"="9d3b9202fddb980345025a4f6ac451ed26a423be",
+    "Regression"="752012f577f8e268bb6d0084ca39a09fa7fbc1c4", 
+    "Fixed"="9d3b9202fddb980345025a4f6ac451ed26a423be"),
+
+  # Test based on: https://github.com/Rdatatable/data.table/issues/5424
+  # Performance regression introduced in: https://github.com/Rdatatable/data.table/pull/4491
+  # Fixed in: https://github.com/Rdatatable/data.table/pull/5463    
+  "Test regression fixed in #5463" = list(
+    pkg.edit.fun = pkg.edit.fun,
+    N = 10^seq(3, 8),
+    expr = quote(data.table:::`[.data.table`(dt_mod, , N := .N, by = g)),
+    setup = quote({
+      n <- N/100
+      set.seed(1L)
+      dt <- data.table(
+        g = sample(seq_len(n), N, TRUE),
+        x = runif(N),
+        key = "g")
+      dt_mod <- copy(dt)
+    }),
+    "Before"="be2f72e6f5c90622fe72e1c315ca05769a9dc854",
+    "Regression"="e793f53466d99f86e70fc2611b708ae8c601a451",
+    "Fixed"="58409197426ced4714af842650b0cc3b9e2cb842")      
+)

inst/tests/froll.Rraw

@@ -78,15 +78,15 @@ test(6000.011, frollmean(x, n, adaptive=TRUE), list(c(NA, 1, 1.25), c(NA, 1, 1.2
 
 #### error on unsupported type
 dx = data.table(real=1:10/2, char=letters[1:10])
-test(6000.012, frollmean(dx, 3), error="x must be list, data.frame or data.table of numeric or logical types")
+test(6000.012, frollmean(dx, 3), error="x must be of type numeric or logical, or a list, data.frame or data.table of such")
 dx = data.table(real=1:10/2, fact=factor(letters[1:10]))
-test(6000.013, frollmean(dx, 3), error="x must be list, data.frame or data.table of numeric or logical types")
+test(6000.013, frollmean(dx, 3), error="x must be of type numeric or logical, or a list, data.frame or data.table of such")
 #dx = data.table(real=1:10/2, logi=logical(10))
 #test(6000.014, frollmean(dx, 3), error="x must be list, data.frame or data.table of numeric types") # commented out as support added in #3749, tested in .009
 dx = data.table(real=1:10/2, list=rep(list(NA), 10))
-test(6000.015, frollmean(dx, 3), error="x must be list, data.frame or data.table of numeric or logical types")
+test(6000.015, frollmean(dx, 3), error="x must be of type numeric or logical, or a list, data.frame or data.table of such")
 x = letters[1:10]
-test(6000.016, frollmean(x, 3), error="x must be of type numeric or logical")
+test(6000.016, frollmean(x, 3), error="x must be of type numeric or logical, or a list, data.frame or data.table of such")
 x = 1:10/2
 test(6000.017, frollmean(x, "a"), error="n must be integer")
 test(6000.018, frollmean(x, factor("a")), error="n must be integer")
@@ -355,8 +355,8 @@ test(6000.074, frollmean(1:3, 2, fill=0L), c(0, 1.5, 2.5))
 test(6000.075, frollmean(1:3, 2, fill=NA_integer_), c(NA_real_, 1.5, 2.5))
 test(6000.076, frollmean(1:3, 2, fill=1:2), error="fill must be a vector of length 1")
 test(6000.077, frollmean(1:3, 2, fill=NA), c(NA_real_, 1.5, 2.5))
-test(6000.078, frollmean(1:3, 2, fill=TRUE), error="fill must be numeric")
-test(6000.079, frollmean(1:3, 2, fill=FALSE), error="fill must be numeric")
+test(6000.078, frollmean(1:3, 2, fill=TRUE), frollmean(1:3, 2, fill=1)) #error="fill must be numeric") # fill already coerced, as 'x' arg
+test(6000.079, frollmean(1:3, 2, fill=FALSE), frollmean(1:3, 2, fill=0)) #error="fill must be numeric")
 test(6000.080, frollmean(1:3, 2, fill="a"), error="fill must be numeric")
 test(6000.081, frollmean(1:3, 2, fill=factor("a")), error="fill must be numeric")
 test(6000.082, frollmean(1:3, 2, fill=list(NA)), error="fill must be numeric")