Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recent update to 2.1.3 breaks overloading of [[ #96

Closed
hhoeflin opened this issue Sep 30, 2016 · 7 comments
Closed

Recent update to 2.1.3 breaks overloading of [[ #96

hhoeflin opened this issue Sep 30, 2016 · 7 comments

Comments

@hhoeflin
Copy link

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
Copy link
Member

wch commented Sep 30, 2016

Hm, what are you using [[ for?

@wch
Copy link
Member

wch commented Sep 30, 2016

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
Copy link
Author

hhoeflin commented Sep 30, 2016

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
#96 (comment), or mute the
thread
https://github.com/notifications/unsubscribe-auth/ACRmYldBHduyN6P4ec-3v5piZMk6nIiQks5qvTcZgaJpZM4KLFLw
.

@wch
Copy link
Member

wch commented Sep 30, 2016

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
Copy link
Author

Cool, thanks.

@gaborcsardi
Copy link
Member

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 wch closed this as completed in 434f924 Sep 30, 2016
@wch
Copy link
Member

wch commented Sep 30, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants