Skip to content
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

Update umi.assay in GetResidual #8203

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

aladyeva-research
Copy link

The current implementation of GetResidual in PrepSCTIntegration does not allow applying it to Spatial datasets as they don't contain "RNA" assay which is set as default value of parameter umi.assay.
This fix change default umi.assay to NULL and in the further check picks RNA or Spatial values, depending on which is presented in the object. If both are presented it defaults to the first value.
Also allows easy to extend of any future default assay values.

Note

Thanks for your interest in contributing! Please make your PR to the develop branch. You can check out our wiki for the current developers guide.

The current implementation of GetResidual in PrepSCTIntegration does not allow applying it to Spatial datasets as they don't contain "RNA" assay which is set as default value of parameter umi.assay.
@saketkc
Copy link
Collaborator

saketkc commented Dec 19, 2023

Hi @aladyeva-wustl, thanks for the PR! Our recommendation would be to use umi.assay="Spatial" when invoking GetResiduals. The default of RNA is to cater to most frequent use case (RNA -> SCTransform). While your solution is good, it would not be helpful if the default layer is anything except RNA or Spatial (and would anyway require the user to specify the parameter explicitly)

@saketkc saketkc closed this Dec 19, 2023
@aladyeva-research
Copy link
Author

Hi @saketkc we can't explicitly specify umi.assay, because we are running PrepSCTIntegration function and there are no such parameters that are passed to GetResiduals.
My decision to add both default values to GetResiduals instead of PrepSCTIntegration is motived by idea that if umi.assay is missing in other functions they will also not work for Spatial transcriptomics data.

@saketkc
Copy link
Collaborator

saketkc commented Dec 19, 2023

I see, that is a valid point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants