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

Thorough use of distributions to clean module-level code #1356

Merged
merged 29 commits into from
May 31, 2022

Conversation

PierreBoyeau
Copy link
Contributor

@PierreBoyeau PierreBoyeau commented Feb 15, 2022

Fixes #1125

@PierreBoyeau PierreBoyeau reopened this Feb 15, 2022
@PierreBoyeau PierreBoyeau changed the title Distributions outb [WIP]: Distributions outb Feb 15, 2022
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #1356 (c581d14) into master (0ced5d1) will decrease coverage by 0.01%.
The diff coverage is 93.29%.

@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
- Coverage   91.03%   91.01%   -0.02%     
==========================================
  Files         113      113              
  Lines        9023     8996      -27     
==========================================
- Hits         8214     8188      -26     
+ Misses        809      808       -1     
Impacted Files Coverage Δ
scvi/distributions/__init__.py 100.00% <ø> (ø)
scvi/model/base/_vaemixin.py 92.30% <66.66%> (-0.29%) ⬇️
scvi/module/_multivae.py 91.32% <75.00%> (+2.31%) ⬆️
scvi/distributions/_negative_binomial.py 87.11% <80.00%> (-0.59%) ⬇️
scvi/module/_peakvae.py 93.93% <80.00%> (+1.78%) ⬆️
scvi/module/_vae.py 94.76% <89.65%> (+0.34%) ⬆️
scvi/module/_vaec.py 85.71% <90.90%> (+1.80%) ⬆️
scvi/model/base/_rnamixin.py 91.57% <91.66%> (-0.53%) ⬇️
scvi/external/gimvi/_module.py 80.00% <100.00%> (-0.27%) ⬇️
scvi/model/_autozi.py 98.79% <100.00%> (-0.05%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ced5d1...c581d14. Read the comment docs.

@PierreBoyeau PierreBoyeau changed the title [WIP]: Distributions outb Distributions outb Feb 15, 2022
scvi/model/_autozi.py Show resolved Hide resolved
scvi/module/_vae.py Show resolved Hide resolved
scvi/module/_totalvae.py Outdated Show resolved Hide resolved
scvi/module/_amortizedlda.py Outdated Show resolved Hide resolved
@justjhong
Copy link
Contributor

nit: please make the title of the PR more meaningful (since it's the commit message for squash merge)

@adamgayoso adamgayoso changed the title Distributions outb Thorough use of distributions to clean module-level code Feb 16, 2022
scvi/nn/_base_components.py Outdated Show resolved Hide resolved
scvi/nn/_base_components.py Outdated Show resolved Hide resolved
scvi/nn/_base_components.py Show resolved Hide resolved
scvi/module/_vae.py Outdated Show resolved Hide resolved
scvi/module/_vae.py Show resolved Hide resolved
scvi/module/_vae.py Show resolved Hide resolved
@adamgayoso adamgayoso added this to the 0.16.0 milestone Feb 18, 2022
@adamgayoso adamgayoso modified the milestones: 0.16.0, 0.17.0 Apr 7, 2022
Copy link
Contributor

@justjhong justjhong left a comment

Choose a reason for hiding this comment

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

looks good to me, needs to be added to release note

Parameters
----------
rate
rate of the Poisson distribution.
Copy link
Contributor

Choose a reason for hiding this comment

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

improve docs on optional args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry wasnt clear, just meant to expand on the definitions which looks good now. You can remove ": optional" because sphinx takes care of that for us

scvi/distributions/_negative_binomial.py Outdated Show resolved Hide resolved
scvi/module/_totalvae.py Show resolved Hide resolved
scvi/model/base/_rnamixin.py Outdated Show resolved Hide resolved
scvi/module/_autozivae.py Outdated Show resolved Hide resolved
scvi/model/_autozi.py Outdated Show resolved Hide resolved
@adamgayoso
Copy link
Member

@PierreBoyeau can you also add a new v0.17.0 release note and get it started in this PR?

@PierreBoyeau
Copy link
Contributor Author

Sure!

@PierreBoyeau
Copy link
Contributor Author

i addressed the points you raised @jjhong922 @adamgayoso

Copy link
Contributor

@justjhong justjhong left a comment

Choose a reason for hiding this comment

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

final minor comments, but i think its good to go

@@ -3,6 +3,7 @@
## Changes

- Experimental MuData support for {class}`~scvi.model.TOTALVI` via the method {meth}`~scvi.model.TOTALVI.setup_mudata`. For several of the existing `AnnDataField` classes, there is now a MuData counterpart with an additional `mod_key` argument used to indicate the modality where the data lives (e.g. {class}`~scvi.data.fields.LayerField` to {class}`~scvi.data.fields.MuDataLayerField`). These modified classes are simply wrapped versions of the original `AnnDataField` code via the new {method}`scvi.data.fields.MuDataWrapper` method [#1474].
- Modification of the `generative` method's outputs to return prior and likelihood properties as `torch.Distribution` objects. Concerned modules are `_amortizedlda.py`, `_autozivae.py`, `multivae.py`, `_peakvae.py`, `_scanvae.py`, `_vae.py`, and `_vaec.py`. This allows facilitating the manipulation of these distributions for model training and inference [#1356].
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the api doc notation e.g. {class}~scvi.module.VAE instead of _vae.py

Parameters
----------
rate
rate of the Poisson distribution.
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry wasnt clear, just meant to expand on the definitions which looks good now. You can remove ": optional" because sphinx takes care of that for us

@justjhong justjhong merged commit 04389f7 into master May 31, 2022
@justjhong justjhong deleted the distributions_outb branch May 31, 2022 18:21
nrclaudio pushed a commit to nrclaudio/scvi-tools-tune that referenced this pull request Jun 21, 2022
* replace normal parameters in inference by distribution

* totalVI changes

* merge fixes

* fix totalvi

* fixes

* fix gimvi

* pyro

* clean

* clean

* dummy

* log prob sum

* fix conversion

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* simplication log_prob_sum

* rename px

* px

* Optional return_dist

* feat generative refactor

* updates

* precommit

* docstring

* docstring

* docstring

* release note

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Comment on lines +249 to +254
validate_args : optional
whether to validate input.
scale : optional
Normalized mean expression of the distribution.
This optional parameter is not used in any computations, but allows to store
normalization expression levels.
Copy link
Member

Choose a reason for hiding this comment

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

@PierreBoyeau @jjhong922 can we fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

are you talking about the docs or the optional parameter here? I can remove the :optional in the docs now

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.

Replace variational distribution parameters by Distribution outputs
3 participants