Update sce_to_seurat function

[sjspielman]

This PR addresses certain aspects of this conversation about data integration: AlexsLemonade/sc-data-integration#123

We would like to make the sce_to_seurat() more flexible in which assay is chosen to include in the new Seurat object. This PR adds an argument assay_name, default "counts", which can be specified for this purpose. I also included a check to make sure the assay exists.

All tests are passing, but I did not write more tests for different assay names (but I can if requested!), but the code I wrote indeed still works fine for a "counts" assay as the passing tests reflect!

I also thought about whether I should update here as well and supply an argument assay = assay_name, but the default that is used here by Seurat is "RNA" which is generally expected, so I decided to leave it alone.

scpcaTools/R/sce_to_seurat.R

Lines 47 to 51 in a822885

	seurat_obj <- Seurat::CreateSeuratObject(counts = sce_counts,
	meta.data = coldata)
	# add rowdata and metadata separately adding it while creating leaves the slots empty without warning
	seurat_obj[["RNA"]]@var.features <- rowdata

Finally, I also reran devtools::document(), and in addition to the expected man pages update for this function, the sce_to_anndata man was updated, and the roxygen note in DESCRIPTION was bumped. I wonder if the docs were just out of date?

Finally #2, should I add myself to DESCRIPTION author field? This is a pretty small update so not sure where this change lands!

[allyhawkins]

I can't comment on these lines but I agree with your comment about changing the actual assay name in Seurat so that it corresponds to the assay name in the SCE object that is being used. I just don't like the idea of saying I want to convert the logcounts assay but then putting that in the RNA slot which corresponds to raw data usually. That just seems wrong. So I think we should add the assay = assay_name line to line 55 and then change line 59 to seurat_obj[[assay_name]]@var.features <- rowdata. I also think there should be a note in the docs stating that the assay converted is saved with that assay name in the Seurat object.

Doing this also makes me want to add further functionality to convert multiple assays, like both counts and logcounts. That's outside the scope of this PR and the need for what we're trying to do but I'm going to file an issue because I like the idea of being able to have both.

As far as authorship, feel free to add your self.

[allyhawkins]

Josh and I have been going back and forth on committing and removing this change because for some reason we have different versions but can't figure out why.

[sjspielman]

Happy to join in the party!

[sjspielman]

Finally #2, should I add myself to DESCRIPTION author field? This is a pretty small update so not sure where this change lands!

Please note this means "second finally," and the GH-linked PR is not relevant 😂

[sjspielman]

So I think we should add the assay = assay_name line to line 55 and then change line 59 to seurat_obj[[assay_name]]@var.features <- rowdata. I also think there should be a note in the docs stating that the assay converted is saved with that assay name in the Seurat object.

👍
I'm now also wondering if we'd want to have a default of using the Seurat-y "RNA" name if the default "counts" assay is pulled out?

[allyhawkins]

I'm now also wondering if we'd want to have a default of using the Seurat-y "RNA" name if the default "counts" assay is pulled out?

I like this idea! I would just add a check that if the assay name is "counts" to make it equal to "RNA" before making the seurat object.

It also looks like you will need to add in the assay_name argument to the test for this function and then change line 37 to bee expect_equal(rowdata_sce, seurat_object[[assay_name]]@var.features) for it to pass tests.

[sjspielman]

The test here was failing because I wrote docs for assigning "RNA" but then didn't actually do it (woops!)! So, the name was being assigned as "counts". The test should be passing now with properly assigning the name as "RNA" to go with counts. I updated the test to test both default and non-default assays. I don't think all the expect_*'s were necessary to go with the new test_that() call but they can't hurt and are super quick.

Also when I was running tests locally, I was getting this from git status :

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	tests/testthat/_snaps/
	tests/testthat/test_anndata.h5

I figure we almost certainly want to be ignoring these files which are created by the tests. So, I got a bit ahead of myself and made a .gitignore for it. Let me know if that's ok!

[allyhawkins]

I think I would also include an explicit check for the assay name being logcounts here? I don't know that you need the rest of the checks but they can't hurt to have since they should all pass if it's converted successfully.

[sjspielman]

Done in 6dd5790!

[allyhawkins]

once this passes checks this is good to go!