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

Implement Decipher model in external #3015

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Implement Decipher model in external #3015

wants to merge 43 commits into from

Conversation

justjhong
Copy link
Contributor

CC @ANazaret

Implements Decipher model (https://github.com/azizilab/decipher, https://www.biorxiv.org/content/10.1101/2023.11.11.566719v1) into external/

For now, it only includes base implementation without many of the downstream workflows from the original implementation.
Includes minor non-breaking changes to the LowLevelPyroTrainingPlan.

Test: was able to approximately reproduce figures from the tutorial (https://github.com/azizilab/decipher/blob/main/examples/1-tutorial.ipynb), some of the v plots for several random seeds below:

Original implementation:
decipher_orig_0
decipher_orig_1030

New implementation:
decipher_tutorial_scvi_0
decipher_tutorial_scvi_1
decipher_tutorial_scvi_2
decipher_tutorial_scvi_3

@justjhong justjhong added the cuda tests Run test suite on CUDA label Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 96.67774% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.61%. Comparing base (38e1d65) to head (e25173b).

Files with missing lines Patch % Lines
src/scvi/external/decipher/_components.py 89.36% 5 Missing ⚠️
src/scvi/external/decipher/_module.py 97.34% 3 Missing ⚠️
src/scvi/external/decipher/_trainingplan.py 97.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3015      +/-   ##
==========================================
- Coverage   84.80%   84.61%   -0.20%     
==========================================
  Files         173      178       +5     
  Lines       14797    15107     +310     
==========================================
+ Hits        12549    12783     +234     
- Misses       2248     2324      +76     
Files with missing lines Coverage Δ
src/scvi/external/__init__.py 100.00% <100.00%> (ø)
src/scvi/external/decipher/__init__.py 100.00% <100.00%> (ø)
src/scvi/external/decipher/_model.py 100.00% <100.00%> (ø)
src/scvi/external/decipher/_trainingplan.py 97.10% <97.10%> (ø)
src/scvi/external/decipher/_module.py 97.54% <97.34%> (ø)
src/scvi/external/decipher/_components.py 89.36% <89.36%> (ø)

... and 1 file with indirect coverage changes

@justjhong
Copy link
Contributor Author

Not sure why the cuda tests are failing on import of the Decipher class. Any ideas? @canergen @ori-kron-wis

@ori-kron-wis
Copy link
Collaborator

ori-kron-wis commented Oct 20, 2024

@justjhong I updated the main branch with the updated cuda test file we need now to run on WIS servers and merged this PR with it. Now the tests work.
However, test_decipher is failing. Not sure why, I can only say it is passing in a non-CUDA environment, and failing in both single and multi GPU environments. Can it be related to the fact that it is Pyro based?

@justjhong
Copy link
Contributor Author

@ori-kron-wis thanks for your help confirming its a real bug. I will debug this on my local CUDA machine and update the PR accordingly

@justjhong justjhong added the on-merge: backport to 1.2.x on-merge: backport to 1.2.x label Oct 21, 2024
@justjhong
Copy link
Contributor Author

@ori-kron-wis I implemented the fix which passes local CUDA tests, but receiving this import error once again. How were you able to bypass this error the previous time? Otherwise, I've prepped the PR with the corresponding release note and docs for the next release. Thank you!

@ori-kron-wis
Copy link
Collaborator

ori-kron-wis commented Oct 21, 2024

you are right, it wasn't solved.
I cleared the runner cache locally on the server + restarted it and now it looks ok again. one of those voodoo things ;-)

@justjhong
Copy link
Contributor Author

@ori-kron-wis Haha these flaky things are never satisfying but thank you for finding the fix! I will add decipher to the docs first before asking you for the final review.

Aside: do you think there should be a simple PR checklist for the standard things for typical contributions? I keep forgetting things (e.g., CHANGELOG, function level documentation, add classes to docs if necessary, add backport label if needed before merge)

@justjhong
Copy link
Contributor Author

@ori-kron-wis once checks pass, this PR should be ready for review and merge 👍

@justjhong
Copy link
Contributor Author

@ori-kron-wis cuda tests failing after merging with main. do you mind taking a look before the pr gets out of date again?

@ori-kron-wis
Copy link
Collaborator

@justjhong yeah , had to do major updates on the servers. hope that it will be fine now.

@justjhong
Copy link
Contributor Author

Thanks @ori-kron-wis ! if everything looks can you approve the PR? @canergen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda tests Run test suite on CUDA on-merge: backport to 1.2.x on-merge: backport to 1.2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants