kuwisdelu / Cardinal

Mass spectrometry imaging toolbox
http://www.cardinalmsi.org
36 stars 14 forks source link

Bug with groups argument when calling image() within a user-defined function #15

Closed bentyeh closed 4 months ago

bentyeh commented 4 years ago

Consider the following code snippet. Cardinal::image() works fine if called from the global environment.

library(Cardinal)
set.seed(200)
mse <- Cardinal::simulateImage(preset = 1, npeaks = 10, dim = c(10, 10))

# this works
Cardinal::image(mse, groups = "circle")

However, calling Cardinal::image() within a user-defined function can lead to errors. Below are 2 examples.

Example 1

# wrapper function around Cardinal::image()
my_image <- function(mse, my_groups) {
  Cardinal::image(mse, groups = my_groups)
}

my_image(mse, my_groups = "circle")
#> Error in eval(expr, envir = p) : object 'my_groups' not found

The bug occurs in the following line:

https://github.com/kuwisdelu/Cardinal/blob/9a61aeb6b43bdb8d5f5dec3b55105be77befa1c7/R/image2-SparseImagingExperiment.R#L66-L67

Note that e was previously defined in the same function

https://github.com/kuwisdelu/Cardinal/blob/9a61aeb6b43bdb8d5f5dec3b55105be77befa1c7/R/image2-SparseImagingExperiment.R#L36

and .try_eval() is defined as

https://github.com/kuwisdelu/Cardinal/blob/9a61aeb6b43bdb8d5f5dec3b55105be77befa1c7/R/utils2.R#L71-L75

substitue(groups) is the name "my_groups". Since my_groups is not a column of pixelData(x) nor a name in the environment e, the tryCatch() statement in .try_eval() tries to find my_groups in p, which is the execution environment of the Cardinal::image() function. Since there is no variable named my_groups in p, we get an error.

For reference, here's the call stack printed by calling lobstr::cst() within .try_eval() just before the error is thrown:

  1. \-global::my_image(...)
  2.   +-Cardinal::image(...) ~/test.R
  3.   \-Cardinal::image(...)
  4.     \-Cardinal:::.local(x, ...)
  5.       +-methods::callNextMethod(...)
  6.       | \-base::eval(call, callEnv)
  7.       |   \-base::eval(call, callEnv)
  8.       \-Cardinal:::.nextMethod(...)
  9.         \-Cardinal:::.local(x, ...)
 10.           \-Cardinal:::.try_eval(...)
 11.             \-lobstr::cst()

Example 2

# slightly different wrapper function around Cardinal::image()
my_image <- function(mse, groups) {
  Cardinal::image(mse, groups = groups)
}

my_image(mse, groups = "circle")
#> Error in as.character(x) : cannot coerce type 'closure' to vector of type 'character'

This time, substitue(groups) is the name "groups". Although groups is not a column of pixelData(x), it is a name in the environment e. Specifically, with the debugger paused inside .try_eval(), printing groups gives

function (x) 
{
    UseMethod("groups")
}
<bytecode: 0x00000209231eab18>
<environment: namespace:dplyr>

So the first line below actually returns without error, with the groups variable now referencing the function dplyr::groups(). Consequently, an error is thrown when trying to convert groups to a factor.

https://github.com/kuwisdelu/Cardinal/blob/9a61aeb6b43bdb8d5f5dec3b55105be77befa1c7/R/image2-SparseImagingExperiment.R#L66-L70