revdep AMAPVox affected: merge no longer accepts list of symbols

[tdhock]

The revdep AMAPVox uses data.table inside this code in example(plantAreaDensity):

library(AMAPVox)
vxsp <- readVoxelSpace(system.file("extdata", "tls_sample.vox", package = "AMAPVox"))
pad <- plantAreaDensity(vxsp, variable.name = "attenuation_PPL_MLE")
vxsp@data <- merge(vxsp@data, pad, on = list(i, j, k))

The merge is on two data tables. Starting from #5233 (git bisect says that is the first to be bad) it gives:

> vxsp@data <- merge(vxsp@data, pad, on = list(i, j, k))
Error in merge.data.table(vxsp@data, pad, on = list(i, j, k)) : 
  object 'i' not found
Calls: merge -> merge.data.table

A simpler example is below, which we could use for a new unit test, which should give the following old result (computed with 154fcf9, the parent of first bad commit)

> library(data.table)
data.table 1.14.3 IN DEVELOPMENT built 2022-11-22 22:14:32 UTC using 6 threads (see ?getDTthreads).  Latest news: r-datatable.com
> xDT <- data.table(a=1)
> iDT <- data.table(a=1,b=1:2)
> merge(xDT, iDT, on=list("a"))
Key: <a>
       a     b
   <num> <int>
1:     1     1
2:     1     2
> merge(xDT, iDT, on=list(a))
Key: <a>
       a     b
   <num> <int>
1:     1     1
2:     1     2

current master gives:

> library(data.table)
data.table 1.14.7 IN DEVELOPMENT built 2022-11-22 22:22:21 UTC using 6 threads (see ?getDTthreads).  Latest news: r-datatable.com
> xDT <- data.table(a=1)
> iDT <- data.table(a=1,b=1:2)
> merge(xDT, iDT, on=list("a"))
Key: <a>
       a     b
   <num> <int>
1:     1     1
2:     1     2
Warning message:
Unknown argument 'on' has been passed. 
> merge(xDT, iDT, on=list(a))
Error in merge.data.table(xDT, iDT, on = list(a)) : object 'a' not found

[ben-schwen]

Might sound a bit harsh, but the example is wrong. There is no on argument in merge (only by) and the passed argument just gets lost in ellipsis

[jangorecki]

Yes, we need to look into documentation to see if it is meant to be supported in the first place.

[tdhock]

Right, I saw that there was no on argument in the docs as well. I checked the inst/tests files and there are currently no tests which use on argument, so maybe this is a case where we should contact revdep maintainer and ask for an update? Also maybe we should add a test?

[ben-schwen]

I suppose the problem arises here because this is an example where merging with [ and merge itself diverges. To stay in line with base I wouldn't start supporting on together with merge.

[jangorecki]

Yes, agree on contacting maintainer of revdep and informing about misuse of merge.

[tdhock]

AMAPVox has published a new CRAN version that fixes this issue.