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

Add check for number of normalized dispersions #2231

Merged
merged 10 commits into from
Aug 12, 2022
Merged

Conversation

lazappi
Copy link
Contributor

@lazappi lazappi commented Apr 11, 2022

In sc.pp.highly_variable_genes() when flavor='cell_ranger' and n_top_genes is set check that enough normalized dispersions have been calculated and if not raise a warning and set n_top_genes to the number of calculated dispersions.

Fixes #2230

In sc.pp.highly_variable_genes() when flavor='cell_ranger' and
n_top_genes is set check that enough normalized dispersions have been
calculated and if not raise a warning and set n_top_genes to the number
of calculated dispersions.

Fixes scverse#2230
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #2231 (b1b0c31) into master (4623bb7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head b1b0c31 differs from pull request most recent head 29c5505. Consider uploading reports for the commit 29c5505 to get more accurate results

@@            Coverage Diff             @@
##           master    #2231      +/-   ##
==========================================
+ Coverage   71.82%   71.85%   +0.02%     
==========================================
  Files          98       98              
  Lines       11539    11542       +3     
==========================================
+ Hits         8288     8293       +5     
+ Misses       3251     3249       -2     
Impacted Files Coverage Δ
scanpy/preprocessing/_highly_variable_genes.py 95.38% <100.00%> (+1.11%) ⬆️

@lazappi
Copy link
Contributor Author

lazappi commented Apr 11, 2022

Just noticed this is almost an exact duplicate of #1985 (check the issues but not PRs 🤦🏻 ). Feel free to close this if you want to keep the older one open.

@Zethson
Copy link
Member

Zethson commented May 4, 2022

@lazappi you are now the chosen one :)

Think that using the size attribute like in https://github.com/scverse/scanpy/pull/1985/files is maybe nicer and more explicit. len() could technically be overwritten and return anything. It's less explicit.

Could you maybe add a quick test which covers this case?

Thank you very much!

@lazappi
Copy link
Contributor Author

lazappi commented May 5, 2022

Made the .size change and added a test. Not entirely sure I have done the test correctly so let me know if that needs adjusting.

* upstream/master:
  Add link to scverse Discourse for README badge (scverse#2240)
  [pre-commit.ci] pre-commit autoupdate (scverse#2232)
@lazappi
Copy link
Contributor Author

lazappi commented May 27, 2022

Sorry, I forgot about this. Not sure why the CI was falling. The only errors I get locally were due to problems with matching images.

@Zethson
Copy link
Member

Zethson commented May 27, 2022

@lazappi no worries! The scanpy CI has always been a little bit flaky and sometimes failures are not the faults of people submitting pull requests. If it fails again, we can dig deeper.

@lazappi
Copy link
Contributor Author

lazappi commented May 30, 2022

It looks like the same AssertionError: Error: Image files did not match. error I was getting locally from some of the spatial tests. I haven't touched this so not sure what's going on there.

@Zethson
Copy link
Member

Zethson commented Jun 17, 2022

@ivirshup this one should be good to go now, right? Do you require release notes for such small things or do you manually add those later?

@ivirshup
Copy link
Member

@Zethson, have you reviewed?

@ivirshup
Copy link
Member

And yes, should get a bug fix release note.

* origin/fix-2230:
  [pre-commit.ci] pre-commit autoupdate (scverse#2271)
  Update CellRank's metion in the ecosystem (scverse#2269)
  fix typo in log1p parameters (scverse#2273)
  Fix error formatting (scverse#2263)
  Fix tests (scverse#2274)
* origin/fix-2230:
  Fix using custom layer with highly_variable_genes (scverse#2302)
  [pre-commit.ci] pre-commit autoupdate (scverse#2303)
  [pre-commit.ci] pre-commit autoupdate (scverse#2289)
@Zethson Zethson enabled auto-merge (squash) August 12, 2022 09:21
@Zethson Zethson merged commit 1844b91 into scverse:master Aug 12, 2022
ivirshup added a commit to meeseeksmachine/scanpy that referenced this pull request Feb 16, 2023
* Add check for number of normalized dispersions

In sc.pp.highly_variable_genes() when flavor='cell_ranger' and
n_top_genes is set check that enough normalized dispersions have been
calculated and if not raise a warning and set n_top_genes to the number
of calculated dispersions.

Fixes scverse#2230

* Use .size instead of len()

* Add test for n_top_genes warning

* Add release note

* Remove blank line

Co-authored-by: Isaac Virshup <ivirshup@gmail.com>
ivirshup added a commit that referenced this pull request Feb 16, 2023
* Backport PR #2414: matplotlib 3.7 compat

* fix scrublet

* Update visium default plot for matplotlib 3.7

* Update hashsolo docstrings

* skip plotting test that changed on mpl 3.7 if mpl < 3.7 is installed

* Fix hashsolo docs (again)

* update anndata-dev tests to install anndata test deps

* Temporarily set warnings as errors to False for doc builds

* Release notes

* Fix using custom layer with highly_variable_genes (#2302)

* Fix using custom layer with highly_variable_genes

* Add tests

* Add release note

* Move release note to correct section

* Format release notes

* Add check for number of normalized dispersions (#2231)

* Add check for number of normalized dispersions

In sc.pp.highly_variable_genes() when flavor='cell_ranger' and
n_top_genes is set check that enough normalized dispersions have been
calculated and if not raise a warning and set n_top_genes to the number
of calculated dispersions.

Fixes #2230

* Use .size instead of len()

* Add test for n_top_genes warning

* Add release note

* Remove blank line

Co-authored-by: Isaac Virshup <ivirshup@gmail.com>

---------

Co-authored-by: Isaac Virshup <ivirshup@gmail.com>
Co-authored-by: adamgayoso <adamgayoso@users.noreply.github.com>
Co-authored-by: Dries Schaumont <5946712+DriesSchaumont@users.noreply.github.com>
Co-authored-by: Luke Zappia <lazappi@users.noreply.github.com>
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.

highly_variable_genes() with flavor="cell_ranger" fails there are less normalized dispersions than n_top_genes
3 participants