Skip to content

Keep simulated spots in the order they came in - #31

Open
rcannood wants to merge 2 commits into
mainfrom
fix/simulated-spot-order
Open

Keep simulated spots in the order they came in#31
rcannood wants to merge 2 commits into
mainfrom
fix/simulated-spot-order

Conversation

@rcannood

@rcannood rcannood commented Jul 28, 2026

Copy link
Copy Markdown
Member

Describe your changes

splatter, sparsim, symsim and zinbwave all sort their input by spatial_cluster and 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:

obs_names same SET  : TRUE
obs_names same ORDER: FALSE
real first 4: 16.918x16.996 18.017x17.034 20.075x17.059 18.979x17.065
sim  first 4: 16.918x16.996 23.122x17.085 15.011x17.051 16.003x17.048

What that actually breaks

Only clustering_ari and clustering_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() and generate_rmse() index by name (intersect(rownames(real), rownames(sim))), so they line up regardless.
  • the ks_* metrics are distributional.

Clustering is the one place the two datasets are compared spot by spot. In downstream:

real_cluster <- input_real_sp$obs[, c("spatial_cluster")]                       # real order
sim_cluster <- generate_sim_spatialCluster(input_real_sp, input_simulated_sp)   # sim order
clustering_ari <- aricode::ARI(real_cluster, sim_cluster)

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

  • Sorts the result back before writing, and takes obs and var straight from the unsorted input.
  • Adds check_alignment() to src/helpers/utils.R, called from generate_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.
  • Drops the 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:
method returned : g3 g1 g4 g2
wanted          : g1 g2 g3 g4
as written      : g4 g3 g2 g1  -> fixed? FALSE
args swapped    : g1 g2 g3 g4  -> fixed? TRUE

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 -- ncell defaults to dim(sce)[2], and we do not pass it, so scDesign3 takes newCovariate <- dat and 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))]), and simcolData defaults to refcolData. Ends up in input order.
  • scdesign2 -- restores the order itself since Order scdesign2 input by spatial cluster #22.
  • positive, negative_shuffle, negative_normal -- take obs and var from the input.

Verified end to end, again in the published splatter image:

obs_names same ORDER: TRUE
var_names same ORDER: TRUE
check_alignment passes: TRUE
shuffled -> The simulated dataset's spots do not match the real dataset's: they are in a different order.

Changes published numbers for clustering_ari and clustering_nmi.

On the numbers from run_2026-07-11_18-00-02

An earlier version of this description leaned on the clustering_ari leaderboard as evidence, with the four reordering methods sitting at the bottom. I have dropped it, because it cannot carry that weight: downstream currently re-runs BayesSpace::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_*.yaml can 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 in common/.

Checklist before requesting a review

  • I have performed a self-review of my code

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Bug fixes
  • Proposed changes are described in the CHANGELOG.md

  • CI Tests succeed and look good!

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.
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
rcannood added 2 commits July 28, 2026 15:55
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
rcannood force-pushed the fix/simulated-spot-order branch from 1eece96 to 4122ed8 Compare July 28, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant