-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add slice functions #90
Conversation
R/dplyr_methods.R
Outdated
stop("tidySingleCellExperiment says:", | ||
" the resulting data container is empty.", | ||
" Seurat does not allow for empty containers.") | ||
} |
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.
Look for "Seurat" in all files, and please replace ;)
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 believe an empty SCE is completely fine. Not sure this check is needed.
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.
Thank you very much for your prompt review. I apologize that the part of the message remained the same as tidyseurat
. Also, to accept the empty SCE, all the checks for zero length ID in the slice functions are removed.
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.
Thanks! Can I please ask to add the 0-length SCE to the uni tests? As we start with our standard SCE and we slice to result to 0 rows, and see what happens (within the unit tests)
Thank you for the guidance. I have added the tests to expect
|
Great. If this warning comes from |
Thank you for the review. I have committed the changes to overcome the warning in |
.max_cell_count <- ifelse(nrow(count_cells)==0, 0, max(count_cells$n)) | ||
|
||
# If repeated cells due to replacement | ||
if (count_cells$n |> max() |> gt(1)){ | ||
if (.max_cell_count |> gt(1)){ |
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.
looks good, does the warning appear in tidyseurat
as well?
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.
Thank you for checking. Seems to be yes, although it results in an error. Should I also replace the part for tidyseurat
?
> pbmc_small |> slice_sample(n=0)
Error: No cells found
In addition: Warning message:
In max(count_cells$n) : no non-missing arguments to max; returning -Inf
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.
@noriakis Yes, please fix the error in tidyseurat
and write a unit test for it.
If this is relative to an empty object I thought you had a stop()
call already.
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.
Thank you for your reply. The error is related to an empty object coming from stop()
call in subset.Seurat()
as you pointed out, so I will just fix the warnings.
Hello, I added the following functions to
dplyr_methods.R
. I would be grateful if you could review the changes.slice_head()
slice_tail()
slice_sample()
slice_min()
slice_max()