r-lib / R6

Encapsulated object-oriented programming for R
https://R6.r-lib.org
Other
405 stars 56 forks source link

Recent update to 2.1.3 breaks overloading of [[ #96

Closed hhoeflin closed 7 years ago

hhoeflin commented 7 years ago

Hi,

I am using R6 methods and for some of them I am overloading the [[ operator to do something specific. The recent changes to the clone method with

for (name in names(public_copies)) {
    if (is.function(public_bind_env[[name]]))
      lockBinding(name, public_bind_env)
  }

breaks this functionality as the public_bind_env[[name]] assume normal operation of "[[" for the class.

wch commented 7 years ago

Hm, what are you using [[ for?

wch commented 7 years ago

The fix for this particular issue is simple, but I'm wondering if there's a principled reason for R6 to avoid using [[ internally while still using $.

hhoeflin commented 7 years ago

I have an object where I am simulating list like access (not so much a problem as cloning is disabled).

Another one where I implemented vector-like access, and wanted to have [[ to use for single element access into the vector.

For now I disabled [[, but I wanted to alert you to the issue. For R6 objects to be as flexible as possible, it would be great if [[ was available.

It is being used here

https://github.com/Novartis/hdf5r/blob/master/R/R6Classes_H5R.R (currently commented out, lines 609 ff)

On Fri, Sep 30, 2016 at 6:21 PM, Winston Chang notifications@github.com wrote:

The fix for this particular issue is simple, but I'm wondering if there's a principled reason for R6 to avoid using [[ internally while still using $.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wch/R6/issues/96#issuecomment-250788338, or mute the thread https://github.com/notifications/unsubscribe-auth/ACRmYldBHduyN6P4ec-3v5piZMk6nIiQks5qvTcZgaJpZM4KLFLw .

wch commented 7 years ago

I think it should be possible to replace every internal instance of $ and [[ with .subset2(), and it is also slightly faster, so I think it makes sense to do it.

library(microbenchmark)

e <- new.env()
e$x <- 1

# Test speeds of $ and .subset2, for present and missing items
microbenchmark(times = 1000,
  e$x,
  .subset2(e, "x"),
  e$y,
  .subset2(e, "y")
)
# Unit: nanoseconds
#              expr min  lq    mean median    uq  max neval
#               e$x 208 232 282.453    257 306.0 3754  1000
#  .subset2(e, "x") 178 192 229.999    217 241.0  661  1000
#               e$y 183 212 253.080    226 278.5  780  1000
#  .subset2(e, "y") 149 176 206.165    185 205.5 5587  1000
hhoeflin commented 7 years ago

Cool, thanks.

gaborcsardi commented 7 years ago

In general the internal implementations of the methods for class A should ignore the class of objects from class A completely. (I know, it sounds silly, but still. :)

In proper modern OO-ish languages like CLU from 1975, the methods get an unclass()-d object. This is the single best language feature I miss every day. It makes it completely clear where the API boundaries are.

CLU does not perform implicit type conversions. In a cluster, the explicit type conversions up and down change between the abstract type and the representation. https://en.wikipedia.org/wiki/CLU_(programming_language)

But we probably need to leave this for R7. :)

Sorry, it is just Friday afternoon.... gist is, yeah, avoiding [[ is great imo.

wch commented 7 years ago

I just pushed a fix. You can replace $ and [[ with .subset2() and get a small performance increase. But for $<- and [[<- (on an environment), you have to use assign(), which is slower. Fortunately, the assignment operators didn't turn up anywhere in the code (after the class is assigned to the object, and thus enabling dispatch to S3 methods).

For future reference (R 3.3.1 on Linux):

library(microbenchmark)

e <- new.env()
i <- 1

# Test speeds of $<- and assign(). Also increment i each time so that new values
# are assigned
microbenchmark(times = 1000,
  e$x <- (i <<- i + 1),
  assign("x", (i <<- i + 1), envir = e),
  (i <<- i + 1)  # Find how much time is spent in incrementing i
)
# Unit: nanoseconds
#                                   expr  min     lq     mean median     uq   max neval
#                   e$x <- (i <<- i + 1)  902  992.0 1253.113 1072.5 1221.5 36127  1000
#  assign("x", (i <<- i + 1), envir = e) 1148 1270.5 1523.144 1339.0 1514.0 16897  1000
#                          (i <<- i + 1)  321  344.0  419.003  353.0  383.0 11116  1000