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.

Sign up for GitHub

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

Jump to bottom

Add support for assays stored as SeuratObject::Assay5 #23

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Add support for assays stored as SeuratObject::Assay5 #23

merged 3 commits into from
Dec 6, 2023

Conversation

esiegel
Copy link
Collaborator

@esiegel esiegel commented Dec 3, 2023 •
edited
Loading

With Seurat version 5, now assays can be stored either as class Assay or Assay5. The way in which data is stored has changed between versions. This PR adds support for reading the counts sparse matrix from either formats.

fixes #22

esiegel
esiegel commented Dec 3, 2023
View reviewed changes
DESCRIPTION Outdated
@@ -14,7 +14,8 @@ Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Suggests:
testthat (>= 3.0.0),
Matrix
Matrix,
SeuratObject
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used in the tests to create a Seurat dataset with a v5 assay. running devtools::check complains if we don't add this as a dependency. For test dependencies it is recommended to add these to suggests

R/util.R
#' @importFrom methods as
#'
#' @export
counts_matrix_from_assay <- function(assay) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this method to get the counts matrix from either of the assay versions instead of updating the select_assay method to just return the counts matrix directly. Doing this so that I don't break anyone's code if they are relying on select_assay

R/util.R Outdated
# Seurat::Assay5 was introduced with Seurat-5.0 and stores the data completely differently
# within the `layers` slot.
if (is(assay, "Assay5")) {
assay <- suppressWarnings(methods::as(object = assay, Class = 'Assay'))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During conversion using as lots of warning pop up if the assay is missing fields even thought the conversion still succeeds.

Another alternative would be to do something like assay@layers[["counts"]] as it seems this is where the counts matrix is stored. By using the as conversion method we don't have to worry if the location changes.

conversion logic can be seen here

tests/testthat/test-util.R
@@ -4,15 +4,29 @@ test_that("select_assay selects active assay", {
rna3 <- create_count_mat(5, 5)

obj <- Seurat::CreateSeuratObject(rna1, assay="rna1")
obj[["rna2_"]] = Seurat::CreateAssayObject(rna2, key="rna2_")
obj[["rna3_"]] = Seurat::CreateAssayObject(rna3, key="rna3_")
obj[["rna2"]] = Seurat::CreateAssayObject(rna2, key="rna2_")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change, but the names here didn't need to have an underscore even though the key parameter to CreateAssayObject requires it

tests/testthat/test-util.R

obj <- Seurat::CreateSeuratObject(rna1, assay="rna1")
obj[["rna2"]] = Seurat::CreateAssayObject(rna2, key="rna2_")
obj[["rna3"]] = SeuratObject::CreateAssay5Object(rna3, key="rna3_")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Seurat package doesn't export the underlying v5 create method like it does with the non v5.

esiegel
esiegel commented Dec 3, 2023
View reviewed changes
DESCRIPTION Outdated
Config/testthat/edition: 3
Imports:
methods,
Seurat,
Seurat (>= 5.0.0),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to support v5 datasets, we need to bump the min version of Seurat. Also, have a direct dependency on SeuratObject because I think using the LayerData method directly reads better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the Imports on Seurat unversioned and Suggests >= 5.0.0? I am not sure if Roxygen will let you specify it that way, but I imagine cutting to Seurat v5 will trigger a lot of support tickets

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, making v5 a requirement is probably going to be an issue in the near term.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will put in effort to not make this a requirement

@esiegel esiegel requested a review from jsicherman December 4, 2023 21:43
jsicherman
jsicherman reviewed Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@jsicherman jsicherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits. I do think it's worth thinking about how to not require users to update to Seurat v5, as (at least to me) it is kind of a painful update with a long dependency chain

DESCRIPTION Outdated
Config/testthat/edition: 3
Imports:
methods,
Seurat,
Seurat (>= 5.0.0),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the Imports on Seurat unversioned and Suggests >= 5.0.0? I am not sure if Roxygen will let you specify it that way, but I imagine cutting to Seurat v5 will trigger a lot of support tickets

tests/testthat/test-util.R
Comment on lines +19 to +25
rna1 <- create_count_mat(5, 5)
rna2 <- create_count_mat(5, 5)
rna3 <- create_count_mat(5, 5)

obj <- Seurat::CreateSeuratObject(rna1, assay="rna1")
obj[["rna2"]] = Seurat::CreateAssayObject(rna2, key="rna2_")
obj[["rna3"]] = SeuratObject::CreateAssay5Object(rna3, key="rna3_")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rna1 <- create_count_mat(5, 5)
rna2 <- create_count_mat(5, 5)
rna3 <- create_count_mat(5, 5)
obj <- Seurat::CreateSeuratObject(rna1, assay="rna1")
obj[["rna2"]] = Seurat::CreateAssayObject(rna2, key="rna2_")
obj[["rna3"]] = SeuratObject::CreateAssay5Object(rna3, key="rna3_")
rna <- create_count_mat(5, 5)
obj <- Seurat::CreateSeuratObject(rna, assay="rna1")
obj[["rna2"]] = Seurat::CreateAssayObject(rna, key="rna2_")
obj[["rna3"]] = SeuratObject::CreateAssay5Object(rna, key="rna3_")

R will pass function arguments by value rather than reference so you shouldn't need to construct multiple instances yourself

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually want to create three different matrices because I want their values to be different for the tests. Turns out R is copy by value, but only if the value is modified.

R/util.R Outdated
# The same as using the helper `assay$counts` but less indirection.
# With Assay5 it is important to use this method as oppossed to grabbing
# the data from `assay@layers` directly as that matrix won't contain the dimnames
SeuratObject::LayerData(assay, "counts")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the above suggestion about keeping Seurat dependency unversioned, you could probably...

if (unlist(packageVersion("Seurat"))[1] > 5) {
  SeuratObject::LayerData(assay, "counts")
} else {
  assay@counts
}

Although I am not sure how that would interact with NAMESPACE

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this:

if (packageVersion("Seurat") >= 5) {
  SeuratObject::LayerData(assay, "counts")
} else {
  if (is(assay, 'Assay5')) {
    stop("Cannot get count matrix: Please upgrade to Seurat > 5 to support dataset")
  }

  assay@counts
}

It looks like it is possible to have a Seurat object with a mix of assay versions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way they get into the else if is if they download a Seurat 5 dataset while on Seurat < 5, right? I think that is a fine condition to stop on. Make sure to unlist the return of packageVersion, though (it is a list with each entry being a corresponding x/y/z)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is my understanding

@esiegel esiegel requested a review from jsicherman December 5, 2023 17:30
@esiegel esiegel merged commit 6988243 into main Dec 6, 2023
3 checks passed
@esiegel esiegel deleted the esiegel/support_assay5 branch December 6, 2023 18:09
@esiegel esiegel mentioned this pull request Jan 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenwilliams22 stephenwilliams22 stephenwilliams22 approved these changes

@jsicherman jsicherman Awaiting requested review from jsicherman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Assay5 assays stored in seurat objects
3 participants
@esiegel @stephenwilliams22 @jsicherman