Skip to content
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

colQuantiles fails with length-1 probs #70

Closed
LTLA opened this issue Dec 25, 2020 · 5 comments · Fixed by #71
Closed

colQuantiles fails with length-1 probs #70

LTLA opened this issue Dec 25, 2020 · 5 comments · Fixed by #71

Comments

@LTLA
Copy link
Contributor

LTLA commented Dec 25, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

LTLA commented Dec 27, 2020

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

"DelayedSubset" = DelayedArray:::new_DelayedSubset,
"DelayedAperm" = DelayedArray:::new_DelayedAperm,
"DelayedUnaryIsoOpStack" = DelayedArray:::new_DelayedUnaryIsoOpStack,
"DelayedUnaryIsoOpWithArgs" = DelayedArray:::new_DelayedUnaryIsoOpWithArgs,
"DelayedDimnames" = function(x) {
DelayedArray:::new_DelayedDimnames(seed = x, dimnames = dimnames(x))
},
"DelayedNaryIsoOp" = function(x) DelayedArray:::new_DelayedNaryIsoOp(seed=x),

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
Copy link
Contributor

hpages commented Jan 1, 2021

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
Copy link
Contributor Author

LTLA commented Jan 2, 2021

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
Copy link
Contributor

hpages commented Jan 6, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants