-
Notifications
You must be signed in to change notification settings - Fork 350
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
Adding model with cycle consistency and VampPrior #2421
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.py
csi/model/_utils.py
->csi/_utils.py
csi/module/_module.py
->csi/_module.py
csi/module/_priors.py
->csi/_priors.py
csi/nn/_base_components.py
->csi/_components.py
csi/train/_trainingplans.py
->csi/_trainingplans.py
- Refactor
csi/module/_module.py
to usescvi.module.base.LossOutput
instead of our previousLossRecorder
and removecsi/module/_loss_recoder.py
- If the plan is to directly inherit from
UnsupervisedTrainingMixin
without any major changes except setting_training_plan_cls
, this can be done within_model.py
by directly subclassingUnsupervisedTrainingMixin
and setting_training_plan_cls
there 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__.py
s) and migrate typing from:
Optional[x]
->x | None
Union[x, y]
->x | y
List[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. |
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.
Some minor typing/style comments as well as implementation ones. I still don't understand all parts of the implementation so I will take another pass at this
@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