R devel - Mar 2007 - R 2.5.0 devel try issue in conjuntion with S4 method dispatch

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)

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, ": ")



-- 
Seth Falcon | Computational Biology | Fred Hutchinson Cancer Research Center
http://bioconductor.org

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, ...)
> plotObject.foo <- function(x, y) {
>   plot(x,y)
> }
>
> setMethod("plotObject", signature=c("foo",
"numeric"), plotObject.foo,
> where=.GlobalEnv)
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

Hi Seth,
>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)

"Matthias Burger" <matthias.burger at epigenomics.com>
writes:
> Well, I understand I should have spent more time trimming my example
> to the minimal required code. Again this piece was copied & reduced.
No need to have done anything differently.  I commented on the code
in hopes that it might be helpful for others learning S4 stuff -- so I
think the discussion is useful even if it isn't your real code :-)
> The actual package uses a namespace which only exports the method but not
> the function.
> Using the .className extension scheme I can still get at the actual
> code at the R prompt via ':::'  which I find just handy.
If you switched to _className, then there would be no S3 confusion.
There are performance implications in doing things this way, but I
agree it is convenient.

Cheers,

+ seth

-- 
Seth Falcon | Computational Biology | Fred Hutchinson Cancer Research Center
http://bioconductor.org