accel: promote exact-nnz Wilcoxon to the CSC default, and fix the two CSC routes that lost for a reason by nick-youngblut · Pull Request #559 · ArcInstitute/scx

nick-youngblut · 2026-09-23T16:27:40Z

Update — the exact-nnz Wilcoxon kernel is now the default (c859d286)

The first version of this PR added the evidence for promoting the kernel and left the switch to a separate decision. That decision is made, and c859d286 flips it:

1-vs-rest rank_genes_groups on the CSC route now runs the nnz kernel, recorded as route cpu_csc_nnz.

SCX_ACCEL_WILCOXON_NNZ=0 falls back to the densify kernel (cpu_csc). Unset, or any other value, leaves the default on.

reference= and rankby_abs=True still use densify, the only kernel that handles them. pdex_ref is unchanged.

See "Exact-nnz Wilcoxon: now the default" below. That commit has not been through a review round.

This PR measures, one op at a time, which of the ops that don't default to prefer_format="auto" should. None of the measurements justified moving another op to auto. Two of them turned out to be losing, or looking like they won, because of a defect, and this PR fixes both. It also promotes the exact-nnz Wilcoxon kernel to the 1-vs-rest CSC default, with a zero-tolerance parity test against the kernel it replaces.

`col_*`: CSR stays the default, and CSR `col_min` / `col_max` / `col_var` get 7–9× faster

The first measurement had CSC ahead on col_min / col_max (1.45×) and col_var (3×) at tabula_sapiens_100k. That result was an artifact.

With no projection, pyscx.accel.col_min / col_max / col_var handed the projected kernels an identity column list. Those kernels run project_csr on every decoded shard, so the whole matrix was copied once per pass, twice for col_var.
col_sums / col_nnz and the X.min/max/var(axis=0) dunders already dispatched four ways and used the reader's whole-axis kernels.
col_aggs.rs now does the same.

Measured on one 16-core cpu node, median of 3, one fresh process per run:

	`main` CSR	this PR CSR	CSC
tabula `col_min`	6.63 s	0.93 s	4.39 s
tabula `col_var`	12.93 s	1.77 s	4.25 s
census_1m `col_min`	45.9 s	5.5 s	43.1 s
census_1m `col_var`	90.3 s	10.5 s	42.1 s

After the fix, CSR is faster on all five ops: CSC is 2.4–4.8× slower at tabula and 4.0–8.5× slower at census_1m. Every arm pair returned the same values (a hash of the output rounded to 1e-3).

On a multi-shard file, the CSR col_var result now matches X.var(axis=0) exactly. Before, the two summed in different orders and differed in the low bits.

The CSC col_* walk no longer loads the whole sidecar into memory. walk_csc_runs read each contiguous run of requested columns as one slab. On an unprojected handle that run is the whole sidecar: 7.2 GB at census_500k and 12.9 GB at census_1m. Runs are now split at CSC shard boundaries, which brings peak down to 1.8 GB and 5.7 GB; 5.7 GB is also the CSR arm's peak on the same file. The output doesn't change, because each column lives in exactly one shard.

HVG: stays CSR; `prefer_format` is not deprecated

On CPU, the CSC route (single-batch seurat_v3) is 4.4× slower at tabula (9.8 s vs 2.2 s) and 6.4× slower at census_1m (84.7 s vs 13.3 s). It uses 1.5–2.2× the peak memory and selects the same genes.

The kwarg stays because on device="gpu", prefer_format="csc" is the only way a filtered handle reaches gpu_csc_v3; the default auto-route leaves such a handle on gpu_csr. The docstring now states the CPU loss. docs/sharding.md § When to add a CSC sidecar drops its HVG bullet and names HVG and col_* as reasons not to add a sidecar.

Backed handle + gene projection: already routes CSC

The plan said a backed handle with only a column projection took cpu_csr under auto. It already takes cpu_csc on main: PR-B (#554) changed the backed handle to serve its view as the column source. This PR only pins that, for subset_var, filter_genes and adata[:, mask]. The new test passes on main too; it guards against a regression, not a fix.

Docs: which shard-cache regime each number comes from

A new block in docs/scanpy.md § prefer_format states which shard cache each DE number was measured with:

Default 4-shard cache: 13.7× / 21.7× at tabula, and 6.2× at census_1m in an earlier one-off A/B. Here the CSR route re-decodes every shard for every gene chunk.
Cache sized to the whole file: 1.95× / 1.45×, the LATEST bench_csc_dispatch arms, with the CSR arm at 83 GB of RSS.

HVG and col_* are single-pass ops, so the cache doesn't enter into them. The negative GPU result is kept. docs/performance.md states the default-cache setting for its DE table. The new HVG and col_* numbers go in scanpy.md, marked as one-off captures, because performance.md claims need a manifest entry.

Exact-nnz Wilcoxon: now the default

The evidence:

Parity test: nnz_matches_the_densify_kernel_exactly_on_tie_heavy_counts compares the nnz kernel to the densify kernel at zero tolerance: scores, p-values, adjusted p-values, log fold changes and gene order. The matrix is counts in {1, 2, 3} with explicit stored zeros, an empty column, a constant column, unlabelled cells, and a chunk width that doesn't divide n_vars. I checked it can fail: scaling the implicit-zero rank-sum term by 1 + 1e-15 fails this test, while the existing 1e-9 property test still passes. After the flip the test calls the densify kernel directly (wilcoxon_rank_sum_densify_csc), not through the process-global gate.
Speed: 1-vs-rest, densify → nnz, measured in fresh processes:
- tabula: 19.6 s → 6.2 s (3.1×), 1,190 → 990 MB.
- census_1m, grouped by disease: 225.9 s → 55.7 s (4.1×), same 5.7 GB peak.

The switch (c859d286):

The gate is on unless SCX_ACCEL_WILCOXON_NNZ is 0 / false / FALSE / off. It is still read once per process.
The CSR-parity and non-finite-value tests now run both CSC kernels, so the densify kernel keeps its coverage even though the default no longer reaches it.
The pyscx tests that expected cpu_csc from a 1-vs-rest call now expect cpu_csc_nnz. Their value comparisons against CSR, some of them bit-for-bit, pass unchanged.

Benchmarks:

bench_csc_dispatch's de_csc arm now times the default, and its csc_dispatch_correct passes only on route cpu_csc_nnz.
The fresh-process arm is now de_csc_densify, run with SCX_ACCEL_WILCOXON_NNZ=0 and floored on route cpu_csc. It is the control that shows what the default is worth, and it needs a fresh process because the gate is read through a OnceLock.
bench_de_csc_routes.py sets the gate itself from --mode before importing pyscx, so each mode always runs the kernel it names.

benchmarks/scripts/bench_de_csc_routes.py now drops unused categorical levels. Without that, census_1m's disease column, which declares the census-wide vocabulary, has declared levels with no cells, and the two-cell minimum-group check (in place since 0.17) raises before any gene is tested.

Census DE arms of `bench_csc_dispatch` (fixed in `bbd4a847`)

The first version of this description listed this as found, not fixed. bench_csc_dispatch._run_de picked cell_type on census fixtures, where some groups have a single cell, so every census DE arm raised under the two-cell minimum, including the new de_csc_nnz arm. _run_de now does two things before calling rank_genes_groups:

It turns groupby levels with fewer than two cells into unlabelled cells, and drops unused levels. Unlabelled cells stay in the rank pool and in every group's "rest".
It falls back to the synthetic split when fewer than two levels remain.

test_bench_csc_dispatch_groupby.py checks the setup: the unmodified call raises on such a column, and _run_de runs on both routes. Found by codex and Cursor Agent.

Local verification

pyscx pytest tests/: 3242 passed.
cargo test -p scx-accel passes, and benchmarks/comprehensive/tests has 534 passed.
cargo clippy --workspace --exclude rscx --all-targets -D warnings and cargo fmt --check are clean.
test_accel_col_ops_agree_with_the_dunders_without_a_projection fails on main's build (col_var, whole handle) and passes here.

… that lost for a reason PR-G of the CSC plan: settle which ops `prefer_format="auto"` should reach, with a measurement for each, and pin the routes that already work. col_* (col_sums/nnz/min/max/var): not flipped, CSR wins all five. The first measurement had CSC ahead on col_min/col_max (1.45x) and col_var (3x); that was the CSR path passing an identity column list to the projected kernels, which project_csr-copied every decoded shard (twice for col_var). Unprojected handles now use the reader's whole-axis kernels, as the X.min/max/var dunders already did: tabula col_min 6.6 -> 0.93 s, col_var 12.9 -> 1.77 s; census_1m col_var 90.3 -> 10.5 s. CSC is then 2.4-4.8x slower at tabula and 4.0-8.5x at census_1m. col_var's CSR result now matches X.var(axis=0) bit for bit (the summation order used to differ in the low bits). The CSC col_* walk also decoded the whole sidecar as one slab on an unprojected handle (7.2 GB at census_500k, 12.9 GB at census_1m); runs are now split at CSC shard boundaries (1.8 GB / 5.7 GB, the open floor), with identical output since a column lives in one shard. HVG: stays csr, not deprecated. CSC is 4.4x (tabula) / 6.4x (census_1m) slower on CPU, but prefer_format="csc" is also how a filtered handle reaches gpu_csc_v3 on GPU. Docs stop recommending a sidecar for HVG or col_*. Backed + gene-only projection already takes cpu_csc under auto (PR-B's view unification); pinned for subset_var, filter_genes and adata[:, mask]. Exact-nnz Wilcoxon: bench_csc__de_csc_nnz arm (fresh process per run, since the env gate is read once, floored on route == cpu_csc_nnz) and a zero- tolerance parity pin against the densify kernel on a tie-heavy matrix; a 1e-15 rank-sum drift fails it while the 1e-9 proptest passes. Measured 3.1x (tabula) and 4.1x (census_1m) over densify. Still opt-in. Docs state the shard-cache regime each DE number was taken in. bench_de_csc_routes.py drops unused categorical levels, without which census_1m hit the two-cell guard.

gemini-code-assist

Code Review

This pull request introduces several benchmark variants, optimizes column aggregation operations by avoiding identity projections when no projection is active, and splits CSC runs at shard boundaries to reduce memory usage. It also updates documentation to clarify that CSC routes for HVG and column reductions are slower than CSR on CPU, and adds comprehensive tests including an exact-match test for the Wilcoxon exact-nnz kernel. The reviewer suggested reducing the extremely generous 3-hour timeout for fresh process benchmark runs to a more reasonable limit (e.g., 30 minutes) to prevent wasting cluster resources if a worker hangs.

gemini-code-assist · 2026-09-23T16:30:19Z

+
+# Generous: the densify CSC DE route takes ~70 s on tabula_sapiens_100k and
+# ~4 min at census_1m.
+_FRESH_PROCESS_TIMEOUT_S = 3 * 3600


The timeout of 3 hours (3 * 3600 seconds) is extremely generous for a task that is expected to take at most ~4 minutes. If a worker process hangs, this could block the SLURM job for a long time and waste cluster resources or exceed the job's walltime limit. Consider reducing this timeout to a more reasonable value, such as 15 or 30 minutes (e.g., 1800 seconds).

Suggested change

_FRESH_PROCESS_TIMEOUT_S = 3 * 3600

_FRESH_PROCESS_TIMEOUT_S = 1800

nick-youngblut · 2026-09-23T16:31:35Z

Defects

P2 — bench_csc__de_csc_nnz is not scoped away from census_1m, but it calls _run_de, which can select cell_type containing a present group with fewer than two cells. rank_genes_groups rejects any participating group below that minimum, so the fresh child exits non-zero and _run_fresh_process returns None; the new arm therefore cannot produce a census result. The categorical cleanup in bench_de_csc_routes.py does not fix this path: it handles disease unused levels, while _run_de selects cell_type, and removing unused levels would not repair a real singleton anyway. Normalize/validate the selected grouping inside _run_de (drop unused categories, then fall back to the existing synthetic two-way grouping when any observed count is < 2) and add a census-fixture benchmark test; alternatively scope the new arm to tabula until that is implemented.

Over-engineering

NIT — _FRESH_PROCESS_VARIANTS and _EXPECTED_ROUTE are one-entry configuration maps, and _run_fresh_process(key, ...) has only that one caller. Keep the shared run_arm helper, but replace these maps and the keyed dispatcher with an NNZ-specific fresh-process helper that passes the literal SCX_ACCEL_WILCOXON_NNZ=1 environment and expects the literal cpu_csc_nnz route. That removes an unused extension surface without changing the benchmark behavior.

Checks

gh pr checks 559 for 0955fc05408cd7756e8aa5bb9ad4eb459297ceeb currently shows Format, Changed-path gate, Clippy, both feature-matrix clippy legs, and I-ORG-1 dedup guards passing. Base install, Test, Test (hdf5 features), and Python bindings are still pending. CI is therefore not complete, so this is not a regression-free/pass verdict.

Reviewed by codex - gpt-5.6-terra.

nick-youngblut · 2026-09-23T16:38:01Z

Cursor Agent - Grok 4.7 High

Reviewed 0955fc05 (full diff plus the callers of walk_csc_runs, the reader kernels col_{min,max,var} now dispatch to, and bench_csc_dispatch._run_de).

Defects

1. Medium — `bench_csc__de_csc_nnz` cannot collect the census number this PR is evidence for

bench_csc__de_csc_nnz calls _run_de, which still prefers cell_type. On the census fixtures that column has groups of one cell, and rank_genes_groups refuses any participating group with fewer than two cells (since 0.17). The worker then exits non-zero, _run_fresh_process returns None, and the new csc_dispatch_correct floor never sees a row: check_absolute_floors skips a missing triple. The floor is only declared for tabula_sapiens_100k.

The same PR fixes this class of failure in benchmarks/scripts/bench_de_csc_routes.py (remove_unused_categories) and reports the 4.1× census_1m nnz number from that script. remove_unused_categories is not enough inside _run_de: a level with one observed cell is used, so it survives. Drop levels with fewer than two cells, or fall back to the synthetic _bench_group split when any remaining level is that small, and run the nnz arm through that helper. Until then the suite arm does not reproduce the measurement it was added to gate.

This is the same crash the existing de_csr / de_csc arms already have. The new arm inherits it, and it is the arm whose census result is the promotion evidence.

2. Low — the new cache caveat and the bold conclusion under the same table disagree

docs/performance.md now says the 13.7× / 21.7× tabula ratios are mostly CSR re-decode under the default 4-shard cache, and that a file-sized cache is 1.95× / 1.45×. The paragraph immediately under that table is unchanged and still states “CSR → CSC-direct is ~13.7× faster” and “a large, measured win” with no cache qualifier. Anyone who reads the bold line gets the claim the new paragraph just withdrew. Qualify that sentence with the default-cache regime, or point it at the 1.95× / 1.45× figures.

3. Low — `test_auto_takes_csc_on_a_gene_only_projection` documents a fix this PR did not make

The docstring says a gene-only backed view “used to take cpu_csr” and that “the handle now hands back a view”. The PR body says that route already takes cpu_csc on main (PR-B / #554) and that this test passes there too. The test is a regression pin. The comment will send the next reader looking for a dispatch change that is not in this diff. State that.

Over-engineering

_FRESH_PROCESS_VARIANTS and _EXPECTED_ROUTE each have one key, bench_csc__de_csc_nnz, and _run_fresh_process looks both up. A missed edit to one of them stamps csc_dispatch_correct against the wrong route. Collapse them into one dict, e.g. {"bench_csc__de_csc_nnz": {"env": {"SCX_ACCEL_WILCOXON_NNZ": "1"}, "route": "cpu_csc_nnz"}}. The fresh process itself should stay: SCX_ACCEL_WILCOXON_NNZ is a OnceLock, so this interpreter cannot flip the kernel after bench_csc__de_csc has already resolved it. run_arm is the existing tool for that.

Blast radius

No prefer_format default moves. HVG stays CSR. The nnz Wilcoxon kernel stays opt-in (SCX_ACCEL_WILCOXON_NNZ).

User-visible numeric change. On an unprojected backed handle, pyscx.accel.col_min / col_max / col_var (deletion vector or not) now call BackedCsrReader::col_{min,max,var} and the masked twins — the same four-way split col_sums / col_nnz and X.min / X.max / X.var(axis=0) already used. col_var on a multi-shard file changes in the low bits and matches X.var(axis=0) bit for bit; min/max of finite values do not depend on visit order. A column projection still goes through projected_agg::col_*_projected. col_presentation without a projection is still not applied on the accel path (the dunders reorder; that mismatch predates this PR).

CSC peak, every caller of walk_csc_runs. Runs now break at csc_shard_col_range starts. That function is the slab walker for all five col_*_projected_csc kernels, so the change covers prefer_format="csc" on col_sums / col_nnz / col_min / col_max / col_var and the gene-axis half of calculate_qc_metrics (compute_gene_axis_csc → col_sums_and_nnz_projected_csc). Both call sites pass a LazyShardSource, whose shard ranges are in the same axis as the identity col_indices those callers build, including a column projection. A column lives in one shard, and a requested run only crosses a boundary when it includes that boundary column, so the split does not change which nonzeros are accumulated or their order. Existing multi-shard parity (test_csc_dispatch.py, csc_cols_per_shard=4) covers the values. Nothing in this diff asserts the new peak bound.

Not on the hot path. The nnz-vs-densify test calls the two drivers at chunk width 5 on one seeded tie-heavy matrix. wilcoxon_rank_sum_streaming_csc may still clamp that width through de_gene_chunk_or_err; at n_obs=300 the default budget will not. The test does not promote the kernel.

CI

On 0955fc05: Format, Clippy, both feature-matrix clippy legs, Test, Test (hdf5), Base install, Changed-path gate, I-ORG-1 dedup guards, and Python bindings pass. Docs anchors is skipping (not a docs-only PR). Python bindings finished after the first version of this comment; the new pytest pins ran in that job. That is CI green on this commit. It does not cover the census bench_csc__de_csc_nnz failure in defect 1, which only shows up when that arm is collected.

nick-youngblut · 2026-09-23T16:38:02Z

Review: PR #559 — accel: measure the CSC routes that do not auto-route, and fix the two that lost for a reason

Reviewer: Antigravity - Gemini 3.8 Flash
Commit Reviewed: 0955fc0
CI Checks Status: gh pr checks 559 reports checks are still pending for commit 0955fc0 (9 successful, 1 skipped, 1 pending). Specifically:

CI/Base install (no extras): pass
CI/Clippy (pull_request): pass
CI/Feature matrix (clippy, hdf5 legs): pass
CI/Feature matrix (clippy, no-hdf5 legs): pass
CI/Test (pull_request): pass
CI/Test (hdf5 features): pass
CI/Changed-path gate: pass
CI/Format (pull_request): pass
CI/I-ORG-1 dedup guards: pass
CI/Docs anchors (docs-only PRs): skipped
CI/Python bindings (pull_request): pending

Because CI/Python bindings is still running, CI has not fully concluded for this commit; however, rust tests, formatting, clippy, and local comprehensive suites have passed without regressions.

Defects (Ranked by Severity)

[Medium] Unchecked dictionary indexing in fresh worker string template (benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:433)
In _FRESH_WORKER, route extraction is performed via direct dict indexing:
```
"route": b._extract_route(adata, b._VARIANT_OP_KEY[key]),
```
In contrast, the in-process runner in run() (line 550) safely uses _VARIANT_OP_KEY.get(key) because ops such as col_sums and col_var return ndarrays and do not record a route in adata.uns. In _FRESH_WORKER, accessing b._VARIANT_OP_KEY[key] will raise an unhandled KeyError if any variant omitted from _VARIANT_OP_KEY is added to _FRESH_PROCESS_VARIANTS.
- Fix: Change b._VARIANT_OP_KEY[key] to b._VARIANT_OP_KEY.get(key, "").
[Low] Whole-process CPU time metrics leak interpreter startup and file loading into op measurement (benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:428-434)
In _FRESH_WORKER, user_s and sys_s are captured via ru = resource.getrusage(resource.RUSAGE_SELF) at the end of execution and serialized directly. Unlike the in-process runner which measures deltas (u1 - u0, s1 - s0) strictly around runner(adata, prefer), the fresh worker reports the cumulative process CPU time—attributing Python bootstrap, heavy module imports, and file opening/decompression to the op's compute time. While peak_rss_mb must be process-wide due to ru_maxrss monotonicity, CPU times can and should be delta-sampled around the op.
- Fix: Sample ru0 = resource.getrusage(resource.RUSAGE_SELF) immediately before runner(adata, prefer) and emit ru.ru_utime - ru0.ru_utime and ru.ru_stime - ru0.ru_stime.
[Low] Test coverage gap for walk_csc_runs shard-boundary splitting under disjoint/offset column projections (pyscx/tests/test_col_projected_agg.py)
test_accel_col_ops_agree_with_the_dunders_without_a_projection thoroughly verifies bitwise agreement of the unprojected CSR route against AnnData dunders on multi-shard files. However, the shard-boundary splitting introduced in walk_csc_runs (pyscx/src/projected_agg.rs) is not directly exercised with edge-case column projections—specifically cases where a projection spans across shard boundaries while completely omitting intermediate shards or starting on a non-zero shard.
- Fix: Add a unit test in pyscx/tests/test_col_projected_agg.py running col_sums(..., prefer_format="csc") on a multi-shard CSC file with non-contiguous column projections that span shard boundaries and omit middle shards.

Over-Engineering & Simplification Opportunities

Parallel synchronized dictionaries for single fresh-process variant (benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:116-121)
- Problem: The PR adds two separate global dicts for fresh process configuration:
```
_FRESH_PROCESS_VARIANTS: dict[str, dict[str, str]] = {
    "bench_csc__de_csc_nnz": {"SCX_ACCEL_WILCOXON_NNZ": "1"},
}
_EXPECTED_ROUTE: dict[str, str] = {"bench_csc__de_csc_nnz": "cpu_csc_nnz"}
```
  This introduces split configuration surfaces for a single variant key.
- Simpler alternative: Collapse both into a single mapping _FRESH_PROCESS_VARIANTS: dict[str, tuple[dict[str, str], str]] = {"bench_csc__de_csc_nnz": ({"SCX_ACCEL_WILCOXON_NNZ": "1"}, "cpu_csc_nnz")} (or inline the expected route into _FRESH_PROCESS_VARIANTS[key]["expected_route"]).
Redundant sorting and duplicate entries in shard_starts (pyscx/src/projected_agg.rs:1065-1069)
- Problem: In walk_csc_runs:
```
let mut shard_starts: Vec<u32> = (0..source.n_csc_shards())
    .filter_map(|i| source.csc_shard_col_range(i).map(|(lo, _)| lo))
    .collect();
shard_starts.sort_unstable();
```
  Because shards are indexed sequentially ($0 \le i < \text{n_csc_shards}$), source.csc_shard_col_range(i) produces monotonically non-decreasing start columns by definition. Calling sort_unstable() is redundant. Furthermore, when projections elide shards, multiple shards report identical lo values (lo == hi), leaving duplicate start values in shard_starts.
- Simpler alternative: Replace shard_starts.sort_unstable() with shard_starts.dedup().

Summary & Blast Radius

Blast Radius: Moderate and well-isolated.
- CSR optimizations (pyscx/src/accel/col_aggs.rs): Eliminates redundant project_csr copies on unprojected col_min, col_max, and col_var, bringing them in line with col_sums and col_nnz and matching the dunders bit-for-bit.
- CSC memory bounding (pyscx/src/projected_agg.rs): Limits memory residency to one CSC shard per slab walk instead of materializing the entire sidecar into a single slab on unprojected handles.
- Exact-nnz Wilcoxon parity (scx-accel/src/csc/wilcoxon.rs): Adds zero-tolerance parity assertions against the densify kernel on tie-heavy count matrices, backing up performance claims without prematurely flipping defaults.
- Docs & Benchmarks: Clarifies shard cache dependencies and CPU/GPU trade-offs accurately.

…here it matters Round-1 review fixes for PR #559. - bench_csc_dispatch._run_de unlabels groupby levels with fewer than two cells (and drops unused ones) before calling rank_genes_groups, falling back to the synthetic split when fewer than two levels remain. Census fixtures' cell_type has singleton levels, so every census DE arm of this benchmark, including the new de_csc_nnz one, raised under 0.17's two-cell guard. Unlabelled cells stay in the rank pool and in every group's rest. (codex, Cursor Agent) - The fresh-process variant table is one dict of (env, expected route) rather than two one-key dicts that had to agree. (all three) - The fresh worker reports user/sys CPU time around the op only, as the in-process arms do; peak RSS stays whole-process. (Antigravity) - performance.md's bold 13.7x / 21.7x sentences now say they are default-cache ratios and point at the file-cache 1.95x / 1.45x. (Cursor) - test_auto_takes_csc_on_a_gene_only_projection says it is a regression pin, not the test for a fix in this PR. (Cursor) - walk_csc_runs dedups shard starts (a projection-emptied shard repeats the next one's), and a new test runs all five CSC col_* ops on a projection that skips whole shards and starts mid-shard. (Antigravity)

nick-youngblut · 2026-09-23T16:51:48Z

Defects

P2 — The new fresh-process NNZ benchmark can hold a benchmark worker for three hours after a hang. The comment says the slowest expected densify run is about four minutes, but the actual limit remains:

_FRESH_PROCESS_TIMEOUT_S = 3 * 3600

and that timeout is applied to every warmup and measured child. Set it to a bounded value such as 30 minutes (or another evidence-backed multiple of the observed worst case) so a stalled process does not consume a SLURM allocation for three hours.

Over-engineering

NIT — The post-fix _FRESH_PROCESS_VARIANTS is still a one-entry configuration map and _run_fresh_process is called for only that one key. Replace it with an NNZ-specific helper that passes the literal SCX_ACCEL_WILCOXON_NNZ=1 environment and requires the literal cpu_csc_nnz route; dispatch to it with one key comparison. This also removes the currently unchecked worker lookup:

b._extract_route(adata, b._VARIANT_OP_KEY[key])

A future fresh variant without route metadata would otherwise fail with KeyError rather than report a failed route gate. This is not a present runtime failure, but there is no second implementation using the generic extension point today.

Prior-round findings vs `bbd4a84`

Codex P2 / Cursor defect 1, census singleton and unused levels: closed. The fix removes every category whose observed count is below two:
```
col = col.cat.remove_categories([c for c in col.cat.categories if counts.get(c, 0) < 2])
```
and falls back through:
```
if groupby is not None and not _unlabel_undersized_groups(adata, groupby):
    groupby = None
```
before constructing the synthetic split. The new test runs both csr and csc preferences.
Codex NIT, replace the two single-entry fresh-process maps and keyed dispatcher: partially fixed. bbd4a84 correctly merges the two maps into one tuple map, but it retains a one-key registry and generic keyed dispatcher. The concrete simplification is in the Over-engineering note above.
Cursor defect 2, unqualified default-cache DE conclusion: closed. The updated prose now says:
```
Every arm opens its handle with to_anndata(backed=True)'s default 4-shard cache
```
and explicitly gives the whole-file-cache comparison as 1.95x / 1.45x.
Cursor defect 3, the gene-only-projection test claiming this PR made the route change: closed. Its docstring now opens:
```
A regression pin, not the test for a fix.
```
Gemini review, three-hour fresh-worker timeout: untouched. The same 3 * 3600 assignment remains, so the P2 above is still applicable.
Antigravity defect 1, direct route-map indexing in the fresh worker: untouched. The worker still uses:
```
b._VARIANT_OP_KEY[key]
```
rather than a guarded lookup; covered by the simplification above.
Antigravity defect 2, fresh-worker CPU time included interpreter setup: closed. The worker now takes ru0 immediately before runner(adata, prefer) and emits:
```
"user_s": ru.ru_utime - ru0.ru_utime,
"sys_s": ru.ru_stime - ru0.ru_stime,
```
Antigravity defect 3, no projection/shard-boundary CSC coverage: closed. test_csc_col_ops_on_a_projection_that_skips_whole_shards covers all five operations with columns spanning shards 0, 3, and 4 while skipping 1–2.
Antigravity over-engineering, two separate fresh-process configuration dictionaries: closed. They are now one mapping:
```
"bench_csc__de_csc_nnz": ({"SCX_ACCEL_WILCOXON_NNZ": "1"}, "cpu_csc_nnz")
```
Antigravity shard-start sort/dedup observation: partially fixed as proposed, and correct as implemented. bbd4a84 adds shard_starts.dedup(), but retains sort_unstable(); retaining the sort is warranted because the ColumnShardSource contract does not promise shard-range order. The implementation states that explicitly:

// Sorted rather than assumed sorted (the trait promises no order)

Correction block and blast radius

The PR description's correction block matches bbd4a84: zero- and singleton-count categories are converted to missing values, fewer than two surviving groups select the synthetic split, and the route test exercises both layouts. The standalone DE script's narrower correction also matches its diff: it calls remove_unused_categories() for the selected categorical groupby.

The behavioral blast radius is otherwise contained:

No prefer_format default changes and no NNZ promotion: the NNZ path is still selected only by SCX_ACCEL_WILCOXON_NNZ in the fresh benchmark child.
Unprojected CSR col_min, col_max, and col_var now call the reader's whole-axis methods (and their masked twins). col_var can change in low bits to match X.var(axis=0); projected handles retain projected kernels.
The CSC slab split reaches all five col_* CSC reductions and calculate_qc_metrics' fused gene-axis path through col_sums_and_nnz_projected_csc. It changes slab/memory behavior, not the per-column accumulation order.
The documentation now materially qualifies the DE claims by cache regime and correctly describes the new tests as regression pins.

Checks

I ran gh pr checks 559 against bbd4a84 before posting. Format, Changed-path gate, Clippy, both feature-matrix clippy legs, and I-ORG-1 dedup guards pass; Docs anchors is skipped. Test, Test (hdf5 features), Base install, and Python bindings are still pending. CI is therefore not complete, and this is not a regression-free/pass verdict.

Reviewed by codex - gpt-5.6-terra.

nick-youngblut · 2026-09-23T16:57:39Z

Review: PR #559 (Round 2) — accel: measure the CSC routes that do not auto-route, and fix the two that lost for a reason

Reviewer: Antigravity - Gemini 3.8 Flash
Commit Reviewed: bbd4a847ddd80c317d990f68ba6c5a21dafc6d7a
CI Checks Status: gh pr checks 559 reports all checks passing for commit bbd4a847 (10 successful, 1 skipped, 0 failing, 0 pending):

CI/Base install (no extras): pass (5m10s)
CI/Clippy (pull_request): pass (1m8s)
CI/Feature matrix (clippy, hdf5 legs): pass (1m25s)
CI/Feature matrix (clippy, no-hdf5 legs): pass (2m8s)
CI/Python bindings (pull_request): pass (8m38s)
CI/Test (pull_request): pass (7m48s)
CI/Test (hdf5 features): pass (4m13s)
CI/Changed-path gate: pass (4s)
CI/Format (pull_request): pass (14s)
CI/I-ORG-1 dedup guards: pass (47s)
CI/Docs anchors (docs-only PRs): skipped

Defects (Ranked by Severity)

[Low] Unchecked dictionary indexing in fresh worker string template (benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:454)
In _FRESH_WORKER, the op key is looked up via direct dictionary indexing:
```
"route": b._extract_route(adata, b._VARIANT_OP_KEY[key]),
```
In contrast, the in-process runner run() (line 551) safely uses _VARIANT_OP_KEY.get(key). While "bench_csc__de_csc_nnz" is registered in _VARIANT_OP_KEY, if any future variant that does not stamp a route on adata.uns (e.g., col_sums or other reductions that return ndarrays) is added to _FRESH_PROCESS_VARIANTS, the fresh child process will crash with an unhandled KeyError rather than reporting route: None.
- Simpler alternative: Use b._VARIANT_OP_KEY.get(key, "").

(No other defects found; the previous round's blockers have been cleanly resolved.)

Over-Engineering & Simplification Opportunities

Single-entry dictionary dispatch mapping for fresh worker (benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:117-119, 467)
- Problem: _FRESH_PROCESS_VARIANTS is a dictionary wrapping a single variant key:
```
_FRESH_PROCESS_VARIANTS: dict[str, tuple[dict[str, str], str]] = {
    "bench_csc__de_csc_nnz": ({"SCX_ACCEL_WILCOXON_NNZ": "1"}, "cpu_csc_nnz"),
}
```
  _run_fresh_process(key, ...) is called only when key in _FRESH_PROCESS_VARIANTS and immediately unpacks env, expected = _FRESH_PROCESS_VARIANTS[key].
- Simpler alternative: Replace the dictionary dispatch with an explicit helper, e.g., _run_fresh_de_csc_nnz(csc_path, result, n_runs) that passes the literal env={"SCX_ACCEL_WILCOXON_NNZ": "1"} and expected="cpu_csc_nnz" to run_arm, and call it directly in run() via if key == "bench_csc__de_csc_nnz": return _run_fresh_de_csc_nnz(...).
Excessive process timeout constant for fresh worker (benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:122)
- Problem: _FRESH_PROCESS_TIMEOUT_S = 3 * 3600 (3 hours) is configured for an operation documented as taking ~70 s on Tabula Sapiens and ~4 min on Census 1M. In the event of a worker hang (deadlock or infinite loop), the runner will hold resources for 3 hours before failing.
- Simpler alternative: Hard-code a tighter timeout such as 1800 (30 minutes) or 600 (10 minutes).

Prior Round Findings Audit (Commit `bbd4a847`)

Below is the line-by-line reconciliation of the findings from Round 1 against commit bbd4a847:

Census DE arms fail on singleton groupby levels under the 0.17 two-cell guard (Codex Defect P2, Cursor Defect 1)

Status: Closed.

Settling Code: benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:265-299:

def _unlabel_undersized_groups(adata: Any, groupby: str) -> bool:
    col = adata.obs[groupby]
    if not hasattr(col, "cat"):
        col = col.astype("category")
    counts = col.value_counts()
    col = col.cat.remove_categories([c for c in col.cat.categories if counts.get(c, 0) < 2])
    adata.obs[groupby] = col
    return len(col.cat.categories) >= 2


def _run_de(adata: Any, prefer: str) -> None:
    obs_cols = list(adata.obs.columns)
    candidates = ["cell_type", "leiden", "louvain", "cluster", "perturbation"]
    groupby = next((c for c in candidates if c in obs_cols), None)
    if groupby is not None and not _unlabel_undersized_groups(adata, groupby):
        groupby = None
    if groupby is None:
        n = adata.n_obs
        adata.obs["_bench_group"] = (np.arange(n) < n // 2).astype(str)
        groupby = "_bench_group"

By removing categories with < 2 cells via remove_categories, those cells become NaN (unlabelled) and stay in the rank pool / "rest" without tripping the 2-cell minimum check on participating groups. If < 2 groups remain, it cleanly falls back to the deterministic 2-way synthetic split. Tested comprehensively in benchmarks/comprehensive/tests/test_bench_csc_dispatch_groupby.py.

Parallel synchronized dictionaries for fresh process configuration (Codex / Cursor / Antigravity Over-engineering)

Status: Closed.

Settling Code: benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:117-119, 467:

_FRESH_PROCESS_VARIANTS: dict[str, tuple[dict[str, str], str]] = {
    "bench_csc__de_csc_nnz": ({"SCX_ACCEL_WILCOXON_NNZ": "1"}, "cpu_csc_nnz"),
}
...
def _run_fresh_process(key: str, csc_path: Path, result: BenchmarkResult, n_runs: int):
    env, expected = _FRESH_PROCESS_VARIANTS[key]

_EXPECTED_ROUTE was removed and unified into _FRESH_PROCESS_VARIANTS.

Disagreement between default-cache caveat and unqualified bold takeaways in docs/performance.md (Cursor Defect 2)

Status: Closed.

Settling Code: docs/performance.md:1894-1906:

**§5.2 — at the default shard cache, CSR → CSC-direct is ~13.7× faster with lower
peak RSS.** The CSR streamer re-decodes every shard for each gene-chunk (here ~123
chunks × 7 shards on the full 61.5K-gene matrix, cache-bound), while the CSC-direct
route reads each column-chunk exactly once — so on a sidecar file, in the bounded
cache a backed handle opens with, the `auto` default's CPU routing is a large,
measured win. Against a CSR route allowed to cache the whole file it is the
1.95× / 1.45× above. ... **§5.3 — the exact sparse-nnz kernel adds ~1.58×** over CSC-densify by
ranking only nonzeros + an analytic zero block instead of an `n_obs` dense sort,
for **~21.7× end-to-end** over the old CSR default at the same default cache, at
lower peak RSS. Bit-identical to the dense kernel (pinned at zero tolerance, above).

Misleading docstring claiming this PR fixed gene-only backed view routing (Cursor Defect 3)

Status: Closed.

Settling Code: pyscx/tests/test_accel_axis_view.py:474-481:

"""At the default `prefer_format="auto"`, a backed handle whose only view is
a gene projection takes `cpu_csc`.

A regression pin, not the test for a fix. The behaviour arrived when the
backed handle started serving its CSC reads through the same view a lazy
one does, which remaps columns into the projected axis. Before that its
column source was the full-axis sidecar reader, so the probe excluded a
column projection by hand and `filter_genes` alone routed `cpu_csr` while
`filter_genes + normalize_total + log1p` routed `cpu_csc`. Nothing asserted
the gene-only case since, so this pins it for all three ways of making one.
"""

Unchecked indexing b._VARIANT_OP_KEY[key] in fresh worker template (Antigravity Defect 1)
- Status: Untouched.
- Settling Code: benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:454:
```
"route": b._extract_route(adata, b._VARIANT_OP_KEY[key]),
```
  b._VARIANT_OP_KEY[key] was not replaced with .get(key, "") and remains in place.

Whole-process CPU time metrics leaking interpreter startup and import time into op measurement (Antigravity Defect 2)

Status: Closed.

Settling Code: benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py:445-453:

adata = b._open_backed(path, bounded=key in b._BOUNDED_CACHE_VARIANTS)
gc.collect()
ru0 = resource.getrusage(resource.RUSAGE_SELF)
t0 = time.perf_counter()
runner(adata, prefer)
wall = time.perf_counter() - t0
ru = resource.getrusage(resource.RUSAGE_SELF)
print(json.dumps({
    "wall_s": wall,
    "user_s": ru.ru_utime - ru0.ru_utime,
    "sys_s": ru.ru_stime - ru0.ru_stime,
    "peak_rss_mb": ru.ru_maxrss / 1024.0,
    "route": b._extract_route(adata, b._VARIANT_OP_KEY[key]),
}))

Test coverage gap for walk_csc_runs shard-boundary splitting on non-contiguous projections skipping shards (Antigravity Defect 3)

Status: Closed.

Settling Code: pyscx/tests/test_col_projected_agg.py:539-572:

@pytest.mark.parametrize("op", ["col_sums", "col_nnz", "col_min", "col_max", "col_var"])
def test_csc_col_ops_on_a_projection_that_skips_whole_shards(tmp_dir, op):
    ...
    cols = list(range(5, 8)) + list(range(28, 35)) + [39]  # shards 0, 3, 4; skips 1-2
    adata = pyscx.open(path).to_anndata(backed=True)
    pyscx.accel.subset_var(adata, cols)
    got = np.asarray(getattr(pyscx.accel, op)(adata.X, prefer_format="csc"), dtype=np.float64)
    csr = np.asarray(getattr(pyscx.accel, op)(adata.X, prefer_format="csr"), dtype=np.float64)
    sub = dense[:, cols].astype(np.float64)
    ref = { ... }[op]
    np.testing.assert_allclose(got, ref, rtol=1e-6, atol=1e-9)
    np.testing.assert_allclose(got, csr, rtol=1e-9, atol=1e-12)

Redundant sorting and duplicate entries in shard_starts (Antigravity Over-engineering 2)

Status: Closed.

Settling Code: pyscx/src/projected_agg.rs:1068-1072:

// Sorted rather than assumed sorted (the trait promises no order), and
// deduplicated: under a projection a shard it skips entirely has an empty
// range whose start repeats the next shard's.
shard_starts.sort_unstable();
shard_starts.dedup();
let starts_shard = |c: u32| shard_starts.binary_search(&c).is_ok();

Analysis of Incidental Changes & Potential Regressions in `bbd4a847`

Inspection of all files modified in bbd4a847 confirms no regressions were introduced:

benchmarks/comprehensive/benchmarks/bench_csc_dispatch.py: Category removal properly produces NaNs which Scanpy / pyscx treats as unlabelled samples participating in the ranking pool and reference ("rest"), avoiding the two-cell minimum crash without changing cell set membership.
test_bench_csc_dispatch_groupby.py: Verifies both the error premise without the helper, normal execution on both CSR/CSC routes with singletons, and the fallback to synthetic split when < 2 categories remain.
test_col_projected_agg.py: Verifies numerical agreement for all 5 column aggregations against both dense slices and CSR under shard-skipping projections.
pyscx/src/projected_agg.rs: Adding .dedup() ensures binary search invariants hold when empty projection ranges produce duplicate start indices.

PR Description Correction Block Verification

The PR description added a dedicated correction block:

## Census DE arms of bench_csc_dispatch (fixed in bbd4a847)

Claims:

_run_de unlabels groupby levels with < 2 cells and drops unused levels before calling rank_genes_groups.

Falls back to synthetic split when fewer than 2 levels remain.

test_bench_csc_dispatch_groupby.py checks both routes and the fallback.

Verification: The claims match the diff precisely. _unlabel_undersized_groups removes categories with count < 2 via col.cat.remove_categories, turning them to NaN. If fewer than 2 categories remain, _run_de falls back to _bench_group. test_bench_csc_dispatch_groupby.py covers each behavior explicitly.

Summary & Blast Radius

Blast Radius: Well-scoped and verified.
- CSR Optimizations (pyscx/src/accel/col_aggs.rs): Eliminates redundant project_csr copies on unprojected col_min, col_max, and col_var, bringing them in line with col_sums/col_nnz and matching the dunders bit-for-bit.
- CSC Slab Bounding (pyscx/src/projected_agg.rs): Limits memory residency to one CSC shard per slab walk instead of materializing the whole sidecar into a single slab on unprojected handles.
- Exact-nnz Wilcoxon Parity (scx-accel/src/csc/wilcoxon.rs): Zero-tolerance parity against the densify kernel on tie-heavy count matrices, establishing exact correctness before any promotion decision.
- Documentation & Benchmarks: Accurately clarifies cache dependency (13.7×/21.7× on default 4-shard cache vs 1.95×/1.45× on whole-file cache) and CPU vs GPU routing trade-offs.

nick-youngblut · 2026-09-23T17:03:22Z

Cursor Agent - Grok 4.7 High

Reviewed bbd4a847 (full diff against d55ebf4f, the fix commit, the callers of walk_csc_runs, ColumnShardSource::csc_shard_col_range, and the two-cell check in rank_genes_groups).

Defects

1. Low — `thresholds.yaml` still says every `_csc` variant of this benchmark is gated

The comment above the new floor still says the five AnnData ops stamp a route, "so every _csc variant is gated." This PR adds bench_csc__col_sums_csc and bench_csc__col_var_csc. Those keys are absent from _VARIANT_OP_KEY and from absolute_floors. A missing sidecar on an explicit prefer_format="csc" raises rather than falling back (the module docstring says so), so the missing floor does not hide a silent wrong-layout timing. The comment is what is wrong: it now describes a gate that does not cover the variants this PR added. Name the gated set (the five AnnData ops, plus de_csc_nnz under the stricter cpu_csc_nnz check) and state that the col_* arms are outside it.

Over-engineering

_FRESH_PROCESS_VARIANTS has one entry, and _run_fresh_process exists to look that entry up. run() already branches on key in _FRESH_PROCESS_VARIANTS. Inline the one variant: a helper that calls run_arm with the literal environment {"SCX_ACCEL_WILCOXON_NNZ": "1"} and compares the recorded route to the literal "cpu_csc_nnz". Keep run_arm. The fresh process has to stay, because SCX_ACCEL_WILCOXON_NNZ is read once per process through a OnceLock.

Prior round (`0955fc05`) vs `bbd4a847`

Codex P2 / Grok Medium — census `_run_de` raises on a one-cell `cell_type` level. Closed.

counts = col.value_counts()
col = col.cat.remove_categories([c for c in col.cat.categories if counts.get(c, 0) < 2])
adata.obs[groupby] = col
return len(col.cat.categories) >= 2

_run_de sets groupby = None when that returns false, and the existing synthetic _bench_group split runs. A count of 0 and a count of 1 both fail < 2, so unused levels and one-cell levels both become NaN via remove_categories. I checked Series.get against a CategoricalIndex on pandas 2.3.3, including integer categories and a zero-count level: levels under 2 are removed, levels at 2 or more are kept. test_bench_csc_dispatch_groupby.py builds that column, asserts the bare rank_genes_groups call raises Could not calculate statistics, and asserts _run_de leaves categories ["a", "b"] for prefer of csr and csc.

Codex NIT — replace the maps and the keyed dispatcher with one NNZ helper. Partially fixed.

The two dicts are now one:

_FRESH_PROCESS_VARIANTS: dict[str, tuple[dict[str, str], str]] = {
    "bench_csc__de_csc_nnz": ({"SCX_ACCEL_WILCOXON_NNZ": "1"}, "cpu_csc_nnz"),
}

_run_fresh_process still does env, expected = _FRESH_PROCESS_VARIANTS[key]. One key, one caller. See over-engineering.

Grok over-engineering — collapse the two dicts into one. Closed. Same tuple dict.

Grok Low — the bold 13.7× / 21.7× sentences had no cache qualifier. Closed.

docs/performance.md §5.2 now starts "at the default shard cache" and points at the file-cache 1.95× (tabula) / 1.45× (census_1m), which the paragraph above the table states. §5.3 qualifies the 21.7× the same way.

Grok Low — `test_auto_takes_csc_on_a_gene_only_projection` documented a fix this PR did not make. Closed.

The docstring now says "A regression pin, not the test for a fix" and attributes the route to the backed handle serving CSC through the same view a lazy handle does.

Antigravity Medium — `_VARIANT_OP_KEY[key]` can KeyError in the fresh worker. Untouched.

The worker still prints "route": b._extract_route(adata, b._VARIANT_OP_KEY[key]). It does not fire on this diff: the only fresh-process key, bench_csc__de_csc_nnz, is in _VARIANT_OP_KEY. A later col_* entry added only to the fresh-process map would KeyError in the child, run_arm would see a non-zero exit, and _run_fresh_process would return None.

Antigravity Low — fresh-worker CPU time included interpreter startup. Closed.

ru0 = resource.getrusage(...) is taken after _open_backed and gc.collect(), and the record is ru.ru_utime - ru0.ru_utime / ru.ru_stime - ru0.ru_stime. peak_rss_mb stays ru.ru_maxrss / 1024.0, the process high-water mark with the open included. The docstring says that.

Antigravity Low — no test for a column projection that skips CSC shards. Closed.

test_csc_col_ops_on_a_projection_that_skips_whole_shards writes csc_cols_per_shard=8 and keeps range(5, 8) + range(28, 35) + [39]. Shards 1 and 2 are empty, and the kept run crosses the boundary between shard 3 ([24, 32)) and shard 4 ([32, 40)). All five col_* ops are compared to the dense slice and to prefer_format="csr".

Antigravity over-engineering — two parallel dicts. Closed. The tuple dict above.

Antigravity over-engineering — replace `sort_unstable` with `dedup`. Partially fixed; the sort that remains is warranted.

shard_starts.sort_unstable();
shard_starts.dedup();

dedup is in. sort_unstable stayed, and the comment says the trait promises no order. ColumnShardSource::csc_shard_col_range documents a per-index range and does not promise that shard-index order is column order. Duplicates do not change binary_search(&c).is_ok(); dedup matches the empty-shard case, where LazyShardSource reports (p_lo, p_lo) for a shard the projection misses and that start repeats the next shard's.

Gemini — `_FRESH_PROCESS_TIMEOUT_S = 3 * 3600`. Untouched.

The constant is still 3 * 3600. I am not re-filing it. estimate_time_minutes for bench_csc_dispatch is a 45-minute base plus 8 minutes per million cells, so the SLURM job walltime reaps a hung worker before this cap does.

Correction block

The new "Census DE arms of bench_csc_dispatch" section matches the diff. _run_de unlabels levels with fewer than two cells, and a false return selects the synthetic split. rank_genes_groups's default reference is "rest", so those NaN cells stay in the rank pool and in every group's rest. The test covers the raise, both prefer values, and the fallback. It uses a synthetic column with a one-cell level and an unused level, not a census fixture.

Blast radius

No prefer_format default moves. HVG stays CSR. The exact-nnz Wilcoxon kernel stays behind SCX_ACCEL_WILCOXON_NNZ.

On an unprojected backed handle, col_min / col_max / col_var call BackedCsrReader::col_{min,max,var} and the masked twins — the four-way split col_sums / col_nnz already used. On a multi-shard file, col_var changes in the low bits and matches X.var(axis=0). A column projection still goes through projected_agg. walk_csc_runs breaks at shard starts for every CSC col_* kernel, including the gene-axis half of calculate_qc_metrics (col_sums_and_nnz_projected_csc). A column lives in one shard, so the split changes how large a slab is held and does not change which values are accumulated or their order.

_unlabel_undersized_groups runs for every _run_de caller (de_csr, de_csc, de_csr_bounded, de_csc_nnz). On a column whose levels all have two or more cells the removed-category list is empty, so a tabula run that already succeeded keeps the same grouping. _run_pdex_ref is unchanged: it still picks perturbation then cell_type with no singleton handling.

CI

gh pr checks 559 for bbd4a847ddd80c317d990f68ba6c5a21dafc6d7a: Format, Changed-path gate, Clippy, both feature-matrix clippy legs, I-ORG-1 dedup guards, Test, Test (hdf5 features), Base install, and Python bindings pass. Docs anchors is skipping (this is not a docs-only PR). That is CI green on this commit. Python bindings runs pyscx pytest, which includes the new col-agg and axis-view tests; cargo test includes nnz_matches_the_densify_kernel_exactly_on_tie_heavy_counts. benchmarks/comprehensive/tests/test_bench_csc_dispatch_groupby.py is in neither job, and the census bench_csc_dispatch arms are not in CI, so this green run does not execute them.

…n honest gate comment Round-2 review fixes for PR #559. - The single fresh-process variant is a dedicated helper with its literal environment and expected route, not a one-entry registry, which also removes the unguarded route-key lookup in the worker. (codex, Cursor Agent, Antigravity) - A fresh child times out after 30 minutes, not 3 hours: ~30x the slowest observed run (56 s at census_1m). (codex, Antigravity) - The thresholds.yaml comment no longer says every `_csc` arm of the benchmark is gated; the col_* arms stamp no route and are outside it. (Cursor Agent)

…SC DE 1-vs-rest `rank_genes_groups` on the CSC route now runs the exact sparse-nnz kernel (route `cpu_csc_nnz`) by default. SCX_ACCEL_WILCOXON_NNZ=0 falls back to the densify kernel (`cpu_csc`); unset or any other value leaves the default on. `reference=` and `rankby_abs=True` still use densify, which is the only kernel that handles them, and pdex_ref is untouched. The evidence, from this PR: bit-identical output to densify at zero tolerance on a tie-heavy matrix (scores, p-values, adjusted p-values, log fold changes and gene order), and 3.1x (tabula_sapiens_100k, 19.6 -> 6.2 s) and 4.1x (census_1m, 225.9 -> 55.7 s) faster at the same or lower peak RSS. - The densify kernel is its own function, so the exact-parity test and the CSR-parity / non-finite tests call it directly rather than through the process-global gate, which the default no longer lets reach it. - bench_csc_dispatch: `de_csc` now times the nnz default and is floored on route `cpu_csc_nnz`; the fresh-process arm becomes `de_csc_densify` (SCX_ACCEL_WILCOXON_NNZ=0, floored on `cpu_csc`), the control that shows what the default is worth. - bench_de_csc_routes.py sets the gate itself from --mode before importing pyscx, so a mode always names the kernel that runs. - Tests asserting `cpu_csc` for a 1-vs-rest Wilcoxon call now expect `cpu_csc_nnz`; values were already compared against CSR and pass unchanged. - Docs (architecture env table, api route list, scanpy, performance, the usage skill) describe the new default and the opt-out.

All 16 versioned workspace members, `pyscx/pyproject.toml` and `rscx/DESCRIPTION`; `tests/scx-integration-tests` stays at `0.0.0`. README's `scx-cli` download snippet follows. (ROADMAP.md no longer exists, so there is no date stamp to move.) 0.20.0 collects the merged PRs since 0.19.0 (#544-#561): - CSC sidecar series: one-pass bucketed CSC builder (#555); `build-csc` as an in-place, rollback-able append (#556); same-pass CSC build on ingest and carry-through on rewrite ops (#557); parallel CSC build and `csc="auto"` as the ingest default (#558); CSC dispatch for row-indexed transforms and row-filtered / gene-subset handles (#553, #554); CSC route coverage and the exact-nnz Wilcoxon kernel as the 1-vs-rest CSC default (#559) - Correctness: Leiden parallel local moving keeps decliners eligible (#544); five CPU-accelerator parity / convergence fixes (#548); GPU cuVS stream sync, reduction determinism, decoder bounds and VRAM accounting (#549); CLI destination-overwrite safety and MTX multimodal / streaming / integer integrity (#550); unscoped whole-matrix reads of a multimodal file refused (#551); dictionary (categorical) output from append / merge / merge --sort-by (#546) and `scx sort`'s obs spill path (#547) - Benchmarks and docs: four community analytical benchmarks (#552), laptop-test recapture (#560), README refresh (#545), docs split into per-topic directories (#561) Behaviour changes worth calling out in the release notes: - **`csc="auto"` is the ingest default** on every entry point and preset (`from_mudata` keeps `off`): files with `n_obs >= 50000` and `n_vars >= 5000` now get a CSC sidecar, costing ingest wall time and +42-71 % on disk. `--csc off` / `csc="off"` opts out (#558). - **Rewrite ops carry the CSC sidecar by default** (`compact`, `merge`, `optimize`, `sort`, `subset`), rebuilt from the output's own shards; `--csc carry|always|off`. `--rebuild-csc` / `rebuild_csc=True` are deprecated aliases for `always` (#557). - **`scx build-csc` appends in place** rather than rewriting the file, and `scx rollback` removes the sidecar (#556). - **Ten CLI subcommands no longer silently overwrite an existing destination**; `--force` is required (convert, merge, subset, query --output, upgrade, cloud-optimize, explode, pack, pull; compact / optimize / sort migrated onto the same guard) and is refused where nothing is written (#550). - **MTX**: a declared-`integer` MTX with a value past 2^24, a non-integral value or `nan`/`inf` is refused on ingest (`--allow-lossy` restores the old behaviour); multimodal MTX export requires a modality (`pyscx.to_mtx` gains `modality=`); export streams one shard at a time (#550). - **An unscoped whole-matrix read of a multimodal file raises** `MultimodalRequiresModality` instead of folding every modality into one `n_obs x n_modalities`-row answer — `to_anndata()`, `to_memory()`, `read_all_csr_shards*`, `scx pull --filter` (#551). - **Numerical changes**: UMAP init scale and `random_init` now match umap-learn, the kNN sigma search, and NB-GLM dispersion shrinkage (the `above_min_disp` residual filter always runs; new `disp_outlier_sd=2.0` carve-out, `None` disables only the carve-out) (#548); parallel Leiden labels change (#544). - **Wilcoxon on the CPU CSC route** defaults to the exact-nnz kernel (route id `cpu_csc_nnz`); `SCX_ACCEL_WILCOXON_NNZ=0` restores densify (`cpu_csc`), and `reference=` / `rankby_abs=True` still use densify (#559). - **Categorical obs columns** keep their declared categories, unused levels and `ordered` bit through append / merge / sort (#546, #547). No benchmark recapture for this release. Pre-release gate on this tree: `cargo fmt --check`; `cargo clippy --workspace --exclude rscx --all-targets -D warnings`; `cargo test --workspace --exclude rscx` (4,220 passed, 0 failed); `cargo test -p scx-convert --features hdf5` (474 passed); `cargo test -p scx-cli --features hdf5 -- --test-threads=1` (253 passed); `maturin develop --release` + `pytest tests/` (3,247 passed, 140 skipped, 2 xfailed; `pyscx.__version__ == "0.20.0"`). Not run: rscx tests, `--features cloud`, GPU pytest, fuzzing, benchmarks. Claude-Session: https://claude.ai/code/session_01RwRKstgr4TNkXhLhkQwmjA Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

gemini-code-assist Bot reviewed Sep 23, 2026

View reviewed changes

nick-youngblut added 2 commits September 23, 2026 10:08

nick-youngblut changed the title ~~accel: measure the CSC routes that do not auto-route, and fix the two that lost for a reason~~ accel: promote exact-nnz Wilcoxon to the CSC default, and fix the two CSC routes that lost for a reason Sep 23, 2026

nick-youngblut merged commit 819bfd7 into main Sep 23, 2026
11 checks passed

nick-youngblut deleted the pr-g-csc-route-coverage branch September 23, 2026 18:07

nick-youngblut mentioned this pull request Sep 24, 2026

chore: bump workspace version to 0.20.0 #562

Merged

Navigation Menu

Repository navigation

accel: promote exact-nnz Wilcoxon to the CSC default, and fix the two CSC routes that lost for a reason - #559

accel: promote exact-nnz Wilcoxon to the CSC default, and fix the two CSC routes that lost for a reason#559
nick-youngblut merged 4 commits into
mainfrom
pr-g-csc-route-coverage

nick-youngblut commented Sep 23, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Sep 23, 2026

Uh oh!

nick-youngblut commented Sep 23, 2026

Uh oh!

nick-youngblut commented Sep 23, 2026 •

edited

Loading

Uh oh!

nick-youngblut commented Sep 23, 2026

Uh oh!

nick-youngblut commented Sep 23, 2026 •

edited

Loading

Uh oh!

nick-youngblut commented Sep 23, 2026

Uh oh!

nick-youngblut commented Sep 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	_FRESH_PROCESS_TIMEOUT_S = 3 * 3600
	_FRESH_PROCESS_TIMEOUT_S = 1800

Conversation

nick-youngblut commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update — the exact-nnz Wilcoxon kernel is now the default (c859d286)

col_*: CSR stays the default, and CSR col_min / col_max / col_var get 7–9× faster

HVG: stays CSR; prefer_format is not deprecated

Backed handle + gene projection: already routes CSC

Docs: which shard-cache regime each number comes from

Exact-nnz Wilcoxon: now the default

Census DE arms of bench_csc_dispatch (fixed in bbd4a847)

Local verification

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

nick-youngblut commented Sep 23, 2026

Defects

Over-engineering

Checks

Uh oh!

nick-youngblut commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Defects

1. Medium — bench_csc__de_csc_nnz cannot collect the census number this PR is evidence for

2. Low — the new cache caveat and the bold conclusion under the same table disagree

3. Low — test_auto_takes_csc_on_a_gene_only_projection documents a fix this PR did not make

Over-engineering

Blast radius

CI

Uh oh!

nick-youngblut commented Sep 23, 2026

Review: PR #559 — accel: measure the CSC routes that do not auto-route, and fix the two that lost for a reason

Defects (Ranked by Severity)

Over-Engineering & Simplification Opportunities

Summary & Blast Radius

Uh oh!

nick-youngblut commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Defects

Over-engineering

Prior-round findings vs bbd4a84

Correction block and blast radius

Checks

Uh oh!

nick-youngblut commented Sep 23, 2026

Review: PR #559 (Round 2) — accel: measure the CSC routes that do not auto-route, and fix the two that lost for a reason

Defects (Ranked by Severity)

Over-Engineering & Simplification Opportunities

Prior Round Findings Audit (Commit bbd4a847)

Analysis of Incidental Changes & Potential Regressions in bbd4a847

PR Description Correction Block Verification

Summary & Blast Radius

Uh oh!

nick-youngblut commented Sep 23, 2026

Defects

1. Low — thresholds.yaml still says every _csc variant of this benchmark is gated

Over-engineering

Prior round (0955fc05) vs bbd4a847

Codex P2 / Grok Medium — census _run_de raises on a one-cell cell_type level. Closed.

Codex NIT — replace the maps and the keyed dispatcher with one NNZ helper. Partially fixed.

Grok over-engineering — collapse the two dicts into one. Closed. Same tuple dict.

Grok Low — the bold 13.7× / 21.7× sentences had no cache qualifier. Closed.

Grok Low — test_auto_takes_csc_on_a_gene_only_projection documented a fix this PR did not make. Closed.

Antigravity Medium — _VARIANT_OP_KEY[key] can KeyError in the fresh worker. Untouched.

Antigravity Low — fresh-worker CPU time included interpreter startup. Closed.

Antigravity Low — no test for a column projection that skips CSC shards. Closed.

Antigravity over-engineering — two parallel dicts. Closed. The tuple dict above.

Antigravity over-engineering — replace sort_unstable with dedup. Partially fixed; the sort that remains is warranted.

Gemini — _FRESH_PROCESS_TIMEOUT_S = 3 * 3600. Untouched.

Correction block

Blast radius

CI

Uh oh!

Uh oh!

Reviewers

Assignees

nick-youngblut commented Sep 23, 2026 •

edited

Loading

Update — the exact-nnz Wilcoxon kernel is now the default (`c859d286`)

`col_*`: CSR stays the default, and CSR `col_min` / `col_max` / `col_var` get 7–9× faster

HVG: stays CSR; `prefer_format` is not deprecated

Census DE arms of `bench_csc_dispatch` (fixed in `bbd4a847`)

nick-youngblut commented Sep 23, 2026 •

edited

Loading

1. Medium — `bench_csc__de_csc_nnz` cannot collect the census number this PR is evidence for

3. Low — `test_auto_takes_csc_on_a_gene_only_projection` documents a fix this PR did not make

nick-youngblut commented Sep 23, 2026 •

edited

Loading

Prior-round findings vs `bbd4a84`

Prior Round Findings Audit (Commit `bbd4a847`)

Analysis of Incidental Changes & Potential Regressions in `bbd4a847`

1. Low — `thresholds.yaml` still says every `_csc` variant of this benchmark is gated

Prior round (`0955fc05`) vs `bbd4a847`

Codex P2 / Grok Medium — census `_run_de` raises on a one-cell `cell_type` level. Closed.

Grok Low — `test_auto_takes_csc_on_a_gene_only_projection` documented a fix this PR did not make. Closed.

Antigravity Medium — `_VARIANT_OP_KEY[key]` can KeyError in the fresh worker. Untouched.

Antigravity over-engineering — replace `sort_unstable` with `dedup`. Partially fixed; the sort that remains is warranted.

Gemini — `_FRESH_PROCESS_TIMEOUT_S = 3 * 3600`. Untouched.