RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
386 stars 32 forks source link

Explore group generics implementation #365

Open hadley opened 11 months ago

hadley commented 11 months ago

Fixes #353

hadley commented 9 months ago

To do:

hadley commented 8 months ago

@t-kalinowski @DavisVaughan before I finish up the implementation for the other group generics, could you please take a look at how it works for Math?

mmaechler commented 8 months ago

Sorry if I'm a "game spoiler": Using tryCatch() in such fundamental group's methods will I think be quite a performance killer e.g. for simple arithmetic / math operations. Is that really really necessary?

hadley commented 8 months ago

If you can suggest an alternative implementation, I'm happy to try it. But I couldn't think of another way to do it.

t-kalinowski commented 8 months ago

One approach might be to update method_ and method_call_ (c functions), so that, rather than than signaling an error directly, they return a sentinel value like NULL, or perhaps, the error call itself (i.e., calling eval() on the return object signals the error). Then, instead of calling S7_dispatch() in the S7_Math() generic, we could inspect for the sentinel instead. For example, S7_Math() could be defined like this:

S7_Math <<- new_generic("Math", "x", function(x, ..., .Generic) {
  cl <- .Call(method_call_, sys.call(), sys.function(), sys.frame())
  if(is.null(cl)) 
    NextMethod()
  else 
    eval(cl)
})
mmaechler commented 8 months ago

One approach might be to update method_ and method_call_ (c functions), so that, rather than than signaling an error directly, they return a sentinel value like NULL, or perhaps, the error call itself (i.e., calling eval() on the return object signals the error). Then, instead of calling S7_dispatch() in the S7_Math() generic, we could inspect for the sentinel instead.
For example ...

"Nice"! That would almost surely be an order of magnitude faster!

Possibly we'd even want to provide (and document and use ourselves) a thin wrapper around .Call(..) such that S7-users could make use of this as well in their own method definitions. {{I do acknowledge I haven't studied what you are doing exactly here, so "I have no clue" ;-)}}

hadley commented 8 months ago

Ok, I'll take a stab at that approach.