MarioniLab / DropletUtils

Clone of the Bioconductor repository for the DropletUtils package.
https://bioconductor.org/packages/devel/bioc/html/DropletUtils.html
56 stars 27 forks source link

Compatibity with DelayedArray 0.31.7 #110

Closed hpages closed 3 months ago

hpages commented 3 months ago

The following breaking changes happened between DelayedArray < 0.31.5 and 0.31.7: The OLD_extract_sparse_array() generic and SparseArraySeed objects are now deprecated in favor of the new extract_sparse_array() generic defined in the SparseArray package. Methods for the new extract_sparse_array() generic are expected to return a SparseArray derivative, typically an SVT_SparseArray object but COO_SparseArray objects are ok.

This commit restores compatibility with DelayedArray 0.31.7.

IMPORTANT NOTE: Some unit tests in DropletUtils won't pass until the beachmat package is modified as follow:

  1. Add a whichNonZero() method for COO_SparseArray objects.
  2. Coerce to COO_SparseArray instead of SparseArraySeed in the whichNonZero() method for DelayedMatrix objects.

Best, H.

hpages commented 3 months ago

@LTLA FWIW the only thing I did to beachmat in order to succesfully run R CMD check on DropletUtils 1.25.1 was:

hpages@XPS15:~/beachmat$ git diff
diff --git a/R/whichNonZero.R b/R/whichNonZero.R
index cf05a2b..63f843f 100644
--- a/R/whichNonZero.R
+++ b/R/whichNonZero.R
@@ -65,15 +65,27 @@ setMethod("whichNonZero", "SparseArraySeed", function(x, ...) {
     list(i=idx[,1], j=idx[,2], x=nzdata(x))
 })

+#' @export
+#' @rdname whichNonZero
+#' @importClassesFrom SparseArray COO_SparseArray
+#' @importFrom SparseArray nzcoo nzdata
+setMethod("whichNonZero", "COO_SparseArray", function(x, ...) {
+    idx <- nzcoo(x)
+    if (ncol(idx)!=2) {
+        stop("'x' should be a 2-dimensional COO_SparseArray")
+    }
+    list(i=idx[,1], j=idx[,2], x=nzdata(x))
+})
+
 #' @export
 #' @rdname whichNonZero
 #' @importFrom DelayedArray getAutoBPPARAM setAutoBPPARAM 
-#' @importClassesFrom DelayedArray SparseArraySeed
+#' @importClassesFrom SparseArray COO_SparseArray
 setMethod("whichNonZero", "DelayedMatrix", function(x, BPPARAM=NULL, ...) {
     oldBP <- getAutoBPPARAM()
     setAutoBPPARAM(BPPARAM)
     on.exit(setAutoBPPARAM(oldBP))

-    out <- as(x, "SparseArraySeed")
+    out <- as(x, "COO_SparseArray")
     callGeneric(out)
 })

Of course more changes are needed in order to make beachmat fully compatible with DelayedArray 0.31.7 but that's a start.

jonathangriffiths commented 3 months ago

Thanks for so proactively making this change.

SparseArray 1.5.19 is required in the DESCRIPTION for this patch, but it is not (yet) available on Bioc devel. I notice that DelayedArray has a version requirement only of 1.5.18 at the moment in its github repo.

Is this version 1.5.19 something I should wait for before merging this, or is it a typo?

hpages commented 3 months ago

Is this version 1.5.19 something I should wait for before merging this, or is it a typo?

Ooops, it's a version I have locally on my laptop but that I never pushed, sorry. Just pushed it now: https://github.com/Bioconductor/SparseArray/commit/3a9d9fab436393d2122c98837da6bd131f4fb439

Anyways, SparseArray 1.5.19 doesn't contain any meaningful change compared to 1.5.18 so I adjusted the patch to only require the latter.

H.

hpages commented 3 months ago

The soft-deprecation of whichNonZero() in favor of nzwhich() and nzvals() in beachmat >= 2.21.4 (see https://github.com/tatami-inc/beachmat/commit/376aea64f891896249b25181a28ec2e89e59c0df) reveals a bug in nzvals() on certain DelayedArray objects.

The problem should be addressed in DelayedArray 0.31.9 (see https://github.com/Bioconductor/DelayedArray/commit/5afb60eaaf583facaebd54a848db704d907d6e2a).

FWIW DropletUtils 1.25.1 passes R CMD check for me with the latest versions of SparseArray (1.5.22), DelayedArray (0.31.9), and beachmat (2.21.4), so hopefully the automated checks here will pass once DelayedArray 0.31.9 has propagated to the BioC 3.20 packages repos.

H.