PeteHaitch / DelayedMatrixStats

A port of the matrixStats API to work with DelayedMatrix objects from the DelayedArray package
Other
15 stars 7 forks source link

colQuantiles fails with length-1 probs #70

Closed LTLA closed 3 years ago

LTLA commented 3 years ago
library(DelayedMatrixStats)
x <- matrix(runif(100000), nrow=10)
colnames(x) <- sprintf("THING%i", seq_len(ncol(x)))
colQuantiles(DelayedArray(x) + 1, prob=0.5)
## Error in dimnames(x) <- dn : 
##   length of 'dimnames' [1] not equal to array extent

This occurs because matrixStats::colQuantiles(), for whatever reason, decides to return a vector instead of a 1-column matrix by default, which breaks the subsequent rbind(). We should set drop=FALSE in the block processing and handle the drop argument in .DelayedMatrix_block_colQuantiles ourselves (which, incidentally, is not done correctly right now either).

PeteHaitch commented 3 years ago

Thanks for raising and the PR. I'm on holidays until Jan 12 and away from my computer. Are you or @hpages able to please check if this is the source of error in release and devel

LTLA commented 3 years ago

That is an unrelated error, due to the use of DelayedArray internals that have since changed their name:

https://github.com/PeteHaitch/DelayedMatrixStats/blob/f2a05aea8fd61c8c120e0cbfec5fbc3286299c81/tests/testthat/helper_test_data.R#L169-L176

The immediate fix is to change the line for DelayedDimnames to DelayedArray:::new_DelayedSetDimnames, but surely there is no need to poke around in DA internals to set up the appropriate objects here.

Upon applying this fix, I see further problems related to the formation of ddiMatrix objects, see changes in Matrix 1.3-0:

x <- matrix(0,0,0)
x2 <- as(x, "Matrix")
x2[integer(0), integer(0)]
## Error in subCsp_ij(x, i, j, drop = drop) : 
##   Cholmod error 'invalid xtype' at file ../MatrixOps/cholmod_submatrix.c, line 103

I've notified Martin about this.

hpages commented 3 years ago

FWIW I've replaced the use of new_DelayedDimnames with new_DelayedSetDimnames in DelayedMatrixStats 1.13.2. With this one out of the way, the build report is now reporting the error related to the latest changes in Matrix (Cholmod error @...): https://bioconductor.org/checkResults/3.13/bioc-LATEST/DelayedMatrixStats/malbec2-checksrc.html

LTLA commented 3 years ago

That should be fixed as of Matrix 1.3-2, when it eventually hits the shelves; manual installation allows DMS check to pass for me.

hpages commented 3 years ago

Matrix 1.3-2 now available on CRAN and starting to make its way to the Bioconductor build machines.