Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a bug in NMF-based cell annotation where normalizing an internal temporary Seurat object could unintentionally modify the returned query object’s normalization/data layers.
Changes:
- Refactors
.seeded_nmf_annotatonto normalize a minimal temporary query Seurat object built from counts, rather than normalizing the input query object directly. - Adds a regression test asserting that query
countsanddatalayers are preserved after NMF annotation. - Documents the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
R/annotate.R |
Avoids mutating the input query object during normalization for NMF by normalizing a separate temporary object. |
tests/testthat/test-AnnotateCells.R |
Adds regression coverage to ensure AnnotateCells(..., method="nmf") preserves existing normalization. |
CHANGELOG.md |
Notes the normalization-propagation fix in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| query_counts <- SeuratObject::LayerData(object, assay = query_assay, layer = "counts") | ||
| query_for_nm <- SeuratObject::CreateSeuratObject( | ||
| counts = query_counts, | ||
| assay = query_assay, | ||
| meta.data = data.frame(row.names = colnames(query_counts)) | ||
| ) |
There was a problem hiding this comment.
query_counts <- LayerData(object, assay = query_assay, layer = "counts") assumes a single counts layer exists. For Seurat v5 Assay5/PNAAssay5 objects (common after merge()), count layers are typically counts.1, counts.2, ... and code elsewhere in the repo calls JoinLayers() before accessing layer = "counts". Please make this robust for multi-layer assays (e.g., join layers on a temporary copy of the assay/object, or explicitly combine all counts.* layers) so AnnotateCells(..., method = "nmf") works with assay version v5 merged objects without mutating the input object.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
.seeded_nmf_annotatonwhere normalization of the internal temporarySeuratobject would propagate to the output.PNA-2475
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added regression tests to control for bug.
PR checklist: