feat: scVI-X release - #3823
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@ori-kron-wis code can be checked. |
| assert params_latent[k].shape == params_orig[k].shape | ||
|
|
||
| for k in keys: | ||
| assert_approx_equal(params_latent[k], params_orig[k]) |
There was a problem hiding this comment.
Results are not equal (only tests failure now), I guess, because of resampling of the minified model, you will need to tweak rtol and atol (increase - but how much?)
|
|
||
| return pair_penalty | ||
|
|
||
| def mmd(self, params, mask=None): |
There was a problem hiding this comment.
if mask is None (default), what are mod_1 and mod_2? it will break.
also seems that this is the case cause line 661 invokes mmd with mask==None
its probably not covered in tests
| ArchesMixin, | ||
| BaseMinifiedModeModelClass, | ||
| ): | ||
| """single-cell Variational Inference :cite:p:`Lopez18`. |
There was a problem hiding this comment.
I guess you will want to put you pre-print title here
| encode_assay_list = [n_assay] | ||
| else: | ||
| encode_assay_list = [0] | ||
| if not encode_covariates: |
There was a problem hiding this comment.
if encode_covaraites if False, what is self.batch_representation_encoder? might not be utilized later on, but still need a value for this model registry (later save/load etc)
| pseudoinputs_data_indices = np.random.randint( | ||
| 0, self.summary_stats.n_cells, n_prior_components | ||
| ) | ||
| assert pseudoinputs_data_indices.shape[0] == n_prior_components |
There was a problem hiding this comment.
do we need to check assert in the runtime code? It will not tell anything.
I suggest otherwise to check if condition, and plot an Error message
| if prior == "gaussian": | ||
| self.prior = GaussianPrior() | ||
| elif prior == "vamp": | ||
| assert pseudoinput_data is not None, ( |
There was a problem hiding this comment.
again, better to add a runtime error/warning message
ori-kron-wis
left a comment
ori-kron-wis
left a comment
There was a problem hiding this comment.
I think the api changes in core function (trainingplans and base_components) are not really needed and we can avoid most of it by changing it internally in scvix module, thus, without affect other models.
For the changes in adversarial_classifier - it affect other models for sure (totalvi, gimvi) - are we certain they are ok?
| @@ -761,9 +805,7 @@ def configure_optimizers(self): | |||
|
|
|||
| if self.adversarial_classifier is not False: | |||
There was a problem hiding this comment.
did we validate TotalVI functionality? multivi? There are parameter changes here (perhaps we can encode for scviX but return to previous values otherwise)
| n_input=self.module.n_latent, | ||
| n_hidden=32, | ||
| n_input=self.module.n_latent + getattr(self.module, "n_adversarial_group", 0), | ||
| n_hidden=128, |
There was a problem hiding this comment.
another parameter change, how di dit affect other adversarial models?
| A list containing, for each category of interest, | ||
| the number of categories. Each category will be | ||
| included using a one-hot encoding. | ||
| n_cont |
There was a problem hiding this comment.
this is still here, but you changed the API (why?) - my suggestion is to revert to n_cont and update scvi_x and not vice versa
There was a problem hiding this comment.
a change made to cope with api changes. need to verify
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3823 +/- ##
==========================================
+ Coverage 88.54% 88.71% +0.16%
==========================================
Files 231 234 +3
Lines 23064 23514 +450
==========================================
+ Hits 20423 20861 +438
- Misses 2641 2653 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ori-kron-wis
added
on-merge: backport to 1.5.x
2 participants
Do not merge or review yet.