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

Fix fastMNN corrected feature output #18

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

Conversation

danielStrobl
Copy link
Contributor

A fastMNN API change causes anndata2ri to break for newer batchelor versions as LowRankMatrix can't be converted to python. Converting to dgCMatrix fixes that.

Not sure if we want to lock to an older batchelor version or do this fix here.

A fastMNN API change causes anndata2ri to break as `LowRankMatrix` can't be converted to python. Converting to `dgCMatrix` fixes that.
@danielStrobl danielStrobl requested a review from mumichae March 21, 2022 13:56
@mumichae
Copy link
Collaborator

Thanks for spotting that! I was wondering why fastMNN suddenly stopped working but didn't know that it was related to a newer version. Will test if the pipeline runs through and get back to you on this

@LuckyMD
Copy link
Collaborator

LuckyMD commented Mar 21, 2022

I think future proofing is better. Thanks for fix @danielStrobl! @mumichae you have power over merging, so if you give it a thumbs up, please merge :).

@mumichae
Copy link
Collaborator

I'm still trying to run things, and as it stands, the code doesn't seem tested yet (I got a syntax error). I still need a bit of time though, as I'm having issues with my R environment atm.

@danielStrobl
Copy link
Contributor Author

danielStrobl commented Mar 21, 2022

this was missing a bracket, was just quickly transferring my fix without testing from openproblems: danielStrobl/SingleCellOpenProblems@2092d72

@mumichae
Copy link
Collaborator

I just realised that we are most likely looking at different problems here. In Openproblems we are using R 4 and Seuratv4, whereas the pipeline uses R 3.6 and Seuratv3. For some reason, I am not getting any errors with fastMNN anymore after recreating the R environment.

Sooner or later, we should move to R 4 and only keep the 3.6 and respective dependencies for reproducibility reasons. Once that is underway, this fix will be useful, but it will break pipelines running with older versions.

@LuckyMD
Copy link
Collaborator

LuckyMD commented Mar 22, 2022

Yes, ideally R 4 as soon as possible.

@lazappi
Copy link
Member

lazappi commented Mar 23, 2022

Just FYI R 4.1 is the current version and R 4.2 comes out in a month. I would probably just skip ahead to whatever the current version is when you get around to this.

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.

4 participants