[r] add row/col max stat functions #103
Conversation
r/R/matrix.R
Outdated
| #' Get the max of each row in an iterable matrix | ||
| #' @param x IterableMatrix object/dgCMatrix object | ||
| #' @export | ||
| row_max <- function(x) { |
There was a problem hiding this comment.
For naming clarity, I think we'll want to do a similar approach as rowVars()/colVars() below, namely:
- Define a
rowMaxsgeneric function with the same arguments asmatrixStats::rowMaxs - Define
rowMaxs.defaultto call out tomatrixStatsormatrixGenericsif present - Put our actual implementaiton in
rowMaxs.IterableMatrix - Call
rlang::on_loadto register the generic at load time if the other libraries are present
This will make it so our users can use rowMaxs() mostly without worry. (If they call matrixStats::rowMaxs() directly it won't work for BPCells objects, but any other copy of rowMaxs they call will support the full range of matrix types)
There was a problem hiding this comment.
I think this makes interfacing with BPCells a lot more elegant, however for the future I wonder if doing this for each function will end up creating a lot of bloat. Maybe we could create a function to automatically perform this strategy, given the default libraries with the same function names?
There was a problem hiding this comment.
That is a good idea! I'm not quite sure about how to programmatically handle defining functions with a given set of arguments, but it's probably something we could figure out with the Advanced R book.
immanuelazn
force-pushed
the
max-by-row-col
branch
from
2c0564c to
6865649
Compare
- Moved code to sit next to the colVars functions - Added an rlang::on_load wrap and updated the docstring for each function - Also deleted the header line `Get the max of each row in an iterable matrix` as that rendered badlly in the docs - Ran `devtools::document()` to re-generate the docs files (these live in the `man` folder and power the built-in R docs system e.g. running `?BPCells::rowMaxs` in R terminal) - This also updates the `NAMESPACE` file so the functions will be exported properly - Reduce to just the comprehensive test - Switch from exect_equal to expect_identical (more stringent since we don't have any precision issues to worry about) - Make the sparse matrix test include both positive + negative, and expand size so we still get good coverage with the new possibilities - Add a 4th transpose case to cover all possibilities of transpose before (yes/no) and transpose after (yes/no) - Fix a typo in NEWS and add a shoutout for your first contribution!
Description
Added row max and col max standalone rudimentary functions for iterable matrices. Can handle transposed matrices.
Tests
Tested against passing the non-iterable matrix version through MatrixStats and asserting equality
Thoughts
Would it make more sense to make this a method within IterableMatrix rather than a standalone function? I see some functions in Matrix.R that aren't methods for their specific matrix class, but instead assert typing. Not sure if that is the correct route for me to take though.