Add conversion & unit tests for pMatrix & ddiMatrix #140
Conversation
|
I think Matrix lives here as an SVN repo on R-Forge. That is consistent with its DESCRIPTION file. |
inst/include/RcppArmadilloAs.h
Outdated
| @@ -403,14 +403,61 @@ namespace traits { | |||
| res = symmatl(res); | |||
| } | |||
| } | |||
| else if (type == "pMatrix") { | |||
| IntegerVector i(ncol); | |||
| IntegerVector p(ncol+1); | |||
There was a problem hiding this comment.
Not sure about this one...
Consider:
(pm1 <- as(as.integer(c(2,3,1)), "pMatrix"))
# 3 x 3 sparse Matrix of class "pMatrix"
#
# [1,] . | .
# [2,] . . |
# [3,] | . .
str(pm1)
# Formal class 'pMatrix' [package "Matrix"] with 4 slots
# ..@ perm : int [1:3] 2 3 1
# ..@ Dim : int [1:2] 3 3
# ..@ Dimnames:List of 2
# .. ..$ : NULL
# .. ..$ : NULL
# ..@ factors : list()Built off of: https://stat.ethz.ch/R-manual/R-devel/library/Matrix/html/pMatrix-class.html
There was a problem hiding this comment.
I perhaps know what you mean. Let me see...
There was a problem hiding this comment.
Also not sure about converting permutation matrix into a numeric matrix.
Maybe we can discuss this in another issue or PR. How about removing this for now?
There was a problem hiding this comment.
Now the bug for pMatrix has been fixed. I used the matrix James provided and passed the unit test locally.
|
|
||
| // Calculate i | ||
| for(std::map<int, int>::iterator tmp = colrow.begin(); tmp != colrow.end(); tmp++){ | ||
| i.push_back(tmp -> second); |
There was a problem hiding this comment.
Using .push_back is problematic on IntegerVector because Rcpp objects are proxy models that serve as shallow wrappers around R objects. Thus, each time you add an element, there is a significant overhead associated with copying the existing vector to a new vector. It would be much better in this case to directly use std::vector<int> and then cast at the end to IntegerVector.
| std::copy(x.begin(), x.end(), arma::access::rwp(res.values)); | ||
| } | ||
| else if (type == "ddiMatrix") { | ||
| IntegerVector i(ncol); |
There was a problem hiding this comment.
This also needs to be tweaked slightly as it does not take into account N or U classes of the ddiMatrix.
e.g.
(d_U <- Diagonal(n = 3))
# 3 x 3 diagonal matrix of class "ddiMatrix"
# [,1] [,2] [,3]
# [1,] 1 . .
# [2,] . 1 .
# [3,] . . 1
str(d_U)
# Formal class 'ddiMatrix' [package "Matrix"] with 4 slots
# ..@ diag : chr "U"
# ..@ Dim : int [1:2] 3 3
# ..@ Dimnames:List of 2
# .. ..$ : NULL
# .. ..$ : NULL
# ..@ x : num(0) (d_N <- Diagonal(x = c(10,1)))
# 2 x 2 diagonal matrix of class "ddiMatrix"
# [,1] [,2]
# [1,] 10 .
# [2,] . 1
str(d_N)
# Formal class 'ddiMatrix' [package "Matrix"] with 4 slots
# ..@ diag : chr "N"
# ..@ Dim : int [1:2] 2 2
# ..@ Dimnames:List of 2
# .. ..$ : NULL
# .. ..$ : NULL
# ..@ x : num [1:2] 10 1
|
I think I can merge this now. Any objections? |
|
Sorry for not seeing this earlier. Everything looks to be in a better state. |
In order to solve #17 and #114 . Now the conversion for all the numeric sparse matrices is done. @thirdwing @eddelbuettel When doing unit tests, I found some bugs in the package Matrix. But there is only a read-only repo on the GitHub so that I cannot open a pull request. Do you have any idea where I can contribute?