theislab / zellkonverter

Conversion between scRNA-seq objects
https://theislab.github.io/zellkonverter/
Other
144 stars 27 forks source link

Fix compatibility with {anndata} #76

Closed rcannood closed 1 year ago

rcannood commented 1 year ago

This is a proposed solution for #75. This ensures that no implicit py_to_r(...) conversion occurs in AnnDataToSCE(...) using the disable_conversion_scope(...) helper function stolen from reticulate :)).

I apologise for the many commits; I couldn't install rhdf5 so I had to use the CI for running the full R CMD Check. If this PR gets accepted at some point, I think it makes sense to squash.

rcannood commented 1 year ago

Update: this PR works on ubuntu-latest but not on :

@lazappi Can you approve this PR so that the workflows can be run? Also, which of the above errors need to be fixed in this PR or can be considered a separate issue?

lazappi commented 1 year ago

I'm pretty happy with this, thanks for submitting it! I made some minor comments but they are mostly just questions I wanted some clarification on.

For the CI I'm kinda happy as long as it passes on one OS. The main thing is that it passes on the Bioconductor build system which tends to have less issues.

The numpy array type issue I think should be dealt with separately, your suggestion seems like a good starting point but we can look into to that more on the other issue. I will check that the changes here haven't made it more of a problem though.

lazappi commented 1 year ago

@rcannood I made the minor changes I commented on so I think this is almost good to go. Only thing left is an error with {anndata} loaded that I'm not sure how to fix.

It has to do with the conversion you added here https://github.com/theislab/zellkonverter/pull/76/files#diff-4c7ea0a920b659d712a5256b98d7e4072805ee394c295eb01c6ce7c990c37e8cR429-R432 using the example compatibility test in test-read.R. If {anndata} isn't loaded this works fine and the following call to .convert_anndata_list() catches a failed conversion but if {anndata} is loaded for some reason the conversion here is recursive and fails with an error (hopefully that made sense). Any suggestions? Adding support for converting these type 20 arrays as you suggested in #45 would be one option but that won't help if there is some other data type that is the issue.

lazappi commented 1 year ago

@rcannood I'm pretty happy this is working ok (and I have some other things I want to work on that need this) so I'm going to merge now. Thanks so much for all your help!

rcannood commented 1 year ago

Sorry that I was away the past few days, there were a few important deadlines and I lost track of my mails.

Thanks for making the necessary changes to get everything to work on your end!