Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feature: updates for cellranger 9.0 #9541
base: develop
Are you sure you want to change the base?
feature: updates for cellranger 9.0 #9541
Changes from 2 commits
3038586
c36bb1c
1d575a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
(Preemptive-- this parameter doesn't seem to be implemented yet)
In my opinion, it would be better not to suggest that blind uppercasing is ever an appropriate way of finding mouse-human orthologs, even in an exploratory analysis.
If the user has some good reason to convert all the symbols to uppercase, they can simply use
toupper
and replace the rownames of the object.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.
This option was put in originally by the seurat team. I'll let them address the usefulness here.
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.
The parameter was introduced well before my time but I tend to agree with @rharao.
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.
Nit: because this formula operates on the counts matrices only and doesn't use the list's names, I think the
imap
call could be replaced bybase::sapply
.Frankly, I'm not sure why this package imports
purrr::imap
anyway, as I can't find it used anywhere in the codebase, not even inLoad10X_Spatial
. But that's an issue for another time.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.
personally, I like the simplicity and readability of tidy functions. I'll leave it up to the seruat developers to decide if they mind the extra import. I haven't heard any complaints since we wrote this function a few years ago.
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.
It's not really an issue; the choice one way or another doesn't affect me. (Personally, I agree; I always prefer the purrr functions in my own scripts.) I only wanted to mention it for the attention of the dev team.
Thanks for your receptiveness 😄
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.
+1 to replacing the
imap
call with asapply
to match the current implementation ofLoad10X_Spatial
.This is also a good point,
@importFrom purrr imap
should be dropped fromLoad10X_Spatial
and as a suggested dependency. Now that you mention it,@importFrom grid rasterGrob
could also be dropped from the function. I'll include these tweaks in #9556 👌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.
should be
seq_along(seurat.list)
.Alternatively, you could replace the for loop with:
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 did also notice that if
Add_10X_CellTypes
doesn't find a cell types csv, it returns the original without warning or error, so if you want to rely on that, you could skip the conditional here.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'm not sure that
is.list(counts)
conditional is necessary. Instead, you can just forcecounts
to always be a list, and then you can do without theelse
block.It looks like there's just one
CellTypes
file perdata.dir
? If that's the case, this could be simplified even more—the call toAdd_10X_CellTypes
could be made on themerged.object
.