search

PeteHaitch / DelayedMatrixStats

A port of the matrixStats API to work with DelayedMatrix objects from the DelayedArray package
Other
15 stars 7 forks source link

Replaced all generic definitions with those from MatrixGenerics. #62

Closed LTLA closed 4 years ago

LTLA commented 4 years ago

There is still an issue with some generics defined in both DelayedArray and MatrixGenerics, e.g., rowMins. Seems like former should depend on the latter to ensure that we're writing methods for the same generic.

PeteHaitch commented 4 years ago

Thanks, @LTLA. I'm sure it's all fine but I'll take a closer look ASAP. @hpages what should we do with the generics defined in both DelayedArray and MatrixGenerics? Is the rowRanges one particularly tricky (given it has a very different meaning in SummarizedExperiment)?

LTLA commented 4 years ago

Seems to make sense to have DelayedArray relinquish rowMins, rowMaxs and rowRanges, and for those methods to move into this package instead. This would mean that DelayedArray doesn't have to import MatrixGenerics and the methods would live in the expected place, i.e., DelayedMatrixStats.

PeteHaitch commented 4 years ago

@hpages how would you like to handle this? I know you're busy coming up to a new release

hpages commented 4 years ago

FWIW I've started to implement stat methods for SparseArraySeed objects in an effort to speed up block processing of sparse DelayedArray objects. I'm considering implementing MatrixGenerics methods for SparseArraySeed objects soon so will need to import MatrixGenerics in DelayedArray. Will try to get to this today.

P.S.: As you are already aware I'm defining my own row/colSums(), row/colMeans(), row/colMins(), row/colMaxs(), and row/colRanges() methods for DelayedMatrix objects, which are redundant with the methods defined in DelayedMatrixStats, and get replaced when DelayedMatrixStats is loaded. This is temporary and for convenience only. Having them in DelayedArray just makes it easier for me to experiment with various strategies to optimize them. When I'm finally happy with what I have, I'll move them to DelayedMatrixStats and apply the same strategy (or similar) to the other methods in DelayedMatrixStats.

hpages commented 4 years ago

Done: https://github.com/Bioconductor/DelayedArray/commit/00c96df93e4d05b541d8c36161290390e2e69402

hpages commented 4 years ago

One last thing: yes rowRanges is tricky. Had to make the following change to MatrixGenerics to accomodate the rowRanges() getter for RangedSummarizedExperiment. Ugly!

LTLA commented 4 years ago

I don't know what happened to the CI here, but this passes R CMD CHECK for me.

PeteHaitch commented 4 years ago

Should the disabled tests in e4b1bd6 be reverted if there are changes upstream?