Skip to content
New issue

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

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

Already on GitHub? # to your account

rray removes custom classes #247

Open
paul-buerkner opened this issue Oct 3, 2019 · 3 comments
Open

rray removes custom classes #247

paul-buerkner opened this issue Oct 3, 2019 · 3 comments
Labels
feature ✨ a feature request or enhancement future

Comments

@paul-buerkner
Copy link

We are currently in the process of writing a new package to efficiently handle posterior samples as return by MCMC samplers of Bayesian models and we consider using rray for matrix and array representations. However, for easy S3 method dispatch, we need to have custom classes on top of rray's classes that simply indicate that the rray is in the right format (without having to check again every time). Unfortunately, rray removes custom classes, while base R array does not. Here is a reproducible example:

A <- array(1:27, dim = c(3, 3, 3))
class(A) <- c("draws_array", class(A))
class(A)
> [1] "draws_array" "array" 
class(A+A)
> [1] "draws_array" "array" 


B <- rray::as_rray(array(1:27, dim = c(3, 3, 3)))
class(B) <- c("draws_array", class(B))
class(B)
> [1] "draws_array"    "vctrs_rray_int" "vctrs_rray"     "vctrs_vctr"
class(B+B)
> [1] "vctrs_rray_int" "vctrs_rray"     "vctrs_vctr"

Is there a way to preserve custom classes for all vectorized (pointwise) operations without having to redefine every numerical function rray provides for its objects?

@DavisVaughan
Copy link
Member

There is, but I haven't exported it yet (it isn't fully tested). If you look at rray_add(), which is what + eventually calls, you'll see vec_ptype_container2() and vec_cast_container() called at the end. These are double dispatching generic functions which restore the "container type" on top of the raw array that is returned from the C code.

In a dev version I had exported these, but decided against it before release because I am not sure if this is a good long term solution for extensibility. The ideas are still being developed in vctrs, and I didn't want to provide something that we would have to immediately break. vctrs doesn't currently have the "container type" idea sorted out yet (see r-lib/vctrs#211).

I'm not currently working on rray, so it would be awhile before I could get back around to this. If you want to take a shot at reexporting vec_cast_container and vec_ptype_container2 in a fork, then defining methods for them in your package for your draws_array class, then that might work. If it works smoothly, I'd be willing to review a PR for this.

If you aren't familiar with the double dispatch idea yet, see this vignette:
https://vctrs.r-lib.org/articles/s3-vector.html#double-dispatch

Here is the commit where I un-exported the functions. You'd have to add the roxygen tags back (also note that there was a name change after this type -> ptype), and maybe change some of the internal documentation that I actually left in.
97c8f5f

@DavisVaughan
Copy link
Member

Do be aware that due to some xtensor-r bugs, I was not able to get missing values implemented in this first version 😢 #21

@paul-buerkner
Copy link
Author

Thank you for this super helpful answer! I will take a look at the non-exported solution you provided. At least for the time being, it may be safer for our purposes to use base R arrays (with drop defaulting to FALSE) and switch to rrays once there is an exported way preserving custom classes in rray.

@DavisVaughan DavisVaughan added feature ✨ a feature request or enhancement future labels Oct 4, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
feature ✨ a feature request or enhancement future
Projects
None yet
Development

No branches or pull requests

2 participants