[BenjaminDEMAILLE]

Fixes #186.

The bug

When some cells have an NA identity, every identity group picks up an NA entry per unidentified cell:

Idents(object) <- c(rep("A", 4), rep("B", 4), NA, NA)
lengths(CellsByIdentities(object))
#>  A  B NA
#>  6  6  2      # should be 4 4 2

CellsByIdentities() selects each group with

cells[as.vector(Idents(object)[cells]) == i]

Comparing an NA identity with a level gives NA, and indexing with NA returns an NA element. The unidentified cells are also collected correctly into their own "NA" group a few lines further down, so they were being counted twice: once as themselves, once as an NA inside each real group.

Anything that consumes the result then carries those NAs along, and group sizes taken from it are wrong.

The change

which(), exactly as suggested in the report.

Tests

New tests/testthat/test_cells_by_identities.R: NA identities no longer appear inside the other groups and the group contents are the expected cells, identities without any NA are unaffected, every cell is accounted for exactly once across all groups, and restricting to a subset of cells behaves the same way.

Against main: 7 failures. With this change: 13 passes. Full suite 189 passing, R CMD check Status OK.

🤖 Generated with Claude Code

[BenjaminDEMAILLE]

A user-visible consequence of this one, found while sweeping Seurat's plotting functions: DotPlot() on an object with any NA identity fails with

Error in rbind: duplicate 'row.names' are not allowed

because the NA padding in the group returned by CellsByIdentities() reaches FetchData() as cell names.

I have sent satijalab/seurat#10478 for the Seurat side, which filters both the cell names and the identity column, so it holds either way. The two are complementary: with this PR but without that one the same call still fails, differently (replacement has 1 row, data has 0), and with that one but without this the padding is still there for anything else that calls CellsByIdentities().

---

Outside contribution: I am not a member of the Seurat dev team, and this comment is not an official answer from the maintainers.

[BenjaminDEMAILLE]

Superseded by #318, which groups the correctness PRs into one so that a single review covers the subject. The commit is carried over unchanged; closing here to keep the queue down rather than because anything was wrong with it.