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

Updated destVI #1457

Merged
merged 45 commits into from
Apr 6, 2022
Merged

Updated destVI #1457

merged 45 commits into from
Apr 6, 2022

Conversation

canergen
Copy link
Member

  1. Fixed cell_type amortization (error with neural network structure). Now performs similar to no cell_type amortization. Changed the default to both amortized.
  2. Added L1_sparsity similar to final version of manuscript.
  3. Added dropout at condSCVI decoder and changed all layers to layer norm to increase stability.
  4. Vamp_prior uses over clustering now to find better metapoints. Beforehand the selection of vamp_prior led to different results. Downside: Currently, adds dependency on scanpy and leiden. Do we want this? kmeans clustering was comparable in performance but more unusual choice. We can also remove the cluster generation step from the code and run it outside manually.
  5. Changed library size to non-log in CondSCVI
  6. Updated hyperparameters to match with reproducibility script from destVI paper.

@canergen canergen mentioned this pull request Mar 22, 2022
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
canergen and others added 3 commits March 22, 2022 12:10
Changed to import only submodules of scanty. Only imports if needed
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #1457 (4176ab3) into master (6e734bc) will increase coverage by 0.02%.
The diff coverage is 98.27%.

@@            Coverage Diff             @@
##           master    #1457      +/-   ##
==========================================
+ Coverage   91.04%   91.06%   +0.02%     
==========================================
  Files         112      112              
  Lines        8775     8809      +34     
==========================================
+ Hits         7989     8022      +33     
- Misses        786      787       +1     
Impacted Files Coverage Δ
scvi/model/_condscvi.py 96.62% <97.36%> (-0.35%) ⬇️
scvi/model/_destvi.py 88.28% <100.00%> (+0.10%) ⬆️
scvi/module/_mrdeconv.py 95.07% <100.00%> (+0.33%) ⬆️
scvi/module/_vaec.py 83.90% <100.00%> (+0.18%) ⬆️

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 6e734bc...4176ab3. Read the comment docs.

@adamgayoso
Copy link
Member

@cane11 we are losing some test coverage for condscvi -- can you add more tests for these new options?

@adamgayoso adamgayoso added this to the 0.16.0 milestone Mar 22, 2022
@canergen
Copy link
Member Author

The coverage difference is related to the import error for scanpy (and differences in number of lines). Should I add a test for the ImportError? The new functions seem to be all covered.

@romain-lopez
Copy link
Member

Thanks, I'll review this ASAP. Ping me if I haven't done it soon enough please :)

@romain-lopez romain-lopez self-requested a review March 22, 2022 20:59
@adamgayoso
Copy link
Member

The coverage difference is related to the import error for scanpy (and differences in number of lines). Should I add a test for the ImportError? The new functions seem to be all covered.

if that's it then no, no need.

Also as an aside, this version is not backwards compatible with previously saved versions of destVI. I think this is personally fine, but in the future we should put experimental tags on models that exist before a publication is out.

@canergen
Copy link
Member Author

canergen commented Mar 23, 2022

It's two bugs (that are not backward compatible), new Vamp_prior is backward compatible using (should we include it in the tutorial?):

adata.obs['vamp_overclustering'] = list(range(adata.n_obs))
adata.obs['vamp_overclustering'] = adata.obs['vamp_overclustering'].astype('category')

I can make the additional parameter updates non hardcoded to increase compatibility. It was hardcoded in the previous version, which is why I kept it hardcoded (dropout in destVI, layer norm in destVI, weights of bayesian prior on beta and eta). Especially, the weight for beta might be different for unusual data inputs (it regularizes how similar sc and spatial technology are).

scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Show resolved Hide resolved
scvi/model/_condscvi.py Show resolved Hide resolved
scvi/model/_destvi.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Show resolved Hide resolved
scvi/module/_mrdeconv.py Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_vaec.py Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
tests/models/test_models.py Outdated Show resolved Hide resolved
@romain-lopez
Copy link
Member

romain-lopez commented Apr 3, 2022

Hi Can, thanks for your newer work on this PR. I think this looks almost perfect except for the glitch with the dropout parameter. I might be wrong but it seems that it's not really used anywhere. It should be either user dependent or fixed everywhere or fixed in some places but not some others. I'd lean towards deleting it as an attribute and keeping it hardcoded!

After these minor points, we can proceed to merging this! @adamgayoso anything else that needs to be done on your end?

scvi/model/_condscvi.py Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
scvi/model/_condscvi.py Outdated Show resolved Hide resolved
n_labels_overclustering = len(keys)
if n_labels_overclustering > p:
raise ValueError(
"Given overclustering contains more clusters than vamp_prior_p. Increase value."
Copy link
Member

Choose a reason for hiding this comment

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

I'd say exactly which parameter name needs to be changed

Copy link
Member Author

Choose a reason for hiding this comment

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

Please check this fix.

scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_mrdeconv.py Outdated Show resolved Hide resolved
scvi/module/_vaec.py Outdated Show resolved Hide resolved
Copy link
Member

@romain-lopez romain-lopez left a comment

Choose a reason for hiding this comment

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

two remaining minor things

To explore the results of the output of the stLVM, we published a utilities function covering functions
for automatic thresholding of cell type proportions, a spatial PCA analysis to find main axis of variation
in spatial gene expression and the described frequentist test for differential expression. Further information
can be found on [destvi_utils](https://destvi-utils.readthedocs.io/en/latest/installation.html)

[^ref1]: Romain Lopez, Baoguo Li, Hadas Keren-Shaul, Pierre Boyeau, Merav Kedmi, David Pilzer, Adam Jelinski, Eyal David, Allon Wagner, Yoseph Addad, Michael I. Jordan, Ido Amit, Nir Yosef (2021),
Copy link
Member

Choose a reason for hiding this comment

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

ah, we can change the title here

Copy link
Member

Choose a reason for hiding this comment

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

Romain Lopez, Baoguo Li, Hadas Keren-Shaul, Pierre Boyeau, Merav Kedmi, David Pilzer, Adam Jelinski, Ido Yofe, Eyal David, Allon Wagner, Can Ergen, Yoseph Addadi, Ofra Golani, Franca Ronchese, Michael I Jordan, Ido Amit, Nir Yosef. DestVI identifies continuums of cell types in spatial transcriptomics data. Nature Biotechnology (in press), 2022.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it consistent with other packages and also added link to VAMP prior paper

scvi/model/_destvi.py Show resolved Hide resolved
@romain-lopez
Copy link
Member

@adamgayoso ready to merge!

@adamgayoso adamgayoso enabled auto-merge (squash) April 6, 2022 22:46
@adamgayoso adamgayoso merged commit c2d7221 into scverse:master Apr 6, 2022
@adamgayoso
Copy link
Member

meeseeksdev backport to 0.16.x

@lumberbot-app
Copy link

lumberbot-app bot commented Apr 7, 2022

Can't Dooooo.... It seem like this is already backported (commit is empty).I won't do anything. MrMeeseeks out.

@adamgayoso
Copy link
Member

meeseeksdev backport to 0.16.x

@lumberbot-app
Copy link

lumberbot-app bot commented Apr 7, 2022

Can't Dooooo.... It seem like this is already backported (commit is empty).I won't do anything. MrMeeseeks out.

nrclaudio pushed a commit to nrclaudio/scvi-tools-tune that referenced this pull request Jun 21, 2022
* Fix destVI cell type amortization, Vamp prior from overclustering, parameter optimization

* Registry wrong in condSCVI in last commit

* REGISTRY key in condSCVI

* Updated training parameters of destVI

* Learned rate adapted for destVI

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

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

* Update import of scanty

Changed to import only submodules of scanty. Only imports if needed

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

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

* Added leidenalg to test dependencies

* Revert "Added leidenalg to test dependencies"

This reverts commit f4ea841.

* Changed scanpy import to give error message

* Added leiden to scanpy requirements

* Defaults to no l1_sparsity.

* Style changes to condSCVI.

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

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

* Space in empty line removed

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

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

* Changed vamp overclustering to kmeans, minor comments, added test cases.

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

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

* Error in test function. Added check for custom clustering

* Merge conflict

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

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

* Edited dataset after setup of CondSCVI

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

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

* Error in test function.

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

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

* Update test function

* Ran precommit

* Changed eta_prior_weight, exposed beta_prior_weight. Small adjustments.

* Doc changes, weight_vprior->mp_vprior

* Updated documentation. Style changes. Renamed weighting parameters.

* L1_reg added to test function

* Updated logsumexp. Small fixes

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

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

* Precommit edit.

* np.array -> torch.cat

* Updated references. Updated dropout_decoder

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adam Gayoso <[email protected]>
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.

3 participants