r-lib / rray

Simple Arrays
https://rray.r-lib.org
GNU General Public License v3.0
130 stars 12 forks source link

Comparison and arithmetic operator return values #170

Closed DavisVaughan closed 5 years ago

DavisVaughan commented 5 years ago

I don't think we should be calling vec_restore() from arithmetic/comparison operators. Probably just new_rray(). The thing is, methods like > will only ever be called if 1 of the 2 inputs is an rray. So then then output would be an rray. We also expose rray_greater() so that we can use the broadcasting ability with base R objects. To keep the output the same as the input, we use vec_restore() combined with vec_type2(). But this has issues, because we are currently hacking vec_restore() by letting it restore to a different inner type when that isn't what it is used for (see #169 ). With >, we would get a vec_type2() result that doesn't match the result type, which is always logical.

Hopefully if we just call new_rray() we can make vec_restore() simpler by just leveraging the default behavior from vctrs.

DavisVaughan commented 5 years ago

Or maybe for things like x + y if x is a logical, it should be intercepted at vec_arith.vctrs_rray_lgl.double and promoted to an integer before proceeding?

juangomezduaso commented 5 years ago

I see. What about this for the case of compare?:

cast_compare<-function (f, x, y) 
{
    if (is.null(x)) {
        x <- logical()
    }
    if (is.null(y)) {
        y <- logical()
    }
    args <- rray_cast_inner_common(x, y)
    res <- f(args[[1]], args[[2]])
    res <- set_full_dim_names(res, rray_dim_names_common(x, y))
    #######  vec_restore(res, vec_type2(x, y))
    class(res)<-class(vec_type_2(x, y))          #### NEW
    res                                          #### NEW

}
DavisVaughan commented 5 years ago

That wouldn't quite work because compare always returns a logical, but the common type of x and y might be an rray of doubles, which has a vctrs_rray_dbl class attached. I've been thinking about this issue for a day or so now. It's a tough problem

juangomezduaso commented 5 years ago

Yes it is. I had forgotten about the "typeof" subclasses. A possible enhancement could be to change the class somehow. For instance with something like this: gsub("^vctrsrray(dbl|int)$","vctrs_rray_lgl", c("vctrs_dbl","vctrs_rray")) or depending on what the authors of subclasses of rray are expected to name them, gsub("^vctrsrray(dbl|int)","vctrs_rray_lgl", c("vctrs_dbl","vctrs_rray")) or more sophisticated expressions. But i better stop throwing half thougth ideas and wait for what you end up doing after reflecting on it

juangomezduaso commented 5 years ago

I wonder if rray methods of functions like vec_type2() , or the vec_cast_container() that HadleyW mentioned in the 340 vctrs issue , should take care of dimnames. Types are supossed to capture all metadata of interest to a class and just as , for instance, the factor methods care about levels (calculating their union), perhaps rray should do so with dimnames inside these methods.

DavisVaughan commented 5 years ago

I made my own vec_cast_container() for now, if you are interested in taking a look.

https://github.com/DavisVaughan/rray/blob/cast-container/R/cast-container.R

It restores the fact that we started with rrays, but otherwise x stays the same (if it was a logical, it will still be a logical. The shape stays the same. The dim names stay the same)

It basically replaced all of my calls to vec_restore()

DavisVaughan commented 5 years ago

173