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

Update moments.py: bug fix #461

Merged
merged 2 commits into from Mar 13, 2023
Merged

Conversation

dummyindex
Copy link
Contributor

@dummyindex dummyindex commented Mar 13, 2023

Without this fix: if there are any key starting with X_ that is not in the groups defined for splicing/unsplicing/new/total (e.g. X_Mu generated by other packages shown below), moments function will raise [0][0] dimensionality issue.

Fixed version:
image

@Xiaojieqiu
Copy link
Collaborator

thanks @dummyindex. X_ is dynamo's default setting and I am not sure other tools may generate M_us layers. I think if M_us exists that is mostly because you have run dyn.tl.moments before and attempts to rerun moments. The easiest way is to skip running moments as implemented in dyn.tl.dynamics once moments is calculated

@dummyindex
Copy link
Contributor Author

dummyindex commented Mar 13, 2023

thanks @dummyindex. X_ is dynamo's default setting and I am not sure other tools may generate M_us layers. I think if M_us exists that is mostly because you have run dyn.tl.moments before and attempts to rerun moments. The easiest way is to skip running moments as implemented in dyn.tl.dynamics once moments is calculated

X_us exists because of anndata is processed by other tools, e.g. scVelo. (The adata case above is from a paper never used dynamo-release but scVelo.) "X_" layers can also be a standard in other packages. Moreover, if users have their customized X_sth layer, moments calculation will fail and a typical user cannot figure out what happens without checking source code, which most users may not have time and energy to do so, even with sufficient programming background. Skipping these layers not related to moments and allowing the calculation to continue should be a reasonable choice.
Regarding running dynamics(): If you directly run dynamics() on the adata with X_us layer, the issue remains because dynamics() still calls moments. The screenshot above uses moments directly to show the issue is caused by moments calculation.

@Xiaojieqiu
Copy link
Collaborator

I am pretty certain that scVelo doesn't use X_us or M_us as we do but instead Mus. But I agree that X_us can be generated. Also dynamics only avoids doing moments if M_* layers exists which doesn't really fix the issue.

@Xiaojieqiu Xiaojieqiu merged commit 704ac86 into aristoteleo:master Mar 13, 2023
6 checks passed
@dummyindex
Copy link
Contributor Author

dummyindex commented Mar 14, 2023

I am pretty certain that scVelo doesn't use X_us or M_us as we do but instead Mus. But I agree that X_us can be generated. Also dynamics only avoids doing moments if M_* layers exists which doesn't really fix the issue.

I have confirmed that the X_Ms is generated during preprocessing part for the conventional case, which causes the moments problem later. The raw data provided by the authors do not contain X_Ms layers.

image

@dummyindex dummyindex deleted the moments-bug-fix branch March 14, 2023 16:56
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.

None yet

2 participants