-
Notifications
You must be signed in to change notification settings - Fork 346
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
nit: please make the title of the PR more meaningful (since it's the commit message for squash merge) |
for more information, see https://pre-commit.ci
There was a problem hiding this 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
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
@PierreBoyeau can you also add a new v0.17.0 release note and get it started in this PR? |
Sure! |
i addressed the points you raised @jjhong922 @adamgayoso |
There was a problem hiding this 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]. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
* 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>
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Fixes #1125