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

Inconsistency in covariate injection into layer #3021

Open
Hrovatin opened this issue Oct 19, 2024 · 3 comments
Open

Inconsistency in covariate injection into layer #3021

Hrovatin opened this issue Oct 19, 2024 · 3 comments

Comments

@Hrovatin
Copy link

The FCLayers enable the re-inject of covariates in every layer. However, this is enabled only for categorical cov with one hot encoding. This is now inconsistent with other covariates (continuous, embedded) that get simply concat to the input before being passed to the layers and thus not re-injected.
For SysVI I will make a new Layers class that will directly follow the FCLayers, but will have another input for continuous covariates, so that these can be likewise injected. - You could consider fixing the above inconsistency in scvi-tools in the same way I guess.
@canergen

@Hrovatin Hrovatin added the bug label Oct 19, 2024
@canergen
Copy link
Member

@ori-kron-wis can you please fix this? It should behave similarly for all types of covariates.
It needs a legacy load though similarly to the fix in scANVI. Maybe the better option would be deeply_inject=‘all’ and clarify in the documentation the difference.

@Hrovatin
Copy link
Author

Hrovatin commented Oct 20, 2024

Great, then I can just re-use your layers if this will be implemented from your side - for sysVI maybe I will already add an updated Layers version myself so that I can then be more easily switched with your new layers, else the rest of the code would also need to be adapted later

@Hrovatin
Copy link
Author

@ori-kron-wis I added my implementation here - Maybe you will need to adjust slightly as I changed parameter signature to add the continuous kwarg (x,cont=None,cat_list=None) - I think you could change it to (x,*cat_list,cont=None) to reduce changes needed in other models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants