Add droplevels for meta.data in UpdateSeuratObject

[samuel-marsh]

Hi Seurat Team,

Quick PR here. Just adds droplevels to UpdateSeuratObject. I chose to add directly to UpdateSeuratObject though it could also be incorporated into droplevels.Seurat though that adds/changes functionality of that function so didn't do that for now in case that's not desired.

The other place I think this could be helpful would be in subset.Seurat so that factor levels in meta.data are updated based on remaining values. I held off on that for now because wasn't sure that functionality was desired though if team thinks it would be good there too I'm happy to add second commit.

Thanks again for all you do maintaining the package!
Best,
Sam

[dcollins15]

Thanks @samuel-marsh, this LGTM 🚀

I chose to add directly to UpdateSeuratObject though it could also be incorporated into droplevels.Seurat though that adds/changes functionality of that function so didn't do that for now in case that's not desired.

It's definitely odd that droplevels.Seurat only updates the object's Idents, but I agree that this would be a pretty significant change to that method's functionality.

The other place I think this could be helpful would be in subset.Seurat so that factor levels in meta.data are updated based on remaining values. I held off on that for now because wasn't sure that functionality was desired though if team thinks it would be good there too I'm happy to add second commit.

I'm on a bit of a tare ATM so I'll probably push these changes up in a follow-up PR shortly.

[jan-glx]

I think #251 should be reverted. This behavior is in striking contrast to the rest of the R world, e.g. subset.data.frame. (Despite this causing confusion for people unfamiliar with factors, see e.g. satijalab/seurat#5069)

If dropping unused levels for all factors in droplevels.Seurat is considered a significant change than doing that in subset certainly is!

I also don't understand the motivation for adding this to UpdateSeuratObject. I think this should be implemented in droplevels.Seurat and in droplevels.Seurat only. When someone is using a factor rather than character they might have their reasons - don't mess with them.

I suggest you instead implement the except argument for droplevels.Seurat, with a warning that behavior will change from colnames(x[[]])!="Idents" to NULL and that they should set either to avoid that warning.

[anashen]

@jan-glx I appreciate you taking the time to bring this to the team's attention (this change was introduced before I joined). I do agree with you that dropping unused levels is not behavior to expect by default when subsetting or updating an object, and rather should be a decision left to the user, which has been suggested in previous issues. I'm leaning towards reverting the change; however, it may take some time to see it updated here since we have some items higher on the priority list. Thanks!

[jan-glx]

I have submitted PR #262 to revert #251 - that is the addition of droplevels to subset.

#247 - that is the addition of droplevels to UpdateSeuratObject should be reverted too but is of low priority to me and perhaps @samuel-marsh can share the original motivation for it before the revert.

Implementing the suggested change in droplevels.Seuratis a bit more work without risking breaking older code and not a priority as it can easily be achieved by so[[]] <- droplevels(so[[]]) already.

In the meantime I am using below monkey-patch to avoid issues caused by #251 but downgrading and waiting is likely a better option.

# monkey-patch SeuratObject:::subset.Seurat to drop the droplevels(meta.data) line
patch_subset_Seurat_remove_droplevels <- function() {
  ns <- asNamespace("SeuratObject")
  
  # keep a backup to allow restore
  if (!exists(".subset.Seurat_original", envir = .GlobalEnv, inherits = FALSE)) {
    .subset.Seurat_original <<- get("subset.Seurat", envir = ns, inherits = FALSE)
  }
  
  f <- get("subset.Seurat", envir = ns, inherits = FALSE)
  ch <- deparse(body(f), width.cutoff = 500)
  
  # drop any line that calls droplevels(meta.data) (robust to spaces)
  keep <- !grepl(
    "meta\\.data\\s*<-\\s*droplevels\\s*\\(\\s*meta\\.data\\s*\\)",
    ch
  )
  
  if (all(keep)) {
    message("No droplevels(meta.data) line found; nothing to patch.")
    return(invisible(FALSE))
  }
  
  new_body <- parse(text = paste(ch[keep], collapse = "\n"))[[1]]
  f2 <- f
  body(f2) <- new_body
  
  # replace in namespace (temporarily unlock binding if needed)
  assignInNamespace("subset.Seurat", f2, ns)
  
  # ensure S3 lookup sees the new definition
  # (usually not required, but cheap and safe)
  registerS3method("subset", "Seurat", f2, envir = ns)
  
  message("Patched SeuratObject:::subset.Seurat (removed droplevels(meta.data)).")
  invisible(TRUE)
}

patch_subset_Seurat_remove_droplevels()

[samuel-marsh]

Hi All,

The motivation was to provide quick way (within safety of another Seurat functions) to drop levels of factors in meta.data slot that were no longer relevant (as can happen after subset). I chose UpdateSeuratObject because I thought it best fit within the behavior of that function (checking object validity and updating where needed). Personally I think it’s still useful there but no objections if dev team would like to revert that as well.

Best,
Sam