Repository navigation
Six fixes, including the FOV crash and the two failing tests on main (replaces six PRs) - #10492
Open
BenjaminDEMAILLE wants to merge 14 commits into
Open
BenjaminDEMAILLE wants to merge 14 commits into
BenjaminDEMAILLE wants to merge 14 commits into
Conversation
Radius.VisiumV2 is defined and correct, but its roxygen block carries `@method Radius VisiumV1`, copied from the block above it. Roxygen therefore emitted S3method(Radius,VisiumV1) twice and never registered VisiumV2, so NAMESPACE has no S3method(Radius,VisiumV2) entry. The consequence is silent rather than loud. VisiumV2 extends FOV extends SpatialImage, so Radius() on a VisiumV2 image walks past the unregistered method and lands on SeuratObject's Radius.SpatialImage, which returns NULL. Callers get NULL instead of the scaled radius: Radius(image, scale = "lowres") #> NULL (Radius.VisiumV2 gives 0.0076999) SpatialPlot then takes its fallback branch, since `length(spot.radius) == 1L` is FALSE, and draws every spot at a fixed 1 mm instead of a radius scaled to the image. Visium V2 and Visium HD plots lose their spot scaling with no warning. Fix the tag and add the NAMESPACE entry. The existing Read10X_Image test already asserted the scaled radius and was failing on main; it now passes, and a test is added that names the mechanism so a future regression is diagnosed rather than merely observed. test_load_10X.R: 5 failures before, 0 failures and 28 passing after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AggregateExpression() and AverageExpression() rename identities that
contain a colon:
pbmc_small$letter.idents <- paste0('anything:', pbmc_small$letter.idents)
colnames(AggregateExpression(pbmc_small, group.by = 'letter.idents')$RNA)
#> "A_anything" "B_anything" # expected "anything:A" "anything:B"
CreateCategoryMatrix() builds the grouping with sparse.model.matrix(),
whose column names concatenate the 'data[, i]' term with that variable's
level and join the variables with ':'. It then recovered the levels by
stripping the terms and splitting on ':' -- which also splits any colon
belonging to a level, and the pieces are then reversed and rejoined with
'_'. Hence "anything:A" -> c("anything", "A") -> "A_anything".
Split on the 'data[, i]' markers instead, so a variable boundary is
identified by the marker that creates it rather than by a character a
level may legitimately contain.
Verified byte-identical to main for names without colons: single and
multiple grouping variables, three-way grouping, underscore replacement,
and AverageExpression.
Note this also covers grouping by several variables, where a colon in one
level was equally lost; a single-variable special case would not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On an object whose image is an FOV (Xenium, Vizgen, VisiumV2), the R
session aborts:
Assertion failed: (this->rows()>0 && this->cols()>0 &&
"you are using an empty matrix"), function redux, file Redux.h
On a build without assertions that is the segfault reported in satijalab#10099.
GetTissueCoordinates() returns those images' cell identifiers in a 'cell'
column, with integer row names. The assay method selects cells with
cells <- rownames(spatial.location)
so it selects none, LayerData() returns a matrix with zero columns, and
RowVar() reduces over it. The reporter had already worked this out; this
change acts on it.
Four changes, cause first then the crash itself:
- FindSpatiallyVariableFeatures.Seurat() moves the 'cell' column into the
row names before passing the coordinates on. That column is not a
coordinate either: leaving it in is what coerces the frame to character
inside dist(), which is the "NAs introduced by coercion" of satijalab#9087.
- RowVar() returns NA per row when given fewer than two columns, matching
stats::var(), instead of asserting. It is exported, so any caller could
take the session down; a guard at one call site is not enough. Verified
identical to apply(x, 1, var) for 0, 1 and n columns. Probing the other
reductions in data_manipulation.cpp with an empty matrix found RowVar to
be the only one affected.
- The assay method raised "promise already under evaluation" whenever
nfeatures was not passed, because its default was `nfeatures = nfeatures`.
- Filtering by variance dropped to a vector with a single surviving
feature, and markvario() returns one variogram rather than a list of
them for a single mark, so ComputeRMetric() then iterated over its
elements.
Run against main, the new test file does not report failures: it aborts
the R process at the second test. With this change all 7 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The usage section recorded 'nfeatures = nfeatures' verbatim, so R CMD check reports a codoc mismatch once the default is a real value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TransferData() fails when any cell's prediction scores are all NaN:
Error in possible.ids[apply(X = prediction.scores, MARGIN = 1,
FUN = which.max)] : invalid subscript type 'list'
The reporter had the mechanism right. which.max() returns integer(0) for a
row that is entirely NaN, so the rowwise apply() returns a ragged list
rather than a vector, and using that list as an index fails:
which.max(c(NaN, NaN)) # integer(0)
apply(scores, 1, which.max) # a list, once any row is all NaN
ids[.] # invalid subscript type 'list'
A cell with no usable score has no defensible label, so it now gets none:
the index is NA and predicted.id is NA. A warning reports how many cells
that covers, since silently unlabelled cells would be its own trap, and
the question in the report was what a NaN score means.
The rowwise selection moves into .MaxScoreIndex() so it can be tested
directly, rather than only through a data condition that is awkward to
provoke on demand.
Against origin/main the new tests report 5 errors; with this change all 13
pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
selection.method defaults to c('markvariogram', 'moransi') and nothing
resolved it, so calling FindSpatiallyVariableFeatures() without the
argument passed a length-2 value on: switch() in the default method, and
`if (selection.method == "markvariogram")` in the assay method, which
errors with "the condition has length > 1".
match.arg() in each of the three methods, which is what the signature
already implies, and which also rejects an unknown method before any work
is done rather than after the variogram is computed.
Fixes satijalab#7020.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways DotPlot() fails on input it should handle. A feature named twice, which happens as soon as marker lists are combined, becomes a duplicated factor level: > DotPlot(object, features = c(markers, markers[1])) Error in `levels<-`: factor level [4] is duplicated The features are only there to fix the order of the axis, so take them unique. VlnPlot() already tolerates this. Cells with no identity come back from CellsByIdentities() as NA. Those NAs reach FetchData(), and the per-group frames then fail to bind: > DotPlot(object, features = markers) Error in `rbind`: duplicate 'row.names' are not allowed Drop them, and say how many were left out. Both the cell names and the identity column are filtered, so this holds whether or not the NA padding from CellsByIdentities() is fixed as well (satijalab/seurat-object#302). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LeverageScore() sizes its decomposition to the object: nv <- ifelse(nrow(x = object) < 50, nrow(x = object) - 1, 50) and then does not use it: Z <- irlba(A = object, nv = 50, nu = 0, verbose = FALSE)$v so 50 components are always requested, and anything with fewer than 50 features or cells stops with Error: max(nu, nv) must be strictly less than min(nrow(A), ncol(A)) That takes SketchData() with it, which is how it is usually reached: SketchData() on pbmc_small fails at every size of sketch. Use nv, bounded by both dimensions, and say so when there is nothing to decompose at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FindSpatiallyVariableFeatures() takes its coordinates from a single image: image <- image %||% DefaultImage(object = object) A merged object has one image per sample, so the analysis covers only the cells of whichever image happens to be first, and says nothing about it. Depending on the image class, this either quietly analyses a part of the object, or fails deeper down with "Please provide the same number of observations as spatial locations" (satijalab#9092). This object has 2 images ('fov.a', 'fov.b'); using 'fov.a', which covers 40 of the 80 cells in assay 'Spatial'. Pass image = to choose another, and run each image separately to cover them all An object with one image says nothing, as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… group/seurat-misc # Conflicts: # NEWS.md
…o group/seurat-misc # Conflicts: # NEWS.md
…nto group/seurat-misc # Conflicts: # NEWS.md
… into group/seurat-misc # Conflicts: # NEWS.md
This was referenced Sep 1, 2026
…nto group/seurat-misc # Conflicts: # NEWS.md
BenjaminDEMAILLE
changed the title
This was referenced Sep 1, 2026
BenjaminDEMAILLE
marked this pull request as ready for review
September 1, 2026 06:38
This was referenced Sep 1, 2026
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.
Draft, and grouped on purpose. This replaces six separate PRs. I have closed the ones it supersedes: #10454, #10455, #10457, #10461, #10478, #10482. The individual commits are preserved, so a single one can be dropped without losing the rest.
FindSpatiallyVariableFeaturescrashmainthis aborts the R session on an FOV-based object: an Eigen assertion inRowVar, and a segfault in a CRAN build without assertions.RowVarnow returnsNA_REALper row below two columns instead of reducing over an empty range; zero-variance features are dropped beforeloess,nfeaturesis passed through rather than defaulting to its own argument name, and thecellcolumn moves to rownames so the coordinate frame is numeric (#10099, #7020, #9092)RadiusonVisiumV2test_load_10X.Rreports two failures onmainright nowAggregateExpressionTransferDataDotPlotLeverageScoreirlbafor 50 components regardless of size, soSketchDatafails on any small objectThe C++ guard is the part worth reviewing carefully: the crash is reachable from any caller that hands
RowVara single-column matrix, not only this one.Verification
Each commit carries a test that fails without its fix; the crash test runs in a separate process, since without the fix it takes the session down. Full suite on the merged branch: 1225 passing, zero failures, which is the suite with the two
Read10X_Imagefailures resolved.🤖 Generated with Claude Code