RcppCore / RcppArmadillo

Rcpp integration for the Armadillo templated linear algebra library
193 stars 56 forks source link

Using arma::sp_mat with new RcppArmadillo #178

Closed privefl closed 7 years ago

privefl commented 7 years ago

Concerning the problems for packages biglasso and bigstatsr with the new version of RcppArmadillo (0.8.100.1.0), I investigated more.

Please see this small reproducible example

// [[Rcpp::depends(RcppArmadillo)]]
#include <RcppArmadillo.h>

// [[Rcpp::export]]
arma::sp_mat testSpArma() {

  arma::sp_mat beta = arma::sp_mat(2, 3);

  beta(0, 0) = 1;

  return beta;
}

/*** R
packageVersion("RcppArmadillo")
library(Matrix)
test <- testSpArma()
test[]
*/

This is basically a minimal example of how we use arma::sp_mat in our two packages. With the previous CRAN version (0.7.960.1.2), it returns a dgCMatrix (from package Matrix) with one 1 at the first position. With the latest version of RcppArmadillo, it returns a dgCMatrix with only 0s.

So my question is how can we change this code in order to work with the new version of RcppArmadillo?

eddelbuettel commented 7 years ago

@privefl Good minimal example, thank you! @conradsnicta We'll take a look.

[Edited. Tab-completed the wrong handle for Conrad at first.]

eddelbuettel commented 7 years ago

Looks like it was missing a single sm.sync() in wrap() on the way out, see fb31a787699a3e66a898dacc117307ec41bdd85b

Now we get

edd@brad:~/git$ r -e 'Rcpp::sourceCpp("/tmp/rcpparma178.cpp")'

> packageVersion("RcppArmadillo")
[1] ‘0.8.100.1.0’

> suppressMessages(library(Matrix))

> test <- testSpArma()

> test[]
2 x 3 sparse Matrix of class "dgCMatrix"

[1,] 1 . .
[2,] . . .
edd@brad:~/git$

@privefl It's early morning here and I won't get to it for a bit but could re-run the tests for biglasso and bigstatr to see how things work now?

privefl commented 7 years ago

Seems OK for both packages.

Thanks @conradsnicta and @eddelbuettel

eddelbuettel commented 7 years ago

Awesome. This was simply one assignment we missed "on the way out" from (Rcpp)Armadillo back to R. But as it is critically important, it is no wonder the tests failed. That's why we run them.

My bad for submitting to CRAN with a bad set if tests (using previous RcppArmadillo). I'll re-run the battery of all of them here.

binxiangni commented 7 years ago

Sorry to mess it up. My bad for not checking the wrap(). 😭

eddelbuettel commented 7 years ago

No worries, it happens. I also messed the last reverse depends check up and should have seen thos. In a a way it is more on all of us to not have thought of a unit test for that case either.

I confirmed here that biglasso and bigstatr are now fine and launched a new reverse-depends check against all (and by now 405) packages.

coatless commented 7 years ago

@binxiangni Heh, if only you knew how often it happens ;-)

binxiangni commented 7 years ago

@coatless Hah, I’ll continue to mess it up and prove how often ;-)

eddelbuettel commented 7 years ago

See, Google of Summer Code really works but you now really know how we operate :)

Reverse-depends test looks good. About 1/3 done, not one issue.

gvegayon commented 7 years ago

fb31a787699a3e66a898dacc117307ec41bdd85b works for me now with https://github.com/USCCANA/netdiffuseR