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

Layers support for PCA and regress_out #2588

Merged
merged 6 commits into from Aug 4, 2023

Conversation

Intron7
Copy link
Member

@Intron7 Intron7 commented Aug 3, 2023

This PR adds .layers support for PCA and regress_out.

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #2588 (e041f3e) into master (89804c2) will not change coverage.
The diff coverage is 100.00%.

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2588   +/-   ##
=======================================
  Coverage   72.12%   72.12%           
=======================================
  Files         104      104           
  Lines       11688    11688           
=======================================
  Hits         8430     8430           
  Misses       3258     3258           
Files Changed Coverage Δ
scanpy/preprocessing/_pca.py 95.23% <100.00%> (+0.03%) ⬆️
scanpy/preprocessing/_simple.py 80.04% <100.00%> (-0.05%) ⬇️

Comment on lines 170 to 175
assert np.allclose(X_pca.uns["pca"]["variance"], layer_pca.uns["pca"]["variance"])
assert np.allclose(
X_pca.uns["pca"]["variance_ratio"], layer_pca.uns["pca"]["variance_ratio"]
)
assert np.allclose(X_pca.obsm['X_pca'], layer_pca.obsm['X_pca'])
assert np.allclose(X_pca.varm['PCs'], layer_pca.varm['PCs'])
Copy link
Member

Choose a reason for hiding this comment

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

These should be exactly equal, right?

Copy link
Member

Choose a reason for hiding this comment

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

Also maybe checkout scanpy.testing._helpers.check_rep_results

@ivirshup ivirshup changed the title Layers support Layers support for PCA Aug 3, 2023
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.

Two very minor changes:

  • Could the layer be recorded in the parameters saved to uns?
  • Could you please add a release note?

@ivirshup ivirshup changed the title Layers support for PCA Layers support for PCA and regress_out Aug 3, 2023
@ivirshup ivirshup self-requested a review August 4, 2023 09:10
@ivirshup ivirshup enabled auto-merge (squash) August 4, 2023 09:11
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.

Updated

@ivirshup ivirshup merged commit 54c3470 into scverse:master Aug 4, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants