fix(neighbors): match Seurat's stored KNN/SNN graphs; the clustering divergence is not a defect - #55
Merged
Merged
Conversation
…tons Chasing the last open item from #54 — shanuz and Seurat disagreeing on clusters over an ifnb RPCA embedding that now matches almost exactly. The divergence itself is not a defect. Given the same neighbour table the two SNN graphs agree off-diagonal to 2.8e-08, so the disagreement is entirely in community detection: Seurat's n.start=10 restarts find a partition 0.17% higher in modularity than shanuz's single igraph pass. That extra modularity buys a split of CD14 Mono into a 73.8%-CTRL and an 83.3%-STIM cluster — residual batch structure, not biology. shanuz's partition scores ARI 0.9195 to seurat_annotations against Seurat's 0.7368, and batch mixing 0.8937 against 0.8328, holding across seeds (17/20 give the same cluster count, 85% beat Seurat's ARI). The Louvain search is left alone. Four real fidelity defects surfaced on the way there, all fixed: * `_knn_to_sparse` symmetrised the KNN graph. Seurat's `nn` is the raw directed table: nnz is exactly n*k, every row sums to k, and column sums vary with in-degree. Symmetrising erased that hub signal. Both in-tree consumers already symmetrise at the point of use, as Seurat's own do. * `_build_snn` deleted the diagonal. `ComputeSNN` stores SNN[i,i] = 1 for every cell; all 13999 were present in Seurat's graph and none in shanuz's. It was invisible to `find_clusters` because `_sparse_to_igraph` takes the strict upper triangle and discarded the very entries that were missing. * `run_umap` did not strip that diagonal. `RunUMAP.Graph` opens with `diag(x = object) <- 0`; without it the restored diagonal would feed n zero-length self-edges to the layout. * `_build_snn` computed Jaccard in float32, ~3e-08 off Seurat on every weight. Now float64: max deviation 4.4e-16. This does not change which edges are pruned — a weak Python `prune_snn` is cast down to float32 so both sides of the comparison round identically — only the weights. `find_clusters` also gains `group_singletons` (default True), porting Seurat's `GroupSingletons`: size-1 clusters are absorbed into whichever non-singleton cluster they are most connected to, scored by *mean* SNN weight, with the candidate list fixed before the loop so one singleton cannot absorb another. Both graphs now reproduce Seurat exactly on ifnb — nn nnz 279980 with column sums [68, 24, 21, 23, 32], snn nnz 1120457 and sum 156062.938571. 11 new tests, each verified to fail when its fix is reverted. One earlier draft guard was decorative twice over: its fixture let sum and mean pick the same cluster, and a second claimed float32 straddled the prune threshold, which is not true. Both corrected rather than kept green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the clustering section to the integration vignette: the three-stage split (identical KNN indices, SNN agreeing to 2.8e-08, divergence only in community detection), the CD14 Mono batch-split table, the seed-robustness evidence, and why no n.start knob was added. Rewrites the "still open" bullets in the anchors vignette and ROADMAP that recorded this as uninvestigated, and documents the four graph defects in CHANGELOG. Tallies: T-int 14 -> 18 defects, project total 44 -> 48, suite 787 -> 798. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shanikawm
added a commit
that referenced
this pull request
Jul 25, 2026
Compare the object model against exact neighbours, and refresh the post-#55 docs
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.
Closes the last item left open by #54: shanuz and Seurat disagreeing on clusters over an ifnb RPCA embedding that now matches to 30/30 PCs.
The divergence is not a defect
Ran Seurat's own RPCA embedding through both tools in three stages, with
nn.method = "rann"so the neighbour search is exact on both sides.Seurat's
FindClustersrunsn.start = 10restarts of its own modularity optimiser and keeps the best; shanuz runs a single igraph multilevel pass. Seurat's partition scores 0.899903 to shanuz's 0.898336 under igraph's own modularity at γ=0.5 — so shanuz genuinely searches less hard, and 20 seeds never reach Seurat's value.But the deeper optimum is the worse answer. shanuz's partition is a strict coarsening: no Seurat cluster is split, and one shanuz cluster absorbs two of Seurat's. Both are CD14 Mono, split along the batch axis:
The extra 0.17% modularity is spent re-discovering the batch effect integration just removed. Against
seurat_annotations, shanuz scores ARI 0.9195 vs Seurat's 0.7368; batch mixing 0.8937 vs 0.8328.Not a lucky seed: across 20 seeds shanuz gives 15 clusters 17 times, 85% beat Seurat's cell-type ARI, and none reaches its modularity. The Louvain search is therefore left alone and no
n.startequivalent added — its main effect here would be to converge harder onto the batch split. Documented caveat: shanuz's single pass is more seed-sensitive (cell-type ARI 0.72–0.93 over those seeds).Four real defects found proving that
All were invisible to
find_clusters, whose igraph conversion takes the strict upper triangle and discarded exactly the entries that were missing. The graphs are stored objects users read directly.nngraphnnz= n·ksnndiagonalSNN[i,i] = 1, all 13,999snnprecisionGroupSingletonsSymmetrising was the costliest: Seurat's
nnis the raw ranked table, so every row sums tokwhile column sums run 21–68 — that spread is the in-degree, andmat + mat.Terased it. Both in-tree consumers already symmetrise at the point of use, as Seurat's do.Restoring the SNN diagonal meant following Seurat to the consumer too:
RunUMAP.Graphopens withdiag(x = object) <- 0, sorun_umapnow strips it rather than feeding the layout n zero-length self-edges.Both graphs now reproduce Seurat exactly on ifnb —
nnnnz 279980 with column sums[68, 24, 21, 23, 32],snnnnz 1120457 and sum 156062.938571.Added
find_clusters(group_singletons=True)ports Seurat's rule: a size-1 cluster joins whichever non-singleton cluster it is most connected to, scored by mean SNN weight, candidate list fixed before the loop so one singleton cannot absorb another. ifnb has none at this resolution; it fires on sparser graphs.Tests
11 new in
tests/test_neighbors_graph_parity.py, 798 passing (was 787). Every guard mutation-tested — revert the fix, confirm a named test fails, 7/7 caught.Two drafts were decorative and were rebuilt rather than kept green:
w + w.T, which made both rules pick the same cluster;prune_snnis cast down to float32, so both sides of the comparison round identically. The code comment asserting that consequence was corrected too; float32 only ever changed the stored weights, not which edges survive.A third trap worth recording:
float32→float64is a same-length edit, so a mutate/restore inside one mtime second left Python reusing the mutated.pycand reporting a fix as caught when it wasn't. The harness now runs with bytecode disabled.🤖 Generated with Claude Code