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

Adding mixin classes for pyro training and posterior sampling #1059

Merged
merged 62 commits into from
Jun 19, 2021

Conversation

vitkl
Copy link
Contributor

@vitkl vitkl commented May 12, 2021

This PR adds mixin classes for

  1. For pyro training
  2. posterior sampling.

It also fixes an issue where pyro model history was erased on load

Fixes #1031

scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
tests/models/test_pyro.py Outdated Show resolved Hide resolved
tests/models/test_pyro.py Outdated Show resolved Hide resolved
tests/models/test_pyro.py Show resolved Hide resolved
tests/models/test_pyro.py Outdated Show resolved Hide resolved
tests/models/test_pyro.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Attention: Patch coverage is 88.46154% with 15 lines in your changes missing coverage. Please review.

Project coverage is 90.64%. Comparing base (a0a6089) to head (52759bb).

Files Patch % Lines
scvi/model/base/_pyromixin.py 92.79% 8 Missing ⚠️
scvi/module/base/_base_module.py 28.57% 5 Missing ⚠️
scvi/model/base/_base_model.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
- Coverage   90.67%   90.64%   -0.03%     
==========================================
  Files          90       91       +1     
  Lines        6745     6885     +140     
==========================================
+ Hits         6116     6241     +125     
- Misses        629      644      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scvi/train/_trainingplans.py Outdated Show resolved Hide resolved
@vitkl
Copy link
Contributor Author

vitkl commented May 17, 2021

Interestingly, the last test works but cell2location with full data and non-amortised inference gives an error which I do not quite understand. See HTML attached (can be reproduced by taking this notebook to colab).
cell2location_analysis_3_scvi_ln_full_data.html.zip

UPD: Problem solved (a48d8ff).

scvi/model/base/_pyromixin.py Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
scvi/model/base/_pyromixin.py Outdated Show resolved Hide resolved
@vitkl
Copy link
Contributor Author

vitkl commented May 31, 2021

Sorry for the accidental merge again, I don't get why this merge happened. I am trying to merge pyro-mixin into pyro-cell2location branch using GitHub pull request web interface. For some reason, that seems to create a merge commit from pyro-cell2location into pyro-mixin.

UPD: fixed now

@adamgayoso adamgayoso merged commit bc415d8 into scverse:master Jun 19, 2021
@vitkl
Copy link
Contributor Author

vitkl commented Jun 22, 2021

@adamgayoso after talking to @martinjankowiak I think the default optimiser should be Adam, not ClippedAdam. I can change that. Should I modify this branch/PR or create a new one?

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.

Add a Pyro training mixin
2 participants