Implement group.by arg to FindAllMarkers

[rharao]

I saw @AlbertoFabbri93's suggestion on #4775 and thought it was a good idea. Currently, one can't group the cells for FindAllMarkers by anything besides the object's idents.

[samuel-marsh]

Hi @rharao,

If you don't mind suggestion I would recommend adding two check addition checks to the PR, just for end users sake.

First, would be a check that group.by is a column in meta.data. If not then the whole object will get the same ident and cause error.
Second, would be accepting "ident" to mean the current object ident which fits with behavior of other package functions.

So overall something like:

if (!is.null(x = group.by)) {
  if (group.by != "ident") {
    if (!group.by %in% colnames(x = [email protected]) {
      stop(paste0(group.by, " is not found in object meta.data."))
    } else {
      Idents(object = object) <- group.by
    }
  }
}

Best,
Sam

[rharao]

@samuel-marsh I'm glad you mentioned this. I considered this, but these conditionals are the ones currently in place with FindMarkers.Seurat. In a future commit, I will implement a check in both functions that the supplied argument is in the colnames if it's a scalar.

One benefit to the minimally guarded assignment here is that the user can supply a vector of cell memberships that might not be part of the object metadata. I can add another conditional to allow this.

My opinion is that Idents<- should at least warn before recycling a scalar to use as idents, but that's another discussion.

[rharao]

@samuel-marsh I think this is now ready unless something else is noticed. I'd appreciate your review if you have time

[samuel-marsh]

Hi @rharao,

Things look ok to me. I’m not member of dev team though so can’t do the official review and merge.

Best,
Sam

[rharao]

@dcollins15 any additional thoughts?

[dcollins15]

Thanks for this @rharao! I was waiting to run this change by a couple of other lab members who're fairly opinionated about the FindAllMarkers interface—they're also enthusiastic about the update 🔥

I want to get this merged in ASAP so I can start the CRAN submission process this afternoon—do you mind if I rebase (just to keep the commit history linear) and then push up a couple of small tweaks?

I've got the following changes in the can:

• Adding a relevant test case to test_differential_expression.R
• Throwing a warning when node and group.by are both set informing the user that the latter parameter will be ignored.
• Updating the changelog
• Bumping the package version (5.1.0.9017 -> 5.1.0.9018)

[rharao]

Don't mind at all. Thanks a lot.

[dcollins15]

LGTM 🚀

[rharao]

looks like a copy-paste error here 😄

[dcollins15]

🤦

[dcollins15]

Not a big deal, I'll need to update NEWS.md as part of the final release anyways—thanks for the catch!

[rharao]

Thanks again @dcollins15 ❤️ I'm deleting the branch on my fork now
Coincidence: #9550 mod #4775 = 0