-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed the sparse matrix issue #89
base: master
Are you sure you want to change the base?
Conversation
This reverts commit abb4e0a.
Thanks, and sorry for a late reply, I will need to test this first and then merge. |
@@ -149,7 +149,7 @@ sc3_prepare.SingleCellExperiment <- function(object, gene_filter, pct_dropout_mi | |||
f_data <- rowData(object) | |||
f_data$sc3_gene_filter <- TRUE | |||
if (gene_filter) { | |||
dropouts <- rowSums(counts(object) == 0)/ncol(object)*100 | |||
dropouts <- Matrix::rowSums(counts(object) == 0)/ncol(object)*100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware of the discussion in #53 , but I am still not sure if hardcoding it this way without checking the type of counts(object)
is a good idea. For hdf5-backed object, the returned matrix can be of class "DelayedMatrix", which is not handled correctly by Matrix::rowSums(), just as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for the comment! That's why I didn't rush to merge it, the issue is quite big and requires some time to think and decide.
Issue was discussed here:
This is a temp fix. The correlation function
cor
andED2
also need regular matrix as input, and converting the sparse matrix to regular isn't memory efficient.