search

Bioconductor / DelayedArray

A unified framework for working transparently with on-disk and in-memory array-like datasets
https://bioconductor.org/packages/DelayedArray
24 stars 9 forks source link

DelayedMatrixStats no longer supports scalar `center=` arguments #115

Closed LTLA closed 1 month ago

LTLA commented 4 months ago

PeteHaitch/DelayedMatrixStats@9048f3e7a24035f3526a2ac0455598138e0c8480 causes

https://github.com/Bioconductor/DelayedArray/blob/700cfdc38e0f97a67942963c1555d0fd83a7bc41/R/DelayedArray-utils.R#L868

to fail, for example:

library(DelayedArray)
mat <- DelayedArray(matrix(runif(500), 25, 20))
scale(mat)
## Error in blockfun(x, ..., useNames = useNames) :
##   length(center) == nrow(x) is not TRUE

Simple fix is to just do center=numeric(nrow(x)).

Session info ``` R Under development (unstable) (2024-04-10 r86396) Platform: aarch64-apple-darwin22.6.0 Running under: macOS Ventura 13.6.4 Matrix products: default BLAS: /Users/luna/Software/R/trunk/lib/libRblas.dylib LAPACK: /Users/luna/Software/R/trunk/lib/libRlapack.dylib; LAPACK version 3.12.0 locale: [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8 time zone: America/Los_Angeles tzcode source: internal attached base packages: [1] stats4 stats graphics grDevices utils datasets methods [8] base other attached packages: [1] DelayedArray_0.29.9 SparseArray_1.3.5 S4Arrays_1.3.7 [4] abind_1.4-5 IRanges_2.37.1 S4Vectors_0.41.6 [7] MatrixGenerics_1.15.0 matrixStats_1.3.0 BiocGenerics_0.49.1 [10] Matrix_1.7-0 loaded via a namespace (and not attached): [1] DelayedMatrixStats_1.25.2 sparseMatrixStats_1.15.0 [3] zlibbioc_1.49.3 lattice_0.22-6 [5] XVector_0.43.1 grid_4.5.0 [7] compiler_4.5.0 tools_4.5.0 [9] Rcpp_1.0.12 crayon_1.5.2 ```
hpages commented 4 months ago

@PeteHaitch Would you be open to the possibility of keeping length-1 centers valid in DelayedMatrixStats, at least for a little bit longer? Length-1 centers in DelayedArray, SparseArray and sparseMatrixStats have been supported for a while and they didn't start issuing warnings recently despite the fact that matrixStats doesn't like them anymore. So having them now cause a hard error in DelayedMatrixStats is inconsistent with DelayedArray, SparseArray and sparseMatrixStats .

Also I can't think of anything wrong with supporting length-1 centers -- they're just a convenience like recycling.

If you agree with this, I'll submit a patch to DelayedMatrixStats. The patch will simply recycle length-1 centers to nrow(x) or ncol(x).

Thanks, H.

PeteHaitch commented 4 months ago

Argh sorry for this. I was trying to follow the defunct-ification by Henrik in matrixStats (https://github.com/HenrikBengtsson/matrixStats/issues/254), since we've (mostly) tried to mimic its API. A patch would be welcome (it might just be reverting the most recent commits).

hpages commented 4 months ago

No worries. Working on it. Will also involve patching MatrixGenerics to make things consistent across the board.

hpages commented 4 months ago

PR for DelayedMatrixStats ready: https://github.com/PeteHaitch/DelayedMatrixStats/pull/101

PeteHaitch commented 4 months ago

Thanks, @hpages I'm in the midst of resolving OSCA build issues and I'll then turn to this (hopefully Friday).

hpages commented 1 month ago

Fixed by https://github.com/PeteHaitch/DelayedMatrixStats/pull/101