Implement group.by arg to FindAllMarkers

NEWS.md

@@ -1,6 +1,8 @@
 # Unreleased
 
 ## Changes
+- Added `group.by` parameter to `FindAllMarkers`, allowing users to regroup their data using a non-default identity class prior to performing differential expression ([#9550](https://github.com/satijalab/seurat/pull/9550))
+#' performing differential expression (see example); \code{"ident"} to use Idents
 - Added `image.type` parameter to `Read10X_Image` enabling `VisiumV1` instances to be populated instead of instances of the default `VisiumV2` class ([#9556](https://github.com/satijalab/seurat/pull/9556))
 - Fixed `IntegrateLayers` to respect the `dims.to.integrate` parameter.
 - Added `stroke.size` parameter to `DimPlot` ([#8180](https://github.com/satijalab/seurat/pull/8180))

R/differential_expression.R

@@ -46,6 +46,7 @@ FindAllMarkers <- function(
   object,
   assay = NULL,
   features = NULL,
+  group.by = NULL,
   logfc.threshold = 0.1,
   test.use = 'wilcox',
   slot = 'data',
@@ -75,11 +76,25 @@ FindAllMarkers <- function(
     return.thresh <- 0.7
   }
   if (is.null(x = node)) {
+    if (!is.null(x = group.by) && !identical(x = group.by, y = "ident")) {
+      if (length(x = group.by) == 1 && ! group.by %in% colnames(x = [email protected])) {
+        stop("'", group.by, "' not found in object metadata")
+      }
+      Idents(object = object) <- group.by
+    }
     idents.all <- sort(x = unique(x = Idents(object = object)))
   } else {
     if (!PackageCheck('ape', error = FALSE)) {
       stop(cluster.ape, call. = FALSE)
     }
+    if (!is.null(group.by)) {
+      warning(
+        paste0(
+          "The `group.by` parameter for `FindAllMarkers` ",
+          "is ignored when `node` is set."
+        )
+      )
+    }
     tree <- Tool(object = object, slot = 'BuildClusterTree')
     if (is.null(x = tree)) {
       stop("Please run 'BuildClusterTree' before finding markers on nodes")
@@ -896,7 +911,7 @@ FindMarkers.DimReduc <- function(
 #' use all other cells for comparison; if an object of class \code{phylo} or
 #' 'clustertree' is passed to \code{ident.1}, must pass a node to find markers for
 #' @param group.by Regroup cells into a different identity class prior to 
-#' performing differential expression (see example)
+#' performing differential expression (see example); \code{"ident"} to use Idents
 #' @param subset.ident Subset a particular identity class prior to regrouping. 
 #' Only relevant if group.by is set (see example)
 #' @param assay Assay to use in differential expression testing
@@ -919,10 +934,13 @@ FindMarkers.Seurat <- function(
   reduction = NULL,
   ...
 ) {
-  if (!is.null(x = group.by)) {
+  if (!is.null(x = group.by) && !identical(x = group.by, y = "ident")) {
     if (!is.null(x = subset.ident)) {
       object <- subset(x = object, idents = subset.ident)
     }
+    if (length(x = group.by) == 1 && ! group.by %in% colnames(x = [email protected])) {
+      stop("'", group.by, "' not found in object metadata")
+    }
     Idents(object = object) <- group.by
   }
   if (!is.null(x = assay) && !is.null(x = reduction)) {

tests/testthat/test_differential_expression.R

@@ -366,12 +366,17 @@ test_that("BPCells FindMarkers gives same results", {
 
 # Tests for FindAllMarkers
 # -------------------------------------------------------------------------------
-results <- suppressMessages(suppressWarnings(FindAllMarkers(object = pbmc_small,pseudocount.use=1)))
-results.clr <- suppressMessages(suppressWarnings(FindAllMarkers(object = clr.obj, pseudocount.use=1)))
-results.sct <- suppressMessages(suppressWarnings(FindAllMarkers(object = sct.obj, pseudocount.use=1, vst.flavor = "v1")))
-results.pseudo <- suppressMessages(suppressWarnings(FindAllMarkers(object = pbmc_small, pseudocount.use = 0.1)))
 
 test_that("FindAllMarkers works as expected", {
+  pbmc_copy <- pbmc_small
+  Idents(pbmc_copy) <- "orig.ident"
+
+  results <- suppressMessages(suppressWarnings(FindAllMarkers(object = pbmc_small, pseudocount.use = 1)))
+  results.clr <- suppressMessages(suppressWarnings(FindAllMarkers(object = clr.obj, pseudocount.use = 1)))
+  results.sct <- suppressMessages(suppressWarnings(FindAllMarkers(object = sct.obj, pseudocount.use = 1, vst.flavor = "v1")))
+  results.pseudo <- suppressMessages(suppressWarnings(FindAllMarkers(object = pbmc_small, pseudocount.use = 0.1)))
+  results.gb <- suppressMessages(suppressWarnings(FindAllMarkers(object = pbmc_copy, pseudocount.use = 1, group.by = "RNA_snn_res.1")))
+
   expect_equal(colnames(x = results), c("p_val", "avg_log2FC", "pct.1", "pct.2", "p_val_adj", "cluster", "gene"))
   expect_equal(results[1, "p_val"], 9.572778e-13, tolerance = 1e-18)
   expect_equal(results[1, "avg_log2FC"], -6.030507, tolerance = 1e-6)
@@ -407,6 +412,10 @@ test_that("FindAllMarkers works as expected", {
   expect_equal(results.pseudo[1, "p_val_adj"], 2.201739e-10, tolerance = 1e-15)
   expect_equal(nrow(x = results.pseudo), 222)
   expect_equal(rownames(results.pseudo)[1], "HLA-DPB1")
+
+  # Setting `group.by` the group by parameter is equivalent
+  # to setting the object's `Idents` before running `FindAllMarkers`.
+  expect_equal(results.gb, results)
 })