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

Add key_added option to sc.tl.paga #957

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

gokceneraslan
Copy link
Collaborator

@gokceneraslan gokceneraslan commented Dec 19, 2019

It'd be cool to allow multiple abstract representations of the same graph (e.g. w.r.t different groups). What do you think about storing PAGA info under adata.uns['paga'][key_added] instead of directly adata.uns['paga']?

@gokceneraslan gokceneraslan changed the title Add key_added option to paga Add key_added option to sc.tl.paga Dec 19, 2019
@LuckyMD
Copy link
Contributor

LuckyMD commented Dec 19, 2019

Is this backward compatible for cases where old AnnData object are loaded?

@gokceneraslan
Copy link
Collaborator Author

The default behavior is the same, if key is not given, everything will be stored under adata.uns['paga'] as before and the plotting functions will also look for adata.uns['paga'] unless a key='key' is specified.

@LuckyMD
Copy link
Contributor

LuckyMD commented Dec 19, 2019

I see... but as it's always adata.uns['paga'] what would happen if I have an old object and then run a new sc.tl.paga() with key set to something. I assume the new version has a dict in adata.uns['paga'] with the keys of the individual paga runs, whereas the old case just had the paga run data in there directly.

@gokceneraslan
Copy link
Collaborator Author

I see... but as it's always adata.uns['paga'] what would happen if I have an old object and then run a new sc.tl.paga() with key set to something.

Then you'll have it under both adata.uns['paga'] and adata.uns['paga'][key]. That's not backwards compatibility though. BC is running the old code (so no key='key') with an old object (where paga is under adata.uns['paga']) with new scanpy and getting the old result, which is satisfied here.

There are weird failure modes though, like using key='groups' or key='connectivities' might override some parts of an existing, old-style paga result. We can forbid keys like these.

Actually this is related to the versioning of AnnData specification. We should keep some sort of version like /attrs/LOOM_SPEC_VERSION in loom. Then it would be easier to understand what to expect from an anndata object that is created at any point in time.

@LuckyMD
Copy link
Contributor

LuckyMD commented Dec 19, 2019

There are weird failure modes though, like using key='groups' or key='connectivities' might override some parts of an existing, old-style paga result. We can forbid keys like these.

this is exactly what i was worried about. 'groups' is probably not such a rare case as a key. But good to take care of that. I guess in the old & new case, you just have a bit of a messy adata.uns['paga'] dictionary, but it will still work (in most cases).

@falexwolf
Copy link
Member

I'd actually be fine with merging this - @gokceneraslan, you can merge yourself, can't you?

@falexwolf falexwolf self-requested a review May 12, 2020 09:36
@falexwolf
Copy link
Member

We'll have 1.5.0 in a couple of days, and so it's fine to make a few behavior changes. We should put a warning in the release notes, though.

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