fix(sparsify): materialize transport-matrix rows instead of pushing indicator columns - #846
Conversation
…ndicators
`sparsify` extracted rows as `push(np.eye(n, batch_size))`. `ott`'s log-sum-exp apply
materializes an `[n, m, k]` tensor for a k-column input, so peak memory was
`n * m * batch_size` rather than the `batch_size` the docstring promised: 17.8 GB to
sparsify an 18 MB plan at n = m = 1500, and ~262 GB at n = m = 8000. The knob you had to
shrink to survive was also the one that made it worse.
Rows are now built directly, through a `_row_materializer` seam on
`BaseDiscreteSolverOutput`: from the factors for low-rank outputs (`Q diag(1/g) R^T`) and
from the potentials for entropic ones (`exp((f_i + g_j - C_ij) / eps)`), reusing the parent
geometry's resolved `epsilon`/`inv_scale_cost` - `Geometry.subset` re-derives both from the
subset and silently changes the values. `GWOutput` keeps its `_rescale_factor`.
`MatrixSolverOutput` slices what it already holds; anything else (e.g. `GraphOTTOutput`,
whose geometry is a `Geodesic` and whose rows are a sub-block of an expanded problem) keeps
the push-based path, which is cheap for a sparse graph kernel.
Peak memory drops 212x at n = m = 1500 (17.8 GB -> 84 MB) and no longer scales with
`batch_size`. Reconstruction is bit-exact for Sinkhorn (offline/online/low-rank-cost),
low-rank Sinkhorn, GW, low-rank GW and FGW, across `scale_cost` in {1.0, mean, max_cost}.
Two behavioural fixes fall out:
* `min_row` is now exact and independent of `batch_size`. The threshold is the largest one
that keeps every row - `min_i max_j T_ij` - so the result stays row-normalizable into a
Markov chain. It was computed over *column* batches, which only underestimates: at
n = m = 1500 it kept 182,645 entries at `batch_size=1024` and 1,227,714 at 64, against
2,319 for the exact rule. Results are unchanged whenever `m <= batch_size`, which is why
small problems never showed it.
* Rows carrying no mass are now handled centrally, for every mode: they keep no entries and
take no part in choosing a threshold, since no threshold can make them non-empty while
letting them participate drags any row-based threshold to 0 and disables sparsification
entirely. A warning is emitted, as the result cannot then be normalized into a Markov chain.
`OTTOutput._with_batch_size` is removed: it only ever rebatched a Sinkhorn output over an
online `PointCloud`, which is exactly the case now built from potentials.
Peak-memory tests live behind a `memory` marker, deselected by default, since they are
environment-sensitive and deliberately large; they run with `pytest -m memory`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sp.issparse` does not narrow for mypy, so `tmap[ixs].toarray()` was reported as `union-attr`. Bind the two branches to separately typed locals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`docs/references.rst` defines `Gromov-Wasserstein` and `fused Gromov-Wasserstein`; `:term:`GW``/`:term:`FGW`` are not glossary entries and warn during the docs build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #846 +/- ##
==========================================
- Coverage 76.62% 76.54% -0.08%
==========================================
Files 36 36
Lines 4175 4217 +42
Branches 670 677 +7
==========================================
+ Hits 3199 3228 +29
- Misses 679 689 +10
- Partials 297 300 +3
🚀 New features to boost your workflow:
|
|
why didn't the first fix work I wonder and I wonder if there are any regression before and after #843 . I'd basically try to test these in all cases to see what regresses and what doesn't |
There's a new test group, pytest.mark.memory, that's excluded in CI. If you run these locally, you should be able to reproduce that #843 didn't fix it. |
| return op | ||
|
|
||
| def _row_materializer(self) -> RowMaterializer: | ||
| """Build a callable materializing rows of the :attr:`transport_matrix`. |
There was a problem hiding this comment.
why use this jax pattern in numpy? Like returning a function to call it later seems redundant no?
|
OK could you perhaps post me some more tables on each branch proving what branch consumes how much memory and time in certain configurations? I'd like to get this right this time. Ok so here the core bottleneck from what I understand is conversion from gpu to cpu without blowing up. And because we move the gpu arrays from jax via converting to numpy arrays we can't have nicely compiled for loops hence the python for loops become painfully slow. Can't we use jax right until the very very end? So the code path is compiled? Like is it possible to move the arrays to cpu jax and have a list of them and convert them once in the end to numpy? |
|
Why #843 didn't fix it. It changed the loop orientation and added GPU (RTX 4090, 24 GB,
"0 MB" means CPU, same three versions, peak RSS above baseline / time / nnz:
Regressions. pre-#843 and #843 agree to the entry everywhere, on both devices, so #843 neither helped nor hurt the memory. On ott. Part of this is upstream: the same call with On staying in jax. I profiled it: 83-84% of the time is jax row construction plus the device to host transfer, 16-17% is the numpy thresholding and CSR assembly. The Python loop isn't the bottleneck, 32 blocks takes 1.78 s and 2 blocks takes 1.71 s, so dropping 30 iterations buys ~4%. Everything up to the block is already compiled jax; only the Two footnotes: GPU and CPU nnz differ marginally (98,784 vs 98,812) from float ordering, and the 🤖 Generated with Claude Code |
|
Ok perfect, I also ran them myself it all seems to make sense. But I'd like to make some changes for the code cleanliness a bit. |
|
Sure, feel free to modify as you like. |
for more information, see https://pre-commit.ci
…and clarity" This reverts commit 78589b0.
|
Thanks a lot! This seems like a conclusive solution to it. I will make a release now |
Problem
sparsifyextracts rows of the transport matrix by pushing dense identity blocks:ott's log-sum-exp apply materializes an[n, m, k]tensor for ak-column input, so peak memory isn · m · batch_size, notbatch_sizeas the docstring promises. Measured onmain(moscot 0.5.1, ott-jax 0.6.0 andmain, jax 0.11.1), for aTemporalProblemwhose dense couplings are 18 MB at n = m = 1500:sparsify_kwargs={"batch_size": …}Isolated at the
ottlevel,output.applyon an[n, k]input peaks at ≈3 · n · m · k · 4bytes. At n = m = 8000 the defaultbatch_sizewould need ~262 GB to sparsify a 256 MB plan. The knob you have to shrink to survive is also the one that makes the result worse (below), and #843's row-oriented loop did not change this — it fixed the orientation but kept the identity push.This is what large-data users hit downstream: scverse/cellrank#1146.
Fix
Rows are built directly, behind one seam —
BaseDiscreteSolverOutput._row_materializer(), a factory returning a callable over row indices (built once persparsifycall, so per-output work such as resolving a geometry is hoisted out of the block loop):LRSinkhornOutput,LRGWOutput):T[rows] = (Q diag(1/g) Rᵀ)[rows], exactlyott's ownmatrix. Never touches a geometry — which matters, sinceLRGWOutput.geomre-linearizes on every access.SinkhornOutput, andGWOutputvialinear_state):T[rows] = exp((f_i + g_j − C_ij) / ε), with the cost rows fromcost_fn.all_pairs(x[rows], y)(PointCloud),cost_1[rows] @ cost_2ᵀ + bias(LRCGeometry) orcost_matrix[rows](denseGeometry).MatrixSolverOutput: slices what it already holds.GraphOTTOutput, whose geometry is aGeodesicand whose rows are a sub-block of an expanded(n+m)²problem — keeps the push-based default, which is cheap there: applying a sparse graph kernel costsnnzper column, notn · m.Two correctness traps this had to avoid, both verified:
geom.subset(row_ixs=…)cannot be used.scale_costandrelative_epsilontravel as unevaluated strings inaux_data, so a subset geometry re-derivesinv_scale_costandepsilonfrom the subset. Withscale_cost="mean"(whatmoscotsolves with) that shiftedinv_scale_cost0.05235 → 0.05396 and produced 15% value error, silently. The materializer captures the parent's resolved values instead.LRCGeometry.subsetis worse — the inheritedGeometry.subsetslices the two factors as if they were cost/kernel.GWOutput._rescale_factor(sqrt(old_transport_mass / linear_state.transport_mass)) must be applied, asmatrixandapplyboth do. It is ~1.0 on converged balanced problems, so it hides easily; there is a test that forces it to 2.0.Results
Peak memory at n = m = 1500 drops from 17.8 GB to 84 MB (212×) and no longer scales with
batch_size(ratio across 64 → 1024 goes from 4.8× to 0.6×);min_rowis now cheaper than materializing the dense plan. Reconstruction is bit-exact (rel_err = 0.0) for Sinkhorn (offline / online / low-rank-cost geometry), low-rank Sinkhorn, GW, low-rank GW and FGW, acrossscale_cost ∈ {1.0, "mean", "max_cost"}.Behavioural changes
min_rowis now exact and independent ofbatch_size. The intent — the largest threshold that keeps every row,min_i max_j T_ij, so the result can still be normalized into a Markov chain — was computed over column batches, which can only underestimate it. At n = m = 1500 it kept 182,645 entries atbatch_size=1024and 1,227,714 at 64, against 2,319 for the exact rule; the threshold and the values also came from two different float paths (pullvspush), which could empty a row outright (I measured 199/200 rows non-empty atbatch_size = m). Results are unchanged wheneverm <= batch_size, which is why small problems never showed this; users withm > batch_sizewill see much sparser couplings.Rows carrying no mass are handled centrally, for every mode: they keep no entries and take no part in choosing a threshold. No threshold can make them non-empty, while letting them participate drags any row-based threshold to 0 and disables sparsification altogether (a single massless row took a test matrix from 21% to 88% dense). A warning is emitted, since the result then cannot be normalized into a Markov chain.
massmode already behaved this way; the modes are now consistent.percentileestimates its threshold from sampled rows rather than pulled column indicators, so it uses the same bounded path as everything else.OTTOutput._with_batch_sizeis removed. It only ever rebatched a Sinkhorn output over an onlinePointCloud— exactly the case now built from potentials — so it became dead code.Tests
tests/solvers/test_base_solver.py:min_rowindependence frombatch_size, the every-row-non-empty contract, exactness againstmin_i max_j T_ij,percentilerow sampling, metadata propagation, aTestMinRowStructureclass checking thepullproxy on a transport-plan-like matrix, and aTestMasslessRowsclass covering all four modes.tests/backends/ott/test_backend.py:TestSparsifyRows— reconstruction across 7 output flavours × 3scale_cost× 3batch_size, a monkeypatchedpush/pull/transport_matrixthat must never be called, themin_rowcontract on real outputs, a parent-scale_costcheck, and the forced GW rescale factor.tests/backends/ott/test_sparsify_memory.py: strict peak-RSS tests, each in a fresh subprocess (peak RSS is process-global andxdistwould confound it). Markedmemoryand deselected by default (addopts = -m 'not memory'), since they are environment-sensitive and deliberately large enough that the old implementation exhausts memory; run them withpytest -m memory.The new tests are discriminating: 37 fail against the current
mainsources and all pass with the fix.One pre-existing test changed:
test_sparsify_minrow'scorrcoef(pull_sparse, pull_dense) > 0.5proxy encoded the old, denser output — on a uniform random fixture the exact rule keeps ~1 entry per row, which cannot correlate with the dense pull. It is replaced by an exactness assertion, and the proxy is retained inTestMinRowStructureon a peaked, plan-like matrix where it is meaningful.Verification
Full suite: 839 passed. The 27 failures in
test_annotation_mapping/test_set_graph_xyare pre-existing in my environment (ott-jaxmain+ jax 0.11) — identical counts with and without this change.black/isort/ruffclean. End-to-end from cellrank (RealTimeKernel.from_moscot(..., sparse_mode="min_row"), n = m = 1500): sparse couplings, no empty rows, row-stochastic transition matrix, and itstest_from_moscottests pass.Out of scope, filed separately:
GraphOTTOutputoverridesshapebut inheritstransport_matrixfromOTTOutput, returning the full expanded(n+m)²matrix.🤖 Generated with Claude Code