rcppmlpack / rcppmlpack2

Rcpp Interface to mlpack (version 2.1.0 and up)
GNU General Public License v2.0
24 stars 9 forks source link

new example src/naive_bayes.cpp #3

Closed MHenderson closed 7 years ago

MHenderson commented 7 years ago

update header, also include generated RcppExports.R

eddelbuettel commented 7 years ago

Nice, really appreciate this. I made one comment on the return type. Could we foresee returning more than a single row? If not, then arma::rowvec (a more compact alias of the typedef'ed variant you had) may be easier.

(Edited to make it rowvec, not row)

eddelbuettel commented 7 years ago

Let me try again down here. I was basically taken aback when I saw arma::Row<size_t> because ... well we had never seen it. It is not an idiom by Conrad and not used in RcppArmadillo -- only Ryan and the MLPACK crew use it.

Oddly, it compiles. But as the example below shows, it does the wrong thing as size_t is unsigned, and R does not have that -- so we wrap around, silently. Which is a bad thing.

Code:

#include <RcppArmadillo.h>

// [[Rcpp::depends(RcppArmadillo)]]

// [[Rcpp::export]]
arma::Col<size_t> foo(arma::Row<size_t> v) {
  v.print();
  return v.t();
}

/*** R
foo(-1:3)
*/

Running it:

R> Rcpp::sourceCpp("/tmp/armarow.cpp")

R> foo(-1:3)
18446744073709551615            0            1            2            3
            [,1]
[1,] 1.84467e+19
[2,] 0.00000e+00
[3,] 1.00000e+00
[4,] 2.00000e+00
[5,] 3.00000e+00
R> 
MHenderson commented 7 years ago

Okay, I think I've fixed this. Using a return type of arma::irowvec and then using arma::conv_to to convert the result from MLPACK from arma::Row<size_t> to arma::irowvec.

eddelbuettel commented 7 years ago

Thanks for your patience and persistence. I'll merge this.

I think we should use irowvec on the interface from R too. Then maybe test that we have no negative values, convert via conv_to to what MLPACK wants and proceed.

eddelbuettel commented 7 years ago

BTW, adding a check for negative value is check:

if (arma::any( rowvecVar < 0) ) Rcpp:stop("Screaming and yelling")

Will add next time