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.

Sign up for GitHub

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

Jump to bottom

Fixes error in get losses functions #2362

Merged
merged 34 commits into from
Jul 30, 2024
Merged

Fixes error in get losses functions #2362

merged 34 commits into from
Jul 30, 2024

Conversation

canergen
Copy link
Member

@canergen canergen commented Dec 8, 2023 •
edited by ori-kron-wis
Loading

  • Closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed
  • Added type annotations to new arguments/methods/functions
  • Added an entry in the latest docs/release_notes/index.md file if fixing a bug or adding a new feature
  • If the changes are patches for a version, I have added the on-merge: backport to x.x.x label

@codecov Codecov
Copy link

codecov bot commented Dec 8, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.29%. Comparing base (9137c05) to head (285f6be).

Files Patch % Lines
src/scvi/model/base/_log_likelihood.py 88.88% 3 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2362   +/-   ##
=======================================
  Coverage   85.29%   85.29%           
=======================================
  Files         167      167           
  Lines       14346    14364   +18     
=======================================
+ Hits        12236    12252   +16     
- Misses       2110     2112    +2
Files Coverage Δ
src/scvi/external/scar/_module.py 87.38% <100.00%> (ø)
src/scvi/model/base/_vaemixin.py 93.75% <100.00%> (ø)
src/scvi/module/_autozivae.py 80.13% <100.00%> (ø)
src/scvi/module/_scanvae.py 88.18% <100.00%> (ø)
src/scvi/module/_totalvae.py 87.19% <100.00%> (ø)
src/scvi/module/_vae.py 95.61% <100.00%> (+0.03%) ⬆️
src/scvi/model/base/_log_likelihood.py 91.89% <88.88%> (-3.35%) ⬇️

@canergen canergen added the on-merge: backport to 1.1.x on-merge: backport to 1.1.x label Dec 12, 2023
martinkim0
martinkim0 reviewed Dec 13, 2023
View reviewed changes
scvi/module/_vae.py Outdated
Comment on lines 612 to 613
if n_mc_samples_per_pass == 1:
log_prob_sum = log_prob_sum.unsqueeze(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait so does this mean that this method was not working properly before? Since the default is n_mc_samples_per_pass=1

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but we only use it for DE genes where it's called with n_samples_per_mc of 100 (?). ScANVI wasn't supporting importance weighting in DE beforehand.
ScANVI doesn't work with multiple samples (major work to fix this) and this might be an issue for multi-GPU support (see bug report). If you want to fix this, I have a fix for broadcast labels but then dropped it as also the encoder doesn't support it.

martinkim0
martinkim0 reviewed Dec 13, 2023
View reviewed changes
scvi/model/base/_log_likelihood.py Outdated
elbo += kl_global
return elbo / n_samples
else:
return elbo + kl_global / n_samples
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to also use torch operations here since compute_reconstruction_error doesn't use numpy. Let me know what you think

Copy link
Member Author

@canergen canergen Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fine with me. Can you puh those changes?

@martinkim0 martinkim0 added this to the scvi-tools 1.1.0 milestone Jan 22, 2024
@martinkim0
Copy link
Contributor

#2429 needs to be addressed before this

@martinkim0 martinkim0 removed the on-merge: backport to 1.1.x on-merge: backport to 1.1.x label Jan 29, 2024
@martinkim0 martinkim0 modified the milestones: scvi-tools 1.1.1, scvi-tools 1.1.x Feb 28, 2024
@martinkim0 martinkim0 removed this from the scvi-tools 1.1.x milestone Jun 21, 2024
@canergen
Copy link
Member Author

@ori-kron-wis We can merge this one after fixing the n_samples issue.

@canergen canergen added on-merge: backport to 1.2.x on-merge: backport to 1.2.x cuda tests Run test suite on CUDA labels Jul 30, 2024
@canergen canergen removed the cuda tests Run test suite on CUDA label Jul 30, 2024
@canergen canergen merged commit 6c458b3 into main Jul 30, 2024
11 of 12 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/scvi-tools that referenced this pull request Jul 30, 2024
@canergen @meeseeksmachine
Backport PR scverse#2362: Fixes error in get losses functions
70c1a82
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 30, 2024
Merged
canergen added a commit that referenced this pull request Jul 30, 2024
@meeseeksmachine @canergen
Backport PR #2362 on branch 1.2.x (Fixes error in get losses function…
0b4c01b 
…s) (#2921)

Backport PR #2362: Fixes error in get losses functions

Co-authored-by: Can Ergen <canergen.ac@gmail.com>
@canergen canergen deleted the can-expose_mc_samples_de branch July 30, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinkim0 martinkim0 martinkim0 left review comments

Assignees

@ori-kron-wis ori-kron-wis

Labels
on-merge: backport to 1.2.x on-merge: backport to 1.2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@canergen @martinkim0 @ori-kron-wis