adding support for Seurat 4, allow exporting multiple matrices, stop …

[maximilianh]

• removing Seurat2 support and replace with UpdateSeuratObject() (discovered thanks to @matthewspeir, will save us countless hours of keeping our Seurat 2/3/4 conda environments updated if it works)
• adding support for Seurat 4 (Seurat4 renamed the field avg_logFC to avg_log2FC, which of course breaks every program using the Seurat marker output)
• allow exporting multiple matrices
• stop if on windows and matrix is too big, which is better than my previous approach
• creating one more function
• tested on pbmc_small from Seurat2, Seurat3, and Seurat4, yay!

[maximilianh]

Hi @andrewwbutler, for cells.ucsc.edu, we're typically getting Seurat .rds files from labs. As these are not very future-proof (see e.g. this PR, Seurat has a habit of changing function and field names without warning) and may be hard to load into scanpy etc, so we use this ExportToCellbrowser() function to convert them to .tsv.gz or .mtx.gz format, plus a file meta.tsv. But with more assays and more slots, this is getting more complicated. What is a good data exchange format these days? Should I move towards loom or h5ad? In the past, whenever I tried these, I ran into other problems (installation, missing support for some feature, e.g. marker tables, hard to get back into Seurat objects, hard to inspect with less or difficult to get just the genes out of them).... or this is a thing of the past? We always provide the original Seurat files, too, so Seurat users will be fine and the .tsv.gz is a fallback for everyone else. Is this something you think is OK or would you rather say "nah, just use format X for all datasets now". Should we push all datasets towards .h5seurat files? Many thanks in advance for any comments!

[maximilianh]

Hi @mojaveazure, you had criticized (rightfully) my weird way of supporting Seurat2 in the original code review, but at the time, due to my ignorance, I didn't see an alternative. This PR addresses your comments, and thanks to UpdateSeuratObject(), I was able to remove a huge chunk of code from this script. I simply didn't know about UpdateSeuratObject() before, sorry.

[maximilianh]

Also, @mojaveazure, is there a way to run automated tests on this? I remember you mentioned this before, but I have no idea how to set it up. Can I copy the CI from another package or module? I could run 1-2 commands each time the code has been modified. If it's difficult, nevermind, it's not essential for me, I am just curious.

[mojaveazure]

Hi Max,

First, we apologise for the avg_logFC/avg_log2FC confusion. We tried to make this change clear during the Seurat v4 beta and before the v4 release, but will try to make these breaking changes more apparent in the future.

What automated tests would you like to run? We run the cell browser vignette every time you submit a PR to check it, but we can look into supporting other tests if you want.

[maximilianh]

Hi, thanks! Do you remember where you announced the change? I guess I should simply subscribe to some email or newsfeed. For for my lack of knowledge about the release process, my R knowledge is pretty limited to Seurat. Are you sure that automated tests are run? On this page, the vingette does not work https://rdrr.io/github/satijalab/seurat-wrappers/man/CellBrowser.html And the vignette command has this line just before it: ## Not run:

…

On Thu, Jul 1, 2021 at 7:01 PM Paul Hoffman ***@***.***> wrote: Hi Max, First, we apologise for the avg_logFC/avg_log2FC confusion. We tried to make this change clear during the Seurat v4 beta and before the v4 release, but will try to make these breaking changes more apparent in the future. What automated tests would you like to run? We run the cell browser vignette every time you submit a PR to check it, but we can look into supporting other tests if you want. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACL4TMJZYJTCSIFPKZZKYLTVSNPPANCNFSM47HMJYZQ> .

[maximilianh]

@mojaveazure just pinging this again, there is no rush, I just stumbled over it again.

[maximilianh]

Please don't merge this just yet, I just realized that the idents() field name is not auto-detected here. I'll submit one more commit shortly.