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

Bugfix to as.SingleCellExperiment #4532

Merged
merged 4 commits into from
May 28, 2021

Conversation

bbimber
Copy link
Contributor

@bbimber bbimber commented May 25, 2021

This is related to the issue I opened here: #4531, which includes a simple repro case.

Whenever I run as.SingleCellExperiment(), it gives errors like this:

Error in value[3L] :
invalid subscript 'e' in 'altExp(, type="character", ...)':
'RNA' not in 'altExpNames()'

I believe the issue is that the 4.0.2 version of as.SingleCellExperiment tries to call SingleCellExperiment::swapAltExp() when passing the name of the currently active experiment.

I think that worked in as.Seurat.SingleCellExperiment(), since you inject "originalexp" into the object, so iterating the true assay names works.

@bbimber
Copy link
Contributor Author

bbimber commented May 25, 2021

Note: this adds a test case to illustrate the failure

Check if SingleCellExperiment is installed before checking
Kill tests that require Bioc 3.13/R 4.1
@mojaveazure mojaveazure linked an issue May 28, 2021 that may be closed by this pull request
@mojaveazure
Copy link
Member

Thanks for taking care of this! The issue is not entirely with Seurat v4.0.2, but also with SingleCellExperiment 1.14.1 (Bioc 3.13) changing how they handle the primary altExp. Prior versions of SingleCellExperiment had all alternate experiments stored together as altExps, but the latest version moves the primary altExp to mainExp

I've made some modifications to your tests:

  1. I've set the test to skip on CRAN since CRAN doesn't always have Bioc packages installed or have weird versions installed (eg. Bioc devel) on their testing servers and we don't want to be bogged down by missing/incorrect Bioc packages.
  2. I've also put a check in the tests to only run the test when SingleCellExperiment is installed: SingleCellExperiment is a suggested package, which means it's not always installed when Seurat is; we don't want our tests to fail on a missing suggested package.
  3. Finally, I've commented out the tests that are specific to SingleCellExperiment 1.14.1; the latest version of SingleCellExperiment is specific to R 4.1 (Bioc 3.13 is specific to R 4.1), and since we still support R 4.0 we don't want our tests to fail because of SingleCellExperiment's requirements. We'll change our own requirements to stay in line with our dependencies (any package listed in Depends or Imports in DESCRIPTION), but not suggested packages.

@bbimber
Copy link
Contributor Author

bbimber commented May 28, 2021

@mojaveazure OK, thanks. what's your release schedule for bugfixes like this?

@mojaveazure
Copy link
Member

mojaveazure commented May 28, 2021

Once the CI passes, we'll get this merged into the develop branch. However, we cannot submit to CRAN right away as their policies require a minimum of two weeks between submissions, and they prefer a month (limited server capacity, especially for large packages such as Seurat). Given that Seurat v4.0.2 was just released last week, it will likely be another 2-3 weeks before we start considering another release to CRAN.

@mojaveazure mojaveazure added the bugfix Fixes a bug label May 28, 2021
Update changelog
@bbimber
Copy link
Contributor Author

bbimber commented May 28, 2021

@mojaveazure OK. While I have you, have you noticed this new warning (which seems like a simple bug): satijalab/seurat-object#13

@mojaveazure
Copy link
Member

mojaveazure commented May 28, 2021

The warning Warning: The following arguments are not used: row.names is expected. When we moved from Seurat v2 to v3, a lot of functions had very slight parameter changes. The ... were introduced to many functions since we moved from a pure functions generics/methods, allowing us to introduce new specialized routines for assays vs dimensional reductions, as well as specialized assays (eg. SCTAssay, Signac::ChromatinAssay).

Feedback we got from this switch was that when users passed a v2 parameter to a v3 function, that parameter was silently ignored and the v3 default was silently used instead. In order to make it clear when a v2 parameter was ignored, we introduced CheckDots to warn when a parameter was passed to a function via ..., but not used in a function. This helped users transition from the v2 parameters to the v3 parameters, and this infrastructure will be left in place to allow us to change parameters again when we need to (we will strive to have this happen only on major or minor releases, never a patch release, following the MAJOR.MINOR.PATCH version numbering scheme)

The reason this specific warning is happening is that we introduced the ability to use data.frame-derivatives that don't have row names (eg. tibble, data.table) when creating Seurat objects. As Seurat requires all assay slots to be a matrix or dgCMatrix, we convert any non matrix/dgCMatrix objects to a dgCMatrix. This doesn't work for tibble/data.table as they lose feature names since they don't have rownames. As such, as.sparse.data.frame gained a row.names parameter to allow users to pass feature names to CreateSeuratObject (which eventually calls as.sparse) to ensure feature names are preserved. Since this argument is only used for the data.frame method, all other as.sparse methods throw this warning. We are looking into finding a way to ignore this specific warning, haven't been able to do so yet.

It is not a bug, so it's a lower priority for us, but we understand the confusion and will work to address it

@mojaveazure mojaveazure merged commit b83acea into satijalab:develop May 28, 2021
@bbimber bbimber deleted the SingleCellExperiment branch May 29, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

as.SingleCellExperiment Fails in Seurat_4.0.2
2 participants