Add SpatialExperiment support, fix SCE normalization, and add optional plaid backend - #181
Merged
Conversation
Per issue #180
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #180 and closes #167.
Two things here. The first is a bug fix that turned out to be broader than reported:
escape.matrix(normalize = TRUE)was failing on everySummarizedExperiment-derived input, not justSpatialExperiment. The second is the optionalplaidbackend, plus theinput.assayargument it needed.Part 1 — Spatial / SCE normalization (#180)
The working hypothesis in the issue was close but not exact. Reproducing it surfaced three distinct defects, all confirmed on
pbmc_smallbefore any fix was written.1.
.pull.Enrich()selected the wrong altExpR/utils.Rread:altExp(sc)with noeargument returns the first altExp, and[[name]]on the resultingSummarizedExperimentdispatches to the colData accessor, not the assay list. For a name like"escape_normalized"that returnsNULL, giving the reportedassay(NULL)S4 dispatch error. Now usesaltExp(sc, name), which is the form.cntEval()already used a few lines away.This was never spatial-specific. It reproduces on a plain
SingleCellExperiment; Seurat and raw-matrix input were unaffected, which is why it went unnoticed. It also brokeperformPCA()on any SCE — not reported, but fixed by the same change.2. Underscored gene-set names were silently dropped
performNormalization()rewrote every_to-unconditionally to match Seurat's feature-name coercion, then subset the gene sets to names matching the enrichment columns. For matrix, SCE, or SPE input the columns keep their underscores, so everyHALLMARK_*,GO_*, andREACTOME_*set matched nothing:Matching is now literal first, with the Seurat-mangled form as a fallback for the columns still unmatched, and per-set scale factors are aligned to
colnames(enriched)explicitly with an assertion.3.
%||%was used but never definedbase::%||%only exists in R >= 4.4.0, whileDESCRIPTIONdeclaresR (>= 4.1). Now defined internally.Also in this part
escape.matrix(normalize = TRUE)no longer attaches an altExp only to pull it straight back off — it stays on the matrix path for every input class. That round trip was the mechanism of the original failure.performNormalization()gives a suppliedenrichment.dataprecedence over scores stored on the object, and warns when both exist.runEscape()callsescape.matrix()with named rather than positional arguments..split_cols(),.match_summary_fun(), and.filter_genes().Part 2 — plaid backend (#167)
backenddefaults to"native"; nothing changes for existing users.methodgains"PLAID","singscore","scSE". These have no native implementation, so they route to plaid regardless ofbackendand say so.backend.argscarries method-specific tuning (alpha,tau,rowtf,aucMaxRank,rmax,nsmooth,stats,chunk,removeLog2,scoreMean), validated against the target function's formals so a typo errors rather than being swallowed.Design notes worth reviewing
New arguments sit after
.... That makes them name-only, so no positional caller can break and partial matching can't landbackend.a =onbackend.args.plaid bypasses escape's chunk loop entirely. Three reasons, in severity order: several plaid scores are not column-separable (median normalization,
rowtf = "ecdf", rank statistics), so chunking would make results depend ongroupsand break the invariance test; plaid forks internally viaparallel, and nesting that insideMulticoreParamforks inside forks; and plaid's speed is the single large sparsecrossprod. plaid owns the cores on that path, and a non-SerialParamBPPARAMis ignored with a message.max.genesis the main trap and is handled explicitly. plaid defaults tomax.genes = 500, which would silently drop most Hallmark, GO, and Reactome sets. escape sets a real cap above anything scoreable rather than trusting a sentinel, and independently checks the returned rows — any set requested but not returned warns by name, and all sets missing is an error. That holds even if plaid changes its filtering rules.The two
normalizearguments stay separate. plaid's (median normalization of scores, inside the calculation) is reachable only asbackend.args$normalizeand keeps plaid's own default. escape's (drop-out scaling, after scoring) is untouched. Both are messaged when they could interact.replaid.ssgseais documented as exact atalpha = 0. Compared directly againstGSVA::gsva()on identical input it correlated at 0.85, not 1. The input assay is not the explanation — native ssGSEA on counts versus logcounts correlates at exactly 1.0, so the entire gap is plaid vs GSVA. Onpbmc_small(230 genes) it drops to 0.50.The vignette, roxygen, and NEWS all state the measured numbers and frame
backend = "plaid"as a fast screen rather than a drop-in replacement. The concordance test threshold is set at 0.6 with a comment that it must not be tightened into an equivalence claim. Every score matrix carries anescape.backendattribute, andrunEscape()mirrors it intometadata()/Misc()so provenance travels with the object.Speed on the real Xenium object (166k spots, UCell, three sets): 28.6 s native → 6.8 s plaid, 4.2×. Real, but not the 10–100× the paper reports for this shape of data.
Part 3 —
input.assayplaid expects log-space input, but
.cntEval()could only reach the maincountsassay — every other name was treated as an altExp."auto"(default) reads counts for the native backend and logcounts for plaid."counts"and"logcounts"resolve to the right place for both object families (Seurat'scounts/datalayers, a SummarizedExperiment'scounts/logcountsassays), so the same call works across classes. Anything else is taken literally.The
.cntEval()change is one line and provably inert: all six in-packageassay = "RNA"call sites passtype = "counts".performNormalization()deliberately still reads raw counts regardless ofinput.assay— its scale factor counts detected genes, and detection is identical in count and log space. That is documented rather than left implicit.Errors name the fix (
Seurat::NormalizeData(),scuttle::logNormCounts(), orinput.assay = "counts") and tolerate Seurat v5 split layers.Docs
escape.matrix()/performNormalization()argument notes. Renders clean.Authors@Rasctb.test-coverage.yamlpreviously installed nothing extra, so it would have skipped every new test.