r-lib / rray

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

Should the math functions be included? #230

Closed DavisVaughan closed 5 years ago

DavisVaughan commented 5 years ago

Thinks like atan2(y, x) could be useful because of broadcasting, but can we efficiently just rely on base R for everything else rather than export rray_sin() and so on?

This would lower the number of functions in the package for sure.

juangomezduaso commented 5 years ago

Yes. Names, metanames, and class are preserved by these base functions

DavisVaughan commented 5 years ago

I think the main issue with this in the past is that the calls to vec_data() would be expensive on R < 3.6. This is slightly better now (it makes 1 copy, not 2), but you still end up with 2 copies for 1 call to sin(), because you have to have 1 copy from vec_data() and then 1 copy from sin() itself on the base R array.

You have to call vec_data() to unclass x because sin() is generic and method dispatch will send you in an infinite loop otherwise.

I currently could get around this with rray_sin() because it calls rray__sin() which is not generic and doesn't care about the fact that you have an rray class attached to your matrix/array. So no vec_data() call was needed.

On R 3.6+, vec_data() doesn't make any copies (or at least delays them), and a call to sin() using the vctrs way (sin(vec_data(x))) would result in 1 copy, which would be great.

I guess I can justify this by prioritizing future versions of R over past versions.

juangomezduaso commented 5 years ago

Yes. To be just a bit slower on slow versions of R seems a fair justification