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 layer handling in Mixscape.mixscape to resolve errors with adata.raw #636

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

Lilly-May
Copy link
Collaborator

PR Checklist

Description of changes

  • Added use_raw=False to the sc.tl.rank_genes_groups method call
  • Currently, Mixscape.mixscape fails for adata objects where adata.raw is present. This is due to the internal call of sc.tl.rank_genes_groups, which uses adata.raw by default for computation if use_raw is not explicitly set to False. If a layer is specified when calling Mixscape.mixscape, this will cause the rank_genes_groups method to fail with the error: ValueError: Cannot specify "layer" and have "use_raw=True". If no layer is specified, rank_genes_groups will use adata.raw for computation, potentially leading to errors like the one reported in Error while using ms.mixscape - ValueError: Input X contains NaN. #617.

Discussion point

  • Currently, the layer is set to X_pert by default only after sc.tl.rank_genes_groups is called, meaning it will run on X unless the user provides a different layer. Should we change this? On what basis should the perturbation markers be calculated? Rather based on the X_pert layer?

@github-actions github-actions bot added the bug Something isn't working label Aug 1, 2024
@Zethson
Copy link
Member

Zethson commented Aug 2, 2024

Thank you!

Currently, the layer is set to X_pert by default only after sc.tl.rank_genes_groups is called, meaning it will run on X unless the user provides a different layer. Should we change this? On what basis should the perturbation markers be calculated? Rather based on the X_pert layer?

The answer should be somewhere here: https://satijalab.org/seurat/articles/mixscape_vignette.html

Yeah, I think it should be called on X_pert but I thought there was even some code that set X to X_pert?

@Zethson
Copy link
Member

Zethson commented Aug 3, 2024

Merging this now but please see my comment. We can discuss this further (on slack).

@Zethson Zethson merged commit c7f7584 into main Aug 3, 2024
3 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants