Adding model with cycle consistency and VampPrior #2421
Conversation
Hrovatin
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2421 +/- ##
==========================================
- Coverage 84.81% 84.40% -0.42%
==========================================
Files 173 178 +5
Lines 14793 15168 +375
==========================================
+ Hits 12547 12802 +255
- Misses 2246 2366 +120
|
There was a problem hiding this comment.
Hey, thanks for putting in this PR! A couple of high-level comments for now.
Could you restructure the files as follows?
csi/model/_model.py->csi/_model.pycsi/model/_utils.py->csi/_utils.pycsi/module/_module.py->csi/_module.pycsi/module/_priors.py->csi/_priors.pycsi/nn/_base_components.py->csi/_components.pycsi/train/_trainingplans.py->csi/_trainingplans.py- Refactor
csi/module/_module.pyto usescvi.module.base.LossOutputinstead of our previousLossRecorderand removecsi/module/_loss_recoder.py - If the plan is to directly inherit from
UnsupervisedTrainingMixinwithout any major changes except setting_training_plan_cls, this can be done within_model.pyby directly subclassingUnsupervisedTrainingMixinand setting_training_plan_clsthere instead, so I would recommend doing that and deletingcsi/model/_training.py
I can take a look at compatibility with scArches in the next round.
We're also slowly migrating to newer typing annotations in Python. Could you add a from __future__ import annotations to the top of every file (except __init__.pys) and migrate typing from:
Optional[x]->x | NoneUnion[x, y]->x | yList[x]->list[x]Tuple[x]->tuple[x]Callable->callable- There's more, I don't remember all of them. The pre-commit hooks will warn about missing ones.
|
I added changes and adapted tutorial accrordingly |
|
Thanks for addressing the comments! I'll take another pass at it soon. |
martinkim0
temporarily deployed
to
private_test
GitHub Actions
Inactive
martinkim0
temporarily deployed
to
private_test
GitHub Actions
Inactive
|
@canergen Sorry, I am no longer active there and overlooked that you responded. I sent you my personal email over zolip to arrange a meeting. |
|
@Hrovatin there are some fixes that need to be done regarding the pre-commit ruff rules (everything that involves linting the code, formatting it etc'..) to be compatible with the rest of our repo and package. |
for more information, see https://pre-commit.ci
|
The changes I pushed today are not yet ready for review. Missing docs, cleanup, and test on real data. |
for more information, see https://pre-commit.ci
For code simplicity the use of mock covariates was replaced with real covariates in cycle. Needs testing on real data.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@canergen @ori-kron-wis The PR is now ready for review. @ori-kron-wis Can you help me fix the cuda test err? For some reason, it can not import SysVI, although for me, it works locally. I also was not able to test on GPU since I only have MacOS access and I get the err where mps is detected but not used. - If you have any solutions for that also please let me know. |
Closes #2383