Hi,
after updating R 2.5.0 devel yesterday we today observed many new
unexpected failures in our daily package build and test system runs,
which can be traced to recent changes in the implementation in try()
(as noted in NEWS).
Investigating this new implementation I come across an issue in
conjuntion with using S4 classes and methods. try(expr) does not return an
object with attribute 'try-error' in case of method dispatch failure
in the wrapped expression which to me seems not
quite correct.
Examples to reproduce the observation:
## using functions all is well:
f <- function(x) { print(x); ret<-try(stop("forced.")); print(ret)}
f(3)
[1] 3
Error in try(stop("forced.")) : forced.
[1] "Error in try(stop(\"forced.\")) : forced.\n"
attr(,"class")
[1] "try-error"
## using S4 classes and methods
setClass("fooBase",
representation("VIRTUAL",
width = "numeric",
height = "numeric"),
prototype(width = 1024,
height = 1024),
validity = NULL,
where = .GlobalEnv,
sealed = TRUE,
)
if (!isGeneric("plotObject")) {
setGeneric("plotObject",
def=function(x, y, ...) {
value <- standardGeneric("plotObject")
return(value)
},
where=.GlobalEnv,
useAsDefault=TRUE
)
}
setClass("foo",
representation("fooBase"),
validity = NULL,
where = .GlobalEnv,
sealed = TRUE)
plotObject.foo <- function(x, y) {
plot(x,y)
}
setMethod("plotObject", signature=c("foo", "numeric"), plotObject.foo,
where=.GlobalEnv)
fooObject <- new("foo")
## should fail and return object with attribute 'try-error' set
ret <- try(plotObject(fooObject, character(1)))
Error in as.list(call)[[1]] == "doTryCatch" :
comparison (1) is possible only for atomic and list types
is(ret)
Error in .class1(object) : object "ret" not found
which I belive is in contradiction to the documentation, where in
Details:
The value of the expression if 'expr' is evaluated without error,
but an invisible object of class '"try-error"' containing the
error message if it fails.
This is crucial for our current implementation of check functions in
package RUnit.
Is this new behaviour 'as intended' and only the documentation has not
caught up?
Regards,
Matthias
sessionInfo()
R version 2.5.0 Under development (unstable) (2007-03-13 r40832)
i686-pc-linux-gnu
locale:
C
attached base packages:
[1] "stats" "graphics" "grDevices" "datasets" "utils" "methods"
[7] "base"
other attached packages:
rcompletion rcompgen
"0.1-2" "0.1-5"
Matthias Burger Project Manager/ Biostatistician
Epigenomics AG Kleine Praesidentenstr. 1 10178 Berlin, Germany
phone:+49-30-24345-371 fax:+49-30-24345-555
http://www.epigenomics.com matthias.burger at epigenomics.com
--
Epigenomics AG Berlin Amtsgericht Charlottenburg HRB 75861
Vorstand: Geert Nygaard (CEO/Vorsitzender), Dr. Kurt Berlin (CSO)
Oliver Schacht PhD (CFO), Christian Piepenbrock (COO)
Aufsichtsrat: Prof. Dr. Dr. hc. Rolf Krebs (Chairman/Vorsitzender)
Investigating this new implementation I come across an issue in
conjuntion with using S4 classes and methods. try(expr) does not return an
object with attribute 'try-error' in case of method dispatch failure
in the wrapped expression which to me seems not
quite correct.
We've seen some similar issues but had not had time to track them
down.
Examples to reproduce the observation:
It isn't S4 specific. The issue seems more about anonymous
calls/functions.
ll = list()
ll[[1]] = function(x) stop("died")
v = try(do.call(ll[[1]], list(1)), silent=TRUE)
Error in as.list(call)[[1]] == "doTryCatch" :
comparison (1) is possible only for atomic and list types
> v
Error: object "v" not found
I don't fully understand why the call is being computed, but the
following seems to get things going.
+ seth
--- a/src/library/base/R/New-Internal.R
+++ b/src/library/base/R/New-Internal.R
@@ -7,7 +7,8 @@ try <- function(expr, silent = FALSE) {
## Patch up the call to produce nicer result for testing as
## try(stop(...)). This will need adjusting if the
## implementation of tryCatch changes.
- if (as.list(call)[[1]] == "doTryCatch")
+ callFun <- as.list(call)[[1]]
+ if (is.name(callFun) && callFun == "doTryCatch")
call <- sys.call(-4)
dcall <- deparse(call)[1]
prefix <- paste("Error in", dcall, ": ")
Seth Falcon | Computational Biology | Fred Hutchinson Cancer Research Center
http://bioconductor.org
Investigating this new implementation I come across an issue in
conjuntion with using S4 classes and methods. try(expr) does not return an
object with attribute 'try-error' in case of method dispatch failure
in the wrapped expression which to me seems not
quite correct.
We've seen some similar issues but had not had time to track them
down.
Examples to reproduce the observation:
It isn't S4 specific. The issue seems more about anonymous
calls/functions.
ll = list()
ll[[1]] = function(x) stop("died")
v = try(do.call(ll[[1]], list(1)), silent=TRUE)
Error in as.list(call)[[1]] == "doTryCatch" :
comparison (1) is possible only for atomic and list types
> v
Error: object "v" not found
I don't fully understand why the call is being computed, but the
following seems to get things going.
+ seth
--- a/src/library/base/R/New-Internal.R
+++ b/src/library/base/R/New-Internal.R
@@ -7,7 +7,8 @@ try <- function(expr, silent = FALSE) {
## Patch up the call to produce nicer result for testing as
## try(stop(...)). This will need adjusting if the
## implementation of tryCatch changes.
- if (as.list(call)[[1]] == "doTryCatch")
+ callFun <- as.list(call)[[1]]
+ if (is.name(callFun) && callFun == "doTryCatch")
call <- sys.call(-4)
dcall <- deparse(call)[1]
prefix <- paste("Error in", dcall, ": ")
Good catch, Seth. The code still looks a bit clunky though, and I wonder
if this wouldn't be nicer:
if (identical(call[[1]], quote(doTryCatch))) call <- sys.call(-4)
I.e., the thing that is clearly wrong is the use of "==" on something
that is not necessarily a simple name, but the use of as.list seems
unnecessary and comparisons between strings and names is a bit awkward too.
"PD" == Peter Dalgaard <p.dalgaard at biostat.ku.dk>
on Fri, 16 Mar 2007 08:09:00 +0100 writes:
PD> Seth Falcon wrote:
>> ml-it-r-devel at epigenomics.com writes:
>>
>>> Investigating this new implementation I come across an issue in
>>> conjuntion with using S4 classes and methods. try(expr) does not return an
>>> object with attribute 'try-error' in case of method dispatch failure
>>> in the wrapped expression which to me seems not
>>> quite correct.
>>>
>>
>> We've seen some similar issues but had not had time to track them
>> down.
>>
>>
>>> Examples to reproduce the observation:
>>>
>>
>> It isn't S4 specific. The issue seems more about anonymous
>> calls/functions.
>>
>> ll = list()
>> ll[[1]] = function(x) stop("died")
>>
>> v = try(do.call(ll[[1]], list(1)), silent=TRUE)
>> Error in as.list(call)[[1]] == "doTryCatch" :
>> comparison (1) is possible only for atomic and list types
>> > v
>> Error: object "v" not found
>>
>> I don't fully understand why the call is being computed, but the
>> following seems to get things going.
>>
>> + seth
>>
>> --- a/src/library/base/R/New-Internal.R
>> +++ b/src/library/base/R/New-Internal.R
>> @@ -7,7 +7,8 @@ try <- function(expr, silent = FALSE) {
>> ## Patch up the call to produce nicer result for testing as
>> ## try(stop(...)). This will need adjusting if the
>> ## implementation of tryCatch changes.
>> - if (as.list(call)[[1]] == "doTryCatch")
>> + callFun <- as.list(call)[[1]]
>> + if (is.name(callFun) && callFun == "doTryCatch")
>> call <- sys.call(-4)
>> dcall <- deparse(call)[1]
>> prefix <- paste("Error in", dcall, ": ")
>>
>>
PD> Good catch, Seth.
yes, indeed!
PD> The code still looks a bit clunky though, and I wonder
PD> if this wouldn't be nicer:
PD> if (identical(call[[1]], quote(doTryCatch))) call <- sys.call(-4)
PD> I.e., the thing that is clearly wrong is the use of "==" on something
PD> that is not necessarily a simple name, but the use of as.list seems
PD> unnecessary and comparisons between strings and names is a bit awkward too.
Indeed, I had similar thoughts when reading the part of code
Seth was patching
{ but would have used the old-fashioned as.name("doTryCatch")
instead of the modern quote(doTryCatch)
for the only reason that I'm probably slightly ``older fashioned''
than Peter ;-)
}
Martin
This is off-topic, but since the discussion moved towards coding
style... Here are some comments on S4 style.
ml-it-r-devel at epigenomics.com writes:
## using S4 classes and methods
setClass("fooBase",
representation("VIRTUAL",
width = "numeric",
height = "numeric"),
prototype(width = 1024,
height = 1024),
validity = NULL,
where = .GlobalEnv,
sealed = TRUE,
)
I think for package code, you don't want to specify the where to be
.GlobalEnv. If you omit the where argument, the class will be defined
within the package environment which is what one usually wants.
if (!isGeneric("plotObject")) {
setGeneric("plotObject",
def=function(x, y, ...) {
value <- standardGeneric("plotObject")
return(value)
},
where=.GlobalEnv,
useAsDefault=TRUE
)
}
This idiom of conditionally defining an S4 generic is wide-spread and
I suspect was required at some point in time. However, at this point,
I don't understand why one would do this and it seems that it can only
lead to hard to catch bugs. I think it should be strongly discouraged.
To define a method on a generic, you need to know what that generic
is. For example, you need to know the names of the formal arguments.
With conditional definition as above, you risk attempting to define a
method on a generic you know nothing about.
If you want your own generic, define it. If you want to set a method
on someone else's generic, say so. For example, you can do:
setMethod(otherPkg::theirGeneric, ...)
This code is a bit confusing to read since an S3 method for class
"foo" and S3 generic plotObject would be plotObject.foo. Maybe not
worth worrying about.
Finally, a further subtle point about how the generic was defined in
your example code. Especially for a standardGeneric, it is best not
to name the result before returning as this can affect when results
get copied. Here's an illustration:
setGeneric("frob1", function(x) {
value <- standardGeneric("frob1")
value
})
setGeneric("frob2", function(x) {
standardGeneric("frob2")
})
setMethod("frob1", "integer",
function(x) vector(mode="integer", length=x))
setMethod("frob2", "integer",
function(x) vector(mode="integer", length=x))
###
x1 <- frob1(5L)
> tracemem(x1)
[1] "<0x3de8098>"
> x1[1L] <- 5L
tracemem[0x3de8098 -> 0x3de80d0]:
>
> x2 <- frob2(5L)
> tracemem(x2)
[1] "<0x3de8140>"
> x2[1L] <- 5L
Best Wishes,
+ seth
Seth Falcon | Computational Biology | Fred Hutchinson Cancer Research Center
http://bioconductor.org
It isn't S4 specific. The issue seems more about anonymous
calls/functions.
ll = list()
ll[[1]] = function(x) stop("died")
v = try(do.call(ll[[1]], list(1)), silent=TRUE)
Error in as.list(call)[[1]] == "doTryCatch" :
comparison (1) is possible only for atomic and list types
> v
Error: object "v" not found
I don't fully understand why the call is being computed, but the
following seems to get things going.
+ seth
I applied your patch and the issues seems to be resolved. Now I just wait
to see if all test case failures related to this disappear.
Thank you for your kind help!
Matthias
Matthias Burger Project Manager/ Biostatistician
Epigenomics AG Kleine Praesidentenstr. 1 10178 Berlin, Germany
phone:+49-30-24345-371 fax:+49-30-24345-555
http://www.epigenomics.com matthias.burger at epigenomics.com
--
Epigenomics AG Berlin Amtsgericht Charlottenburg HRB 75861
Vorstand: Geert Nygaard (CEO/Vorsitzender), Dr. Kurt Berlin (CSO)
Oliver Schacht PhD (CFO), Christian Piepenbrock (COO)
Aufsichtsrat: Prof. Dr. Dr. hc. Rolf Krebs (Chairman/Vorsitzender)
I applied your patch and the issues seems to be resolved. Now I just wait
to see if all test case failures related to this disappear.
Thank you for your kind help!
Matthias
I have just committed my variation of Seth's patch, so please check the
current r-devel too.
Matthias Burger Project Manager/ Biostatistician
Epigenomics AG Kleine Praesidentenstr. 1 10178 Berlin, Germany
phone:+49-30-24345-371 fax:+49-30-24345-555
http://www.epigenomics.com matthias.burger at epigenomics.com
--
Epigenomics AG Berlin Amtsgericht Charlottenburg HRB 75861
Vorstand: Geert Nygaard (CEO/Vorsitzender), Dr. Kurt Berlin (CSO)
Oliver Schacht PhD (CFO), Christian Piepenbrock (COO)
Aufsichtsrat: Prof. Dr. Dr. hc. Rolf Krebs (Chairman/Vorsitzender)