bnprks / BPCells

Scaling Single Cell Analysis to Millions of Cells
https://bnprks.github.io/BPCells
Other
167 stars 17 forks source link

export rbind2 and cbind2 methods? #25

Closed brgew closed 1 year ago

brgew commented 1 year ago

Hi Ben,

I hope that you haven't answered this question already.

I am testing some matrix combining code and I see errors telling me that BPCells::rbind2 and BPCells::cbind2 are not exported. I modified the BPCells/R/matrix.R code to export the methods and our code works now. Am I messing up yet again? (I hope not.)

If it seems reasonable to add these exports, I can create a pull request with the modifications.

I appreciate your patience with me.

Ever grateful, Brent

bnprks commented 1 year ago

The S4 generics are definitely tricky so it's possible this requires an export, though I didn't think it would.

rbind2 and cbind2 are generics from the build-in methods package, which BPCells provides specialized implementations for when using IterableMatrix objects. So my expectation would be that using rbind2 or methods::rbind2 would work, and BPCells::rbind2 probably wouldn't.

If that doesn't work a pull request would be fine, though I'd want to confirm the problem for myself before throwing in the export annotation -- if you could provide some short example code that hits the issues that would be nice.

brgew commented 1 year ago

Hi Ben,

This is an occasion on which I did some checking BEFORE I opened this issue.

I googled for Roxygen2 exportMethod and found

(https://cran.r-project.org/web/packages/roxygen2/vignettes/namespace.html)

which says

Exports For a function to be usable outside your package, you must export it. By default roxygen2 doesn’t export anything from your package. If you want an object to be publicly available, you must explicitly tag it with @export.

Use the following guidelines to decide what to export:

I interpreted the last bullet point to mean that the rbind2 and cbind2 may need to be exported.

However, I will work on an example too.

Ever grateful, Brent

bnprks commented 1 year ago

Yeah, I've seen those docs and I agree with you on that last bullet point.

Weirdly, this does not match with some of my experimenting. For instance, this works fine for me:

library(BPCells)
m <- matrix(1:12, nrow=3)
m <- as(m, "dgCMatrix")
m <- as(m, "IterableMatrix")
m2 <- rbind2(m, m)
m3 <- cbind2(m, m)

But maybe the issue only pops up when in use in another package which wouldn't have the library(BPCells) call. I'm basically just wanting to double-check you weren't prefixing your calls with BPCells::rbind2, and that both rbind2 and methods::rbind2 fail for you in the same context.

If that all still fails, then that's a good catch you've made and I should learn to trust the docs more :) No need to make a perfectly exhaustive example if it's only a problem within a package, just seeing the call site and error message are likely enough for me to reproduce.

brgew commented 1 year ago

Hi Ben,

Hmm. I am leery of making a fool of myself: but my understanding of R is weak and my feet are already well-perforated so...

Here is an example that fails, and I expect it to succeed

library(BPCells)

v1 <- c(1,2,3,4,5,6)
m1 <- matrix(v1,nrow=2)
bpm1 <- BPCells:::write_matrix_memory(as(m1, 'dgCMatrix'))

v2 <- c(7,8,9,10,11,12)
m2 <- matrix(v2,nrow=2)
bpm2 <- BPCells:::write_matrix_memory(as(m2, 'dgCMatrix'))

cm1 <- BPCells::rbind2(bpm1, bpm2)

The error message is

Error: 'rbind2' is not an exported object from 'namespace:BPCells'

I expect that the call to rbind2 with its qualification should work, if rbind2 is exported. The BPCells::rbind2 definition looks like (as you know well, I am sure)

setMethod("rbind2", signature(x = "IterableMatrix", y = "IterableMatrix"), function(x, y, ...)

so I (naively) expect that calling BPCells::rbind2() with both arguments as BPCells objects should resolve to the BPCells::rbind2() method.

I confess that I don't know what to expect when BPCells::rbind2()is called with one dgCMatrix object and one BPCells object. My experience with this is nil. Maybe there is a default method for the generic?

Incidentally, in Monocle3, I am not importing BPCells so I qualify the calls with the BPCells package name.

Oh, I think that your comment about using BPCells in another package may point to the problem. Could it be that loading BPCells in the session and using the unqualified rbind2 results in using the rbind2 that you loaded but qualifying it forces R to ignore the loaded rbind2() in your session, and try to import it from the installed BPCells package (as happens when I call BPCells::rbind2() in Monocle3)?

Ever grateful, Brent

bnprks commented 1 year ago

Hi Brent,

We're all learning here so no need to be self-conscious :). I think we're getting to territory that's at the edge of my own knowledge (but hopefully just barely within). In general, I'd highly recommend the Advanced R online book for a reference on S3 and S4 generics: link

If I change your last line to:

cm1 <- methods::rbind2(bpm1, bpm2)

then it works for me with no problem.

The distinction here is that the function (rbind2) is not defined in BPCells but rather the methods package(via setGeneric() ). What is defined in BPCells is a method, which specifies what should happen when rbind2 is called with IterableMatrix objects (via setMethod()).

Now, I don't know when/where R picks up on the fact that BPCells has registered a specialization of rbind2, but it appears to not rely on having called library(BPCells):

v1 <- c(1,2,3,4,5,6)
m1 <- matrix(v1,nrow=2)
bpm1 <- BPCells::write_matrix_memory(as(m1, 'dgCMatrix'))

v2 <- c(7,8,9,10,11,12)
m2 <- matrix(v2,nrow=2)
bpm2 <- BPCells::write_matrix_memory(as(m2, 'dgCMatrix'))

cm1 <- rbind2(bpm1, bpm2)

The above runs for me with no error in a fresh R terminal

brgew commented 1 year ago

Hi Ben,

I am surely confused.

Ever grateful, Brent