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

Compatibity with DelayedArray 0.31.7 #110

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

hpages
Copy link
Contributor

@hpages hpages commented Jul 9, 2024

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.

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

hpages commented Jul 9, 2024

@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
Copy link
Collaborator

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

hpages commented Jul 12, 2024

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: Bioconductor/SparseArray@3a9d9fa

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

hpages commented Jul 17, 2024

The soft-deprecation of whichNonZero() in favor of nzwhich() and nzvals() in beachmat >= 2.21.4 (see tatami-inc/beachmat@376aea6) reveals a bug in nzvals() on certain DelayedArray objects.

The problem should be addressed in DelayedArray 0.31.9 (see Bioconductor/DelayedArray@5afb60e).

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.

@jonathangriffiths jonathangriffiths merged commit ded6e8d into MarioniLab:devel Jul 23, 2024
2 of 3 checks passed
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 this pull request may close these issues.

2 participants