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

Rewrite conversion for ddiMatrix #145

Merged
merged 1 commit into from Jul 11, 2017

Conversation

Projects
None yet
4 participants
@binxiangni
Contributor

binxiangni commented Jul 10, 2017

When doing tests for ddiMatrix, an extreme case came out that drag == "N" and 0 in x. So I rewrite the conversion for ddiMatrix to support that.

The unit tests for sparse matrix conversion are also moved to runit.sparseConversion.R to keep the functionality.

When looking through the documentation of Armadillo, I find that it might not support boolean matrix (please correct me if I am wrong). The multiplication of boolean matrix is different from that of numeric matrix. So the conversion for lsparseMatrix and nsparseMatrix might not make sense and could be left to be done when Armadillo supports the operation of boolean matrices. Next I'll add warning message for lsparseMatrix and nsparseMatrix and maintain the conversion after the new feature for boolean sparse matrix comes up in Armadillo.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 10, 2017

Member

Correct on lack of boolean. We could map on the way in -- Armadillo uses umat for indexing. Or we could just say "cannot do". Unsure.

Member

eddelbuettel commented Jul 10, 2017

Correct on lack of boolean. We could map on the way in -- Armadillo uses umat for indexing. Or we could just say "cannot do". Unsure.

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jul 10, 2017

Contributor

I think that even if the boolean matrix can be represented in umat, the multiplication for umat might be wrong. It will regard the matrices as numeric matrices and apply multiplication of numeric matrices to them rather than that of boolean matrices. I write a small program to test it.

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

using namespace Rcpp;
using namespace arma;

// [[Rcpp::export]]
umat times(umat X, umat Y) {
  return X * Y;
}

/*** R
library(Matrix)
xtxt <- c("1 1 0 1",
          "0 1 1 0")
X <- as.matrix(read.table(text=xtxt))
dimnames(X) <- NULL
ytxt <- c("1 1 0",
                "0 1 0",
                "0 1 1",
                "1 0 0")
Y <- as.matrix(read.table(text=ytxt))
dimnames(Y) <- NULL
times(X, Y)
*/

//times(X, Y)
//     [,1] [,2] [,3]
//[1,]    2    2    0
//[2,]    0    2    1

As page 176th of the note points out, the result should be

     [,1] [,2] [,3]
[1,]    1    1    0
[2,]    0    1    1
Contributor

binxiangni commented Jul 10, 2017

I think that even if the boolean matrix can be represented in umat, the multiplication for umat might be wrong. It will regard the matrices as numeric matrices and apply multiplication of numeric matrices to them rather than that of boolean matrices. I write a small program to test it.

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

using namespace Rcpp;
using namespace arma;

// [[Rcpp::export]]
umat times(umat X, umat Y) {
  return X * Y;
}

/*** R
library(Matrix)
xtxt <- c("1 1 0 1",
          "0 1 1 0")
X <- as.matrix(read.table(text=xtxt))
dimnames(X) <- NULL
ytxt <- c("1 1 0",
                "0 1 0",
                "0 1 1",
                "1 0 0")
Y <- as.matrix(read.table(text=ytxt))
dimnames(Y) <- NULL
times(X, Y)
*/

//times(X, Y)
//     [,1] [,2] [,3]
//[1,]    2    2    0
//[2,]    0    2    1

As page 176th of the note points out, the result should be

     [,1] [,2] [,3]
[1,]    1    1    0
[2,]    0    1    1
@coatless

This comment has been minimized.

Show comment
Hide comment
Contributor

coatless commented Jul 10, 2017

@thirdwing

This comment has been minimized.

Show comment
Hide comment
@thirdwing

thirdwing Jul 11, 2017

Member

It is OK if we can't support every type. Some friendly warnings should be fine.

Member

thirdwing commented Jul 11, 2017

It is OK if we can't support every type. Some friendly warnings should be fine.

@eddelbuettel eddelbuettel merged commit 7a50d12 into RcppCore:master Jul 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment