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 use_raw=None in scanpy.tl.score_genes #1999

Merged
merged 6 commits into from Oct 27, 2021

Conversation

michalk8
Copy link
Contributor

This PR fixes the case when use_raw=None in scanpy.tl.score_genes. It causes to first fetch var_names from adata.var_names, but later a subset on adata.raw can happen, which can have different gene names.
Also fixes the type of use_raw and adds a ValueError if gene_pool is empty (otherwise, crashes with non-informative error message).

related issue: theislab/cellrank#746

@codecov
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #1999 (046c34e) into master (5acf8cb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 046c34e differs from pull request most recent head de7e7ca. Consider uploading reports for the commit de7e7ca to get more accurate results

@@            Coverage Diff             @@
##           master    #1999      +/-   ##
==========================================
+ Coverage   71.63%   71.64%   +0.01%     
==========================================
  Files          92       92              
  Lines       11248    11250       +2     
==========================================
+ Hits         8057     8060       +3     
+ Misses       3191     3190       -1     
Impacted Files Coverage Δ
scanpy/tools/_score_genes.py 85.71% <100.00%> (+1.56%) ⬆️

@ivirshup ivirshup added this to the 1.8.2 milestone Oct 27, 2021
Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just needs a release note

@ivirshup
Copy link
Member

Could the note be a little more descriptive? Without a specific issue linked to this it's hard to tell what incorrect behavior was changed.

@ivirshup ivirshup enabled auto-merge (squash) October 27, 2021 15:51
@ivirshup
Copy link
Member

ivirshup commented Oct 27, 2021

Great, thanks!

We do have the CI checks as required for merging, unfortunately.

Question for you about the pre-commit CI. Do you know what's going on here with the extra service? Do we no longer need the github actions pre-commit check? Can also figure this out on Friday.

@ivirshup
Copy link
Member

Heads up, I'm going to make an empty commit to re-trigger the CI.

@ivirshup ivirshup merged commit 8c07642 into scverse:master Oct 27, 2021
meeseeksmachine pushed a commit to meeseeksmachine/scanpy that referenced this pull request Oct 27, 2021
ivirshup pushed a commit that referenced this pull request Oct 27, 2021
Co-authored-by: michalk8 <46717574+michalk8@users.noreply.github.com>
@michalk8
Copy link
Contributor Author

We do have the CI checks as required for merging, unfortunately.

Ok, didn't wanted to waste the resources and thought you can just merge manually even if checks don't pass.

Question for you about the pre-commit CI. Do you know what's going on here with the extra service? Do we no longer need the github actions pre-commit check? Can also figure this out on Friday.

Yes, afaik this was enabled a few weeks back for the whole organization.

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