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
Enable FastMNN #246
Enable FastMNN #246
Changes from 5 commits
4b6f2ed
10b0c7f
ecfadd9
452d5b0
738b707
cc6f3a5
b1540f5
86fb84d
f3c96b8
942f6d8
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.
Sounds like a nice message, but it will be better if you are more explicit and say
Cellscope uses "sctransform".' after sentence
There are several methods to achive normalization.`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.
also a typo in
achive
-- it isachieve
and there should be a comma just afterfactors
on the first line (I think :D ). It would read better if you rephrased the first sentence toNormalization aims to remove technical factors, like sequencing depth for 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.
Also, it is worth to point out that I personally have no idea what
technical factors
mean. That can just as well be because of my lack of biology/bioinformatics background. I am noting it, because if biologists might not know this, we will need to rephrase it. @vickymorrison any thoughts?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 for the review @ivababukova! For this PR I looked at what was already in place in SeuratV4Options to create the counterpart to FastMNN and Unisample. In the new commit Martin has done a great job of refactoring. Regarding
technical errors
, I guess it refers to possible biases that occur when sequencing or in the lab in the sample preparation.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.
For the first sentence, how about: "Normalization aims to remove technical variation that is not biologically relevant, e.g. sequencing depth." ?
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 like it
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 thing is that we could add as covariates for the normalization the mt-content, so I guess we should not especify biologically relevant. For the current version we cannot add covariates but I think that we will support it.
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 would argue that 'not biologically relevant' still stands - if the user chooses to exclude specific gene categories that is because they believe those genes are not biologically relevant to the question they are trying to answer.
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 makes sense! I'll change!