Bioconductor / MatrixGenerics

S4 Generic Summary Statistic Functions that Operate on Matrix-Like Objects
https://bioconductor.org/packages/MatrixGenerics
12 stars 1 forks source link

Implement useful fallback method for colVars() #13

Closed hpages closed 3 years ago

hpages commented 4 years ago

Hi @const-ae, @PeteHaitch, @LTLA, @lawremi,

This improves a little bit upon my earlier proposal on the community-bioc slack this morning (bigdata-rep channel) of loading all the suggested packages the first time a MatrixGenerics' generic is called (e.g. the first time MatrixGenerics::colVars() is called). With this new approach, if I'm passing a dgCMatrix object and if sparseMatrixStats is already loaded, the fallback mechanism won't try to load anything. So this addresses Nicholas Knoblauch's concern that there is no reason to throw an error that DelayedMatrixStats is not installed in that case.

library(MatrixGenerics)
library(Matrix)
sm <- as(matrix(1:12, ncol=3), "sparseMatrix")

Without fallback mechanism:

rowVars(sm)
# Error in (function (classes, fdef, mtable)  : 
#   unable to find an inherited method for function ‘rowVars’ for signature ‘"dgCMatrix"’

With the fallback mechanism:

If sparseMatrixStats is installed, the fallback mechanism loads it so dispatch can find the method:

colVars(sm)
# [1] 1.666667 1.666667 1.666667

If sparseMatrixStats and "other suggested packages to search" (e.g. DelayedMatrixstats) are not installed we get:

colVars(sm)
# Error in load_next_suggested_package_to_search("colVars()", x) : 
#   Failed to find a colVars() method for dgCMatrix objects.
#   However, the following packages are likely to contain the missing method
#   but are not installed: sparseMatrixStats, DelayedMatrixStats
#   Please install them (with 'BiocManager::install()') and try again.
#   Alternatively, if you know where the missing method is defined, install
#   only that package.

If sparseMatrixStats is the only "other suggested packages to search" that is not installed we get:

colVars(sm)
# Error in load_next_suggested_package_to_search("colVars()", x) : 
#   Failed to find a colVars() method for dgCMatrix objects.
#   However, the following package is likely to contain the missing method
#   but is not installed: sparseMatrixStats
#   Please install it (with 'BiocManager::install()') and try again.

Note that this approach still doesn't guarantee that users will always have all the packages that define every possible methods for the MatrixGenerics' generics, so is complementary to Aaron's approach of using beachmat or scuttle as an umbrella package for the sc-stack that he controls. But at least it provides a reasonable safety net for packages that don't belong to this stack. They need to import MatrixGenerics only.

Open for discussion.

H.

const-ae commented 4 years ago

I like the explicit error messages that tell the users what they need to do, and agree that this is useful regardless if there is an umbrella package that loads MatrixGenerics, DelayedMatrixStats and sparseMatrixStats.

hpages commented 4 years ago

I'll try to come up with a full patch next week.

PeteHaitch commented 4 years ago

Agreed, this looks useful.