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

chore(module): remove unused code paths in scanvae loss #2644

Merged
merged 7 commits into from Apr 3, 2024
Merged

Conversation

martinkim0
Copy link
Contributor

@martinkim0 martinkim0 commented Apr 1, 2024

BREAKING CHANGE: removes unused code paths in the loss method of SCANVAE due to the default being feed_labels=False. internal functions that call this method either have their own default set to feed_labels=False or have feed_labels=True but don't pass it into the loss method (e.g. compute_elbo).

also adds typing within the modified file.

closes #2134

@@ -323,32 +320,6 @@ def loss(
else:
kl_divergence_l = 0.0

if is_labelled:
Copy link
Contributor Author

@martinkim0 martinkim0 Apr 1, 2024

Choose a reason for hiding this comment

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

feed_labels is always False => y is always None => is_labelled is always False

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.83%. Comparing base (d51d370) to head (c16c706).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2644      +/-   ##
==========================================
+ Coverage   88.78%   88.83%   +0.05%     
==========================================
  Files         158      158              
  Lines       13245    13234      -11     
==========================================
- Hits        11759    11756       -3     
+ Misses       1486     1478       -8     
Files Coverage Δ
scvi/model/base/_log_likelihood.py 96.55% <100.00%> (ø)
scvi/module/_scanvae.py 87.80% <100.00%> (+5.09%) ⬆️
scvi/train/_trainingplans.py 94.54% <ø> (ø)

... and 2 files with indirect coverage changes

y = None
is_labelled = False if y is None else True

# Enumerate choices of label
ys, z1s = broadcast_labels(y, z1, n_broadcast=self.n_labels)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could probably clean up this function call since y is always None - it would help with debugging and addressing #2429

@martinkim0 martinkim0 requested a review from canergen April 1, 2024 23:39
@martinkim0
Copy link
Contributor Author

per our discussion in slack @canergen

@martinkim0
Copy link
Contributor Author

martinkim0 commented Apr 1, 2024

sanity check with the Atlas-level integration of lung data tutorial:

harmonization.pdf

@martinkim0 martinkim0 added the on-merge: backport to 1.2.x on-merge: backport to 1.2.x label Apr 2, 2024
@martinkim0 martinkim0 merged commit 85540d1 into main Apr 3, 2024
10 of 12 checks passed
@martinkim0 martinkim0 deleted the scanvi-loss branch April 3, 2024 00:19
meeseeksmachine pushed a commit to meeseeksmachine/scvi-tools that referenced this pull request Apr 3, 2024
martinkim0 added a commit that referenced this pull request Apr 3, 2024
…paths in scanvae loss) (#2671)

Backport PR #2644: chore(module): remove unused code paths in scanvae
loss

Co-authored-by: Martin Kim <46072231+martinkim0@users.noreply.github.com>
@canergen
Copy link
Contributor

canergen commented Apr 3, 2024

LGTM! Would an old model still load? I guess so as all of these feed_labels are created dynamically. Otherwise, we need to update the scvi-hub scANVI models.

@canergen
Copy link
Contributor

canergen commented Apr 3, 2024

Not sure it's breaking in this case as it was never active.

@martinkim0
Copy link
Contributor Author

Yeah I think it should be backwards compatible - we have tests for old scanvi loads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Clean up loss computation in SCANVI
2 participants