search

drisso / SingleCellExperiment

Clone of the Bioconductor repository for the SingleCellExperiment package, see https://bioconductor.org/packages/devel/bioc/html/SingleCellExperiment.html for the official development version.
63 stars 17 forks source link

Improve `setReplaceMethod` for `sizeFactors` #34

Closed mjsteinbaugh closed 4 years ago

mjsteinbaugh commented 4 years ago

Can we restrict the default sizeFactors<- assignment method to use numeric rather than ANY? This follows the conventions used in DESeq2.

Note that I didn't update the roxygen documentation and NAMESPACE in this pull request.

LTLA commented 4 years ago

Is there a practical reason for this? I don't feel any particular need to follow DESeq2's conventions, and restricting to numeric will require an additional method to set NULL.

mjsteinbaugh commented 4 years ago

Here's why I think this would be practical, and setting a separate NULL method is worth it. Currently, you can do operations like this, which make no sense to me:

sizeFactors(sce) <- "XXX"
head(sizeFactors(sce))
## [1] "XXX" "XXX" "XXX" "XXX" "XXX" "XXX"

Also, from the ANY method it's not immediately obvious that you can set the sizeFactors as NULL using the assignment method, which isn't always the case for certain S4 generics.

LTLA commented 4 years ago

I'd be surprised if anyone actually did set sizeFactors to a character vector, but I suppose it's better to be safe than sorry.

The counter-argument against restricting to numeric is that it prevents people from storing size factors in numeric-like S4 vectors, e.g., run-length encodings. However, this is an unlikely use case given that the size factors don't take much memory to store anyway.

I can't say I'm thrilled with the break from the existing coding style, but if you add a NULL method I will accept this and clean it up later.

mjsteinbaugh commented 4 years ago

I think you're right that it may be better to keep it as is to support S4 numerics like Rle, in case the matrices start to get really large or have repetitive size factor values. I'll close this for now -- it was just something I came across when unit testing my code and figured we could tighten up a bit. Thanks!

LTLA commented 4 years ago

I'm still up for this, I don't really think that anyone will be storing size factors as anything other than numeric vectors.