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

Support for Rcpp::as< arma::Cube<T> > #63

Closed
nathan-russell opened this issue Nov 30, 2015 · 5 comments
Closed

Support for Rcpp::as< arma::Cube<T> > #63

nathan-russell opened this issue Nov 30, 2015 · 5 comments

Comments

@nathan-russell
Copy link
Contributor

I took a shot at implementing a means of converting Rcpp::Vectors to arma::Cubes, but wanted to get some feedback on my code before submitting a PR because I think there are some minor sticking points worth discussing. The full change is reflected in RcppArmadilloAs.h on my fork, in the form of

  1. a specialization of Rcpp::Exporter< arma::Cube<T> >; and
  2. 3 further Exporter specializations, one for each of arma::fcube, arma::ucube, and arma::cx_fcube

The first specialization works for fine for the Cube<double>, Cube<sword>, and Cube<cx_double>, but was choking on the remaining three typedefs. I could not come up with an elegant approach to encapsulate everything in one template, so I ended up with the additional three specializations that "piggy back" on their counterparts which do compile; e.g. to get an arma::Cube<float> / arma::fcube, a SEXP is first converted to an arma::cube using (1), and then converted to an arma::fcube using Armadillo's conv_to< >::from utility. Presumably this adds additional overhead which is not ideal, but as stated, I was unable to avoid this extra step. I am certainly open to any alternative suggestions however.

In my unit test file (using the C++ functions here), two of the tests involving arma::cx_fcubes use a modified tolerance value of 1.5e-7 when compared to base R's equivalent calculation. I suspect this has to do with a loss of precision in the conversion from double to float, but regardless I wanted to call attention to this.

Finally, my Exporter specialization is written such that an exception will be thrown if the input object to be converted does not have exactly three dimensions. Is this desirable?

Taking into account everything above, the corresponding Travis build was successful, but I certainly some discussion is in order.

@eddelbuettel
Copy link
Member

That is awesome. I meant to follow-up on the StackOverflow question suggesting exactly that.

Conrad is playing things old-school so we cannot ping him just by GitHub handle. I will send him an email.

Back in the real world, I am not sure we really need much besides the double case. Armadillo supports float, but R doesn't. Ditto for all things unsigned, and complex is rare too.

Now, you are of course more rigorous than my thinking-out-loud here so I with that 👍 for the two-step conversion which is clearly better than not having any. And remember: we cannot get from float to R's double without copying anyway...

I'll try to take a closer look.

@eddelbuettel
Copy link
Member

By the way, Conrad did toss a pre-release of Armadillo 6.300 at me which I'll put through a full reverse-depends check. If you prepare a PR I could merge it into that test branch too... This is probably going to be fairly orthogonal and only touch "our" files. But no rush -- we can also deal with this in a few days.

@nathan-russell
Copy link
Contributor Author

That sounds great Dirk. I also was thinking that people are much less inclined to reach for float / unsigned int / std::complex<float> than their counterparts, but figured it was worthwhile to have the compatibility. I'll get started on putting this into a PR.

@eddelbuettel
Copy link
Member

👍

@eddelbuettel
Copy link
Member

I guess we forgot a (closes #63) tag in PR 64. So closing by hand.

Thanks again!

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

2 participants