derekbeaton / GSVD

19 stars 6 forks source link

tolerance_eigen error with rank 1 matrices #32

Open LukeMoraglia opened 1 year ago

LukeMoraglia commented 1 year ago

Hey Derek,

I got an error when using tolerance_eigen on a rank-1 matrix. The error comes from the call to colSums and says

Error in colSums(eigen_res$vectors) : 
  'x' must be an array of at least two dimensions

In line 64 below, eigen_res$vectors gets converted to a vector when there is only one eigenvalue to keep. https://github.com/derekbeaton/GSVD/blob/0f41cf29bcdcadc6c3a45847427df83e5226ceb3/R/tolerance_eigen.R#L64-L68

I converted back to matrix and that seems to have fixed the issue for me. https://github.com/LukeMoraglia/GSVD/blob/a47ee067abe3a468137b34a4d5fd0984abca40ad/R/tolerance_eigen.R#L64

Code to reproduce error

library(GSVD)
set.seed(42)
X <- matrix(sample.int(20, 20*3, replace = TRUE), 20, 3)
R <- cor(X)

# Create a rank-1 matrix from R
eig_R <- tolerance_eigen(R)
R_rank1 <- as.matrix(eig_R$vectors[,1]) %*% eig_R$values[1] %*% t(as.matrix(eig_R$vectors[,1]))

tolerance_eigen(R_rank1)
derekbeaton commented 1 year ago

Hi Luke,

Thanks for catching this and for suggesting a fix. I think a safer---maybe even more standard?---fix would be:

eigen_res$vectors <- eigen_res$vectors[,evs.to.keep,drop=FALSE]

If you don't mind: Could you try that version? In either case, the above or your approach, could you create a pull request so I can merge your change into the code? If you do, please also add yourself as [ctb] to the DESCRIPTION file and in the author spot for this file

LukeMoraglia commented 1 year ago

@derekbeaton,

Awesome, I like the drop=FALSE solution better! Can do with a pull request. For adding to the DESCRIPTION file, should I add myself as [ctb] to the Author, Authors@R, or both?

LukeMoraglia commented 1 year ago

Submitted PR #33 with a fix. I didn't add myself as ctb to Authors@R