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

Fix PercentageFeatureSet #8659

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

maxim-h
Copy link

@maxim-h maxim-h commented Mar 20, 2024

Fixes #8658

@samuel-marsh
Copy link
Collaborator

Hi,

Not member of dev team but just a thought here. A check should also probably be added to prevent integrated assay from being selected here because as you mentioned it doesn't have counts and shouldn't be used, would just provide more informative error message to end user.

Best,
Sam

@maxim-h
Copy link
Author

maxim-h commented Mar 20, 2024

Hi Sam.

It is a good point. As of now it runs without errors, but just returns NULL. So it was really hard to understand what the problem was without looking at the code.

I wouldn't be confident throwing errors from functions that currently return. Might mess up something elsewhere.
But adding a warning message might be useful. Only it probably should be coming from the Layers function and not PercentageFeatureSet. So it needs to be a pull request to SeuratObject.

maxim-h added a commit to maxim-h/seurat-object that referenced this pull request Mar 20, 2024
This is meant to help diagnose issues when assay doesn't have desired layer. 
For example see satijalab/seurat#8658 and discussion in satijalab/seurat#8659.

The warning message might not be the prettiest, but I didn't want to go through `Layer.Seurat` method to get the assay name.
@samuel-marsh
Copy link
Collaborator

I'll leave up to dev team to decide but personally I think abort in percentagefeatureset makes more sense to me. End users may still end up confused (for those that don't understand integration doesn't have corrected counts) as to why function fails. And a warning about integrated assay not being appropriate for this particular function makes sense. But either way works.

Best,
Sam

mojaveazure pushed a commit to maxim-h/seurat-object that referenced this pull request May 3, 2024
This is meant to help diagnose issues when assay doesn't have desired layer. 
For example see satijalab/seurat#8658 and discussion in satijalab/seurat#8659.

The warning message might not be the prettiest, but I didn't want to go through `Layer.Seurat` method to get the assay name.
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