-
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
New user guide / tutorials #1127
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1127 +/- ##
=======================================
Coverage 90.27% 90.27%
=======================================
Files 93 93
Lines 7341 7341
=======================================
Hits 6627 6627
Misses 714 714
Continue to review full report at Codecov.
|
c566ed6
to
f03bbcc
Compare
update scvi update scvi vi tutorial vi vi vi vi
6c43f3b
to
c17f6c5
Compare
@adamgayoso Can you send a link to a draft version of the site generated from these changes? |
@watiss https://scvi--1127.org.readthedocs.build/en/1127/user_guide/index.html You can also find it in the checks of the PR, via the readthedocs check, after it completes. |
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.
Left a bunch of nits, but overall the structure of stuff looks nice. Also, if there is a way to have a hierarchical structure on the outline on the left sidebar, it would be nice to match it with the User Guide landing page.
where `predictions` stores :math:`\gamma_{nc}` for each cell :math:`n` and cell type :math:`c`. | ||
|
||
|
||
Implementation details |
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.
Given the number of variables here, I think a table like in the scVI guide would be helpful.
Thus, for each cell, we observe both RNA and protein information. | ||
Additionally, a design matrix :math:`S` containing :math:`p` observed covariates, such as day, donor, etc, is an optional input. | ||
While :math:`S` can include both categorical covariates and continuous covariates, in the following, we assume it contains only one | ||
categorical covariate with :math:`B` categories, which represents the common case of having multiple batches of data. |
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.
The number of categories was K in the scVI guide, should we be consistent?
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.
probably yes
docs/user_guide/models/totalvi.rst
Outdated
Generative process | ||
======================== | ||
|
||
.. figure:: figures/totalvi_graphical_model.svg |
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.
would suggest moving this further below once you have defined the variables
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.
also noticed that this image doesnt include the global parameters
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.
going to leave this as is, we typically haven't included global params in these graphical models
* - :doc:`/user_guide/models/scvi` | ||
- [Lopez18]_ | ||
- :class:`~scvi.model.SCVI` | ||
- Dimensionality reduction, removal of unwanted variation, integration across replicates, donors, and technologies, differential expression, imputation, normalization of other cell- and sample-level confounding factors |
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.
nit: I'd bullet point-ify 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.
same for the totalVI one
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.
need to figure this out, will post in the issue.
|
||
|
||
|
||
.. topic:: References: |
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.
Should we also link the scvi-tools preprint?
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.
I would say that scvi-tools encompasses all the methods. Best to make the original method called scVI seem independent so others don't think including their methods in scvi-tools means that it's scVI.
* - Latent variable | ||
- Description | ||
- Code variable (if different) |
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.
nit: The table header looks a bit funny because the last column name is too long relative to its width. Consider adjusting column widths or this column name
Closes #1108
This is the initial skeleton for the new docs. We can merge this and then continue to make PRs for the rest of the models.