Keep simulated spots in the order they came in - #31
Open
rcannood wants to merge 2 commits into
Open
Conversation
rcannood
added a commit
that referenced
this pull request
Jul 28, 2026
Sorting for the simulation is only half of it: the result has to be sorted back, or scdesign2 joins the group of methods whose output is in a different spot order than the real dataset and whose clustering_ari is therefore scored against mismatched spots. See #31.
This was referenced Jul 28, 2026
rcannood
added a commit
that referenced
this pull request
Jul 28, 2026
* order scdesign2 input by spatial cluster `simulate_count_scDesign2()` writes its output one cell type at a time, in contiguous blocks (`new_count[, llim:ulim] <- ...`), but scdesign2 was pulling the coordinates from the input in its original order. The datasets are not sorted by spatial_cluster -- MOBNEW starts 1,2,3,3,4,2,4,1 -- so every spot got the counts of an unrelated spot and the spatial signal was destroyed before the metrics ever saw it. It shows: scdesign2 came last on morans_I (25.7, against 1.8 for scdesign3_nb) and on svg_precision. Every other method already does `input[order(input$obs$spatial_cluster)]` first; this one now does too. Also make cell_type_sel and cell_type_prop agree: they are matched by position, but were built with `unique()` and `table()` respectively, which only coincide when the labels happen to sort the same way. Both now come from `unique()` on the sorted data, which also matches the block order for 10 or more clusters, where numeric and character sorting disagree. * update changelog * restore the input spot order in scdesign2 Sorting for the simulation is only half of it: the result has to be sorted back, or scdesign2 joins the group of methods whose output is in a different spot order than the real dataset and whose clustering_ari is therefore scored against mismatched spots. See #31. * update changelog
splatter, sparsim, symsim and zinbwave all sort their input by spatial_cluster and then hand back the sorted result, so the simulated dataset came out in a different spot order than the real one. `downstream` compares the two element-wise -- ARI and NMI survive a permutation of the cluster labels, but not one of the spots -- so clustering_ari and clustering_nmi were comparing mismatched spots for those four methods. * Sort the result back before writing, and take obs and var straight from the unsorted input. * Add `check_alignment()`, called from generate_sim_spatialcluster, which every method's output passes through on its way to the metrics. A method that gets this wrong now fails loudly instead of quietly scoring badly. * Drop the `match(rownames(x), rownames(y))` reorder this supersedes. It had its arguments the wrong way round -- `match(current, desired)` is the inverse permutation -- so it could only ever have scrambled a reordered output further. It never fired, because it was always applied to an already-aligned matrix.
rcannood
force-pushed
the
fix/simulated-spot-order
branch
from
July 28, 2026 13:59
1eece96 to
4122ed8
Compare
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.
Describe your changes
splatter,sparsim,symsimandzinbwaveall sort their input byspatial_clusterand then hand back the sorted result, so the simulated dataset comes out in a different spot order than the real one. Ran splatter against the test dataset in its published image:What that actually breaks
Only
clustering_ariandclustering_nmi, and it is worth being precise about why -- the four datasets are internally consistent, they are just presented in a different order than the real one:crosscor_cosine/crosscor_mantel-- Moran's I is computed within a single dataset from that dataset's own coordinates, so it does not care how the spots are ordered.svg_precision/svg_recall-- same,SPARK::sparkx()runs per dataset and the results are compared as gene sets.ctdeconvolute_rmse/ctdeconvolute_jsd--generate_jds()andgenerate_rmse()index by name (intersect(rownames(real), rownames(sim))), so they line up regardless.ks_*metrics are distributional.Clustering is the one place the two datasets are compared spot by spot. In
downstream:ARI is invariant to a permutation of the labels, not of the samples, so those two vectors have been compared in different orders. This is a different bug from the one scdesign2 had in #22 -- there the counts were scrambled against their own coordinates, which destroyed the spatial signal itself and hurt every spatial metric. Here the signal survives, it is just attached to the wrong reference.
What this does
obsandvarstraight from the unsorted input.check_alignment()tosrc/helpers/utils.R, called fromgenerate_sim_spatialcluster-- the one place every method's output passes through on its way to the metrics. A method that gets this wrong now fails loudly rather than quietly scoring badly.match(rownames(x), rownames(y))reorder that this supersedes. Worth flagging that it was never the safety net it looked like: the arguments are the wrong way round.match(current, desired)is the inverse permutation, so had a method ever reordered its output, that line would have scrambled it a second way rather than repairing it:check_alignment()is a guard, not a behaviour change -- I checked that it passes for all eleven methods as they stand after this PR, so nothing that currently runs starts failing:scdesign3_nb/scdesign3_poisson--ncelldefaults todim(sce)[2], and we do not pass it, so scDesign3 takesnewCovariate <- datand keeps the input rows and their names.srtsim-- builds per-domain blocks like scDesign2 does, but reindexes them by name afterwards (rawcount[, rownames(simcolData(simsrt))]), andsimcolDatadefaults torefcolData. Ends up in input order.scdesign2-- restores the order itself since Order scdesign2 input by spatial cluster #22.positive,negative_shuffle,negative_normal-- takeobsandvarfrom the input.Verified end to end, again in the published splatter image:
Changes published numbers for
clustering_ariandclustering_nmi.On the numbers from
run_2026-07-11_18-00-02An earlier version of this description leaned on the
clustering_arileaderboard as evidence, with the four reordering methods sitting at the bottom. I have dropped it, because it cannot carry that weight:downstreamcurrently re-runsBayesSpace::spatialCluster()itself, unseeded, so every row of that table also carries the noise that #30 is about. The two effects are not separable from the published scores.The case for this PR does not need them. That the four methods return their spots in a different order is a property of the scripts, checked directly above, and ARI comparing samples pairwise is a property of ARI.
Related
Longer term the invariant ("a simulated dataset describes the same spots and genes, in the same order, as its input") is not something
file_*.yamlcan express today: the API validates slots and types within a single file, with no way to state a relationship between a component's input and its output. Every task with an in-place dataset transform shares that blind spot, so it may be worth raising incommon/.Checklist before requesting a review
I have performed a self-review of my code
Check the correct box. Does this PR contain:
Proposed changes are described in the CHANGELOG.md
CI Tests succeed and look good!