Skip to content

0.1.8: tied genes untested in wilcoxon_test, safer result reuse and resume - #58

Merged
jaydu1 merged 24 commits into
mainfrom
library-fixes
Oct 5, 2026
Merged

jaydu1 merged 24 commits into
mainfrom
library-fixes

Conversation

@jaydu1

@jaydu1 jaydu1 commented Oct 5, 2026

Copy link
Copy Markdown
Member

Summary

0.1.8 fixes findings from a code review of 0.1.7 and consolidates the test suite.

Behaviour changes

  • A gene whose values are all tied across control and perturbation is now untested in wilcoxon_test: NaN in every derived column, with or without tie_correct and with or without batch_column. It used to be score 0, p 1, as in Scanpy. SCHEMAS["de_result"] goes to 2, so DE results written by 0.1.7 are recomputed once.
  • min_pct_both is removed from the DE functions. Use min_pct_ctrl and min_pct_pert.

Fixes

  • A checkpoint that differs from the current call only in arguments left at None now resumes. Before, every 0.1.7 checkpoint was deleted silently after min_pct_both was removed. A resume that can't use the checkpoint it finds now warns and says why.
  • An argument left at None no longer invalidates a finished result.
  • groupby/reference and perturbation_column/control_label now count as the same call. cx.tl.rank_genes_groups and a direct DE call on the same file now reuse each other's results. wilcoxon_test checks for reuse before reading the input's labels.
  • wilcoxon_test(scanpy_format=True) writes uns["rank_genes_groups"] at any result size. It used to skip it, while recording that it had been written, when the result was too large to load. The result's provenance stamp is written with the file, before the checkpoint is removed, so a run killed while the result is read back is reused rather than recomputed.
  • shrink_lfc(method="full") honours memory_limit_gb, including the batches joblib queues per worker. It sizes its workers once instead of once per perturbation.
  • nb_glm_test records in uns["frozen_control"] whether it fitted with frozen control statistics.

Cleanup

  • Seven unused Wilcoxon kernels are deleted, about 800 lines. The kernel test now checks the production kernel against scipy's ranks.
  • The test suite is consolidated (about 3,000 fewer lines).

Commits

  • b335574…4459878: test-suite consolidation.
  • c0856ac resolves rank_genes_groups aliases with the shared helper.
  • 64d873d honours memory_limit_gb in shrink_lfc(method="full").
  • 7221270 stops None arguments from invalidating cached results.
  • e032ced removes min_pct_both.
  • cf6012c reports all-tied genes as untested in wilcoxon_test.
  • Fixes from a review of the commits above:
    • 4caa9ab treats all-tied genes as untested whatever tie_correct is, and bumps the DE result schema.
    • 98033c2 deletes the unused Wilcoxon kernels.
    • 2df193d resumes checkpoints that differ only in arguments left at None.
    • effff10 lets aliases and rank_genes_groups share DE results.
    • 5165f6a writes the Wilcoxon Scanpy structure and stamp together with the result.
    • 5aa2078 records nb_glm's frozen-control mode.
    • 1c57938 budgets joblib's queued batches in the apeGLM worker count.
  • c50e245 bumps the version to 0.1.8.
  • bf4eacc re-executes the tutorials on 0.1.8.

Notes for review

  • With freeze_control=None, the mode nb_glm actually uses is deliberately left out of the result fingerprint. It is chosen from the memory available at run time, so including it would recompute identical calls whenever free memory changed. Pass True or False to require one mode.
  • The schema bump recomputes t_test and nb_glm results as well as Wilcoxon ones, once.

Testing

  • The full suite passes in the pert environment (Python 3.11.9) and in base (Python 3.10.12).
  • Each review-fix commit was checked out on its own and passed the tests for what it changes.
  • The new regression tests were run against cf6012c. Those for the stratified tie_correct=False case, checkpoint matching, aliases, the large-result Scanpy structure and the stamp ordering fail there and pass here.
  • Both tutorials were re-executed in pert from an empty tutorial_outputs/, with no errors, warnings or absolute paths. Apart from timings, measured read speeds and the last digit of one PCA variance, every output is unchanged.

🤖 Generated with Claude Code

jaydu1 and others added 24 commits October 4, 2026 11:18
The package is installed editable, so the sys.path inserts in 25 files
were dead code. Real-data paths in test_qc_parity no longer depend on the
working directory, the hand-rolled temp dir becomes tmp_path, try/except
assertions become pytest.raises, and tests use min_pct_ctrl/min_pct_pert
instead of the deprecated min_pct_both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- test_result_provenance's reuse test now compares every result field and
  the force=True hint; a new test covers force=True recomputing. This
  replaces the per-method AutoReload classes in test_de_reload.py and the
  reload test in test_rank_genes_groups_outputs.py.
- test_de_reload.py becomes test_pickle.py with the pickle round trips
  only (8 tests -> 4).
- Two batch_process cache tests duplicated the provenance tests, and two
  workflow 'resume' tests only re-ran a finished job; test_de_resume
  covers real interruption.
- test_groupby_alias.py tests the shared resolver directly and checks
  that each entry point's aliases give results equal to the canonical
  names, instead of 26 'result is not None' runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The streaming decision was tested in three places with hard-coded
  numbers (and docstrings disagreeing on the peak multiplier). It is now
  one contract test plus one table of benchmark datasets in
  test_memory_dispatch.py, next to the chunk-size heuristics.
- Standard-vs-streaming Wilcoxon parity compares every result field. The
  four Adamson tests each ran both full paths to compare one field; one
  test on 2,000 genes of the same data replaces them (~148 s -> 3 s).
- Presort and kernel-parity tests become two parametrized tests; result
  file helper tests are merged.
- memory_limit_gb smoke tests that only checked shapes now assert that
  the result does not depend on the limit. The shrink_lfc ones are
  dropped: shrink_lfc accepts memory_limit_gb but never uses it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test_convert_to_csc.py and test_convert_to_csr.py were line-for-line
copies with csr and csc swapped. test_convert_format.py runs each case
for both targets and checks the stored arrays byte for byte against
scipy's canonical matrix, instead of the looser per-file checks. Dense
sources are now covered for both targets. The int64_indptr tests are
dropped: they only asserted that indptr has an integer dtype, and there is
no int32/int64 selection to test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The nine _low_expr_in_both_mask tests and four _nonestimable_glm_mask
tests were the same call with different arrays; each is now one
parametrized table with named cases. The end-to-end checks that were
copied per DE method are parametrized over the methods, and now also
check that the LFC is NaN and that the other genes stay tested. The
'legacy behaviour' test that accepted 'NaN or 1.0' now pins t_test's
actual result. The min_pct_both test now checks the override, not just
the warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CSC parity test read data/Tian-crispra.h5ad, which is not tracked by
git, so it skipped in CI. One parametrized test now compares each
streaming strategy (column-oriented; row-oriented with memmap, memory and
no cache; delta and full-recompute gene counts) against in-memory QC on a
synthetic screen in CSR, CSC and dense storage. It also checks the
written file, and asserts that every filter actually removed something.

Removed: the three '_basic' tests (subsets of the parity test), the
strategy-threshold test (it re-implemented the threshold inside the test
and never called crispyx), and the storage-format test on real files
(now asserted in test_convert_format.py). The verbose-message tests run
on synthetic data.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- test_deseq2_size_factors_streaming_parity copied the streaming
  algorithm into the test and compared the copy with the in-memory path,
  so the library's streaming branch never ran. The 4 GB threshold is now
  the module constant _DESEQ2_STREAMING_THRESHOLD_GB, and the test sets
  it to 0 to run the real streaming passes. Both paths are checked
  against DESeq2's median-of-ratios definition, with and without a gene
  that has zeros.
- The two normalized-effects-vs-Scanpy tests become one test
  parametrized over method.
- The 'dense' and 'sparse' QC workflow tests wrote the same CSR input;
  they are merged into one test that checks values, gene symbols and
  encoding.
- Edge cases assert results instead of 'does not crash': a one-cell
  perturbation matches scipy's Mann-Whitney U, and an all-zero gene is
  NaN in nb_glm_test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test_dimred.py only checked shapes and uns keys; the two PCA methods
were only compared with each other. Now:
- sparse-covariance PCA matches sklearn.decomposition.PCA to machine
  precision (variance ratios, loadings and scores up to sign);
- incremental PCA matches on the well-separated leading components;
- cx.pp.pca on a backed file writes the sklearn embedding, restricted to
  highly variable genes only when asked;
- sklearn-method neighbors give exactly NearestNeighbors' sets (with and
  without n_pcs), and the pynndescent method agrees on >95% of them.
The 64 tests (47 shape/hasattr checks) become 22, using tmp_path and
local RNGs instead of temp files and the global seed.

The dense-vs-streaming nb_glm test said both paths use the same
algorithm but allowed a 5% difference; they agree to rounding, so the
tolerance is now 1e-10 on effect, statistic and both p-values.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- test_data_helpers: the NB-GLM chunk-size, backed obs/var editing and
  compute_overlap tests become parametrized tables. Overlap results are
  checked against full expected count and Jaccard matrices; this pins
  that an empty set's Jaccard index is reported as 0.
- test_disk_dispatch / test_messages: the per-assertion classes for
  estimate_bytes and the reading/saving/done printers become tables.
- test_profiling: timing runs against a fake clock, so its assertions are
  exact and the tests no longer sleep (20 tests -> 9, same coverage).
- test_plotting: plots are checked for what they draw (points, bars,
  annotations, titles) instead of 'is not None'.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- dispersion_scope='per_comparison' (the batched method-of-moments path)
  and size_factor_scope='per_comparison' had no tests. New tests check
  that the MoM estimator recovers a known dispersion and does not depend
  on gene batching; that per-comparison size factors equal DESeq2 size
  factors of the compared cells alone; and that both scopes still find
  the planted effects end to end.
- The QC strategy-parity screen is large enough to flush the subset
  writer's buffer, which before only the untracked Tian file reached.
  Direct tests cover the checkpoint-interval helper and the
  pca/neighbors/umap copy and in-place branches.
- test_reachable_via_tl_namespace compared live free-disk readings from
  two calls and failed whenever free space changed in between; it now
  compares the deterministic fields.
- Drop a namespace test subsumed by its neighbour, and imports left
  unused by the earlier edits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cx.tl.rank_genes_groups re-implemented the groupby/reference checks
inline, word for word. It now calls resolve_group_reference_aliases like
every other entry point, so the behaviour and messages cannot drift.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The parameter was documented to cap the parallel apeGLM workers but was
never read, and _estimate_max_workers, the helper for exactly this, was
imported in de.py and glm.py without being called. The worker count is
now capped so the workers fit in the budget (or the memory available
when it is None). The per-worker estimate is a batch of gene columns, as
loky sends them, not the whole matrix, so a large screen keeps its
parallelism.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Result reuse compared the stored fingerprint with the call's by plain
equality, and fingerprints record every argument including those left at
None. Adding or removing a None-default parameter therefore made every
existing result look different, so long nb_glm_test runs were redone,
with the unhelpful reason "'params' differs". reuse_mismatch now drops
None-valued params on both sides before comparing, so stamps from 0.1.7
still match. A value that was set and is now absent, or the reverse,
still triggers a recompute that names the argument.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
min_pct_both set min_pct_ctrl and min_pct_pert together. The mask helper
warned that it was deprecated, but t_test, wilcoxon_test and nb_glm_test
mapped it silently, so users never saw the warning. Following the
no-compatibility-shims rule, it is removed from the three DE functions,
the cx.tl keyword lists and _low_expr_in_both_mask. _resolve_de_aliases
then did nothing beyond the shared group/reference resolver, so the
callers use that directly. The previous commit keeps stored results
reusable despite the dropped argument.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When every value of a gene is tied across control and perturbation (e.g.
zero in every cell, which only reaches the test with the low-expression
filters off), the rank-sum z is 0/0. The kernels wrote z=0, p=1 and the
LFC came out as 0: 'tested, no change', as in Scanpy. t_test and
nb_glm_test report NaN for the same gene, and crispyx's contract is that
an untested comparison is NaN in every derived column.

All six Wilcoxon kernels now write NaN for U, z, p and the effect in that
case, and the standard, streaming and stratified paths set the LFC to NaN
for those genes. The stratified path still keeps the pooled LFC for
perturbations that share no batch with control. Documented in
wilcoxon_test's docstring and the changelog.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cf6012c made a gene whose values are all tied NaN only where the
tie-corrected variance happened to be zero. With tie_correct=False the
variance is never zero, so such a gene still came out as z=0, p=1 in the
non-stratified path when it was tied at a non-zero value, and in the
stratified path even when it was zero everywhere. wilcoxon_test therefore
disagreed with itself on the same gene with and without batch_column.

The kernels now detect an all-tied gene directly (_all_tied) rather than
through the variance, so the result no longer depends on tie_correct. In
the stratified kernel a gene is untested when it is tied within every
batch that contributes.

Bump SCHEMAS["de_result"] to 2: the stored values of tied genes changed,
and the reuse check compares the schema, not the crispyx version, so DE
results written by 0.1.7 would otherwise be reused with the old values.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Seven rank-test kernels were imported into de.py but never called:
_rankdata_2d_numba, _tie_correction_numba,
_compute_rank_sums_batch_numba, _merge_sorted_with_ranks_numba,
_wilcoxon_sparse_batch_numba, _wilcoxon_presorted_ctrl_numba and
_wilcoxon_all_perts_numba. Every Wilcoxon path runs
_wilcoxon_batch_perts_presorted_numba (or its stratified counterpart),
so these copies had to be kept in step by hand, and the kernel test
compared two of them with each other instead of testing the one in use.

The kernel test now checks _wilcoxon_batch_perts_presorted_numba against
scipy's ranks, with and without the tie correction. Its data has
non-zero ties and genes tied at zero, tied at a non-zero value, and
silent in the perturbation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7221270 stopped None-valued arguments from invalidating a finished
result, but ResumableRun still compared checkpoint fingerprints exactly.
Every checkpoint written by 0.1.7 lists min_pct_both: null, so after
e032ced removed that parameter, resume=True deleted every in-progress
run without a word.

The comparison now lives in _checkpoint.fingerprint_difference, which
ignores None-valued params and says what differs. Result reuse and
ResumableRun both use it. A resume that cannot use the checkpoint it
finds warns with the reason before discarding it, so lost work is never
silent, including for checkpoints from 0.1.6.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The result fingerprint was built from the raw arguments before the
aliases were resolved, so groupby= and perturbation_column=, or
reference= and control_label=, named different calls for the same
result. cx.tl.rank_genes_groups also inferred the control itself and
passed it explicitly, so it never reused a direct wilcoxon_test with
control_label=None on the same file, or was reused by one. Each call
recomputed and overwrote the other's output.

The DE functions and compute_pseudobulk_effects now fingerprint the
resolved column and control (_grouping.canonical_call_args).
rank_genes_groups leaves inferring the control to the DE function, as a
direct call does. compute_pseudobulk_effects now resolves its aliases
with the shared helper.

wilcoxon_test now checks for a reusable result right after resolving
the output path, as t_test and nb_glm_test do. Before, it first opened
the input and read every label, which made each cache hit slow on large
screens.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_finish_wilcoxon_result ran after the output had been renamed and the
checkpoint discarded. It read the result back, then added
uns["rank_genes_groups"] and the provenance stamp in place. This had two
problems:

- When the result was too large to load, the Scanpy structure was
  skipped, but the stamp still recorded scanpy_format=True. A later
  scanpy_format=True call reused a file without it.
- A run killed between the checkpoint's removal and the stamp (during
  the read-back, which can take minutes) left an unstamped output and no
  checkpoint, so even resume=True recomputed everything.

uns["rank_genes_groups"] is now built from the file's own layers, a
block of groups at a time, so it is written whatever the result's size.
_write_rank_genes_groups takes a row reader, which the in-memory writer
used by t_test and nb_glm_test shares. The standard and stratified paths
write the structure and the stamp into the partial file before it takes
the output's name, and the streaming path into its work file before the
rename. The checkpoint is discarded only after that, so the output is
complete and reusable from the moment it exists.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With freeze_control=None, nb_glm_test decides whether to fit with frozen
control statistics from the memory available, and the two modes fit
slightly different models. The checkpoint already records the mode, but
the result did not, so nothing said which model produced a file.

The result now stores it in uns["frozen_control"]. The resolved mode
stays out of the result's fingerprint on purpose: it depends on free
memory at the time of the run, so an identical call would otherwise be
recomputed whenever free memory shifted. The freeze_control docstring
now says that a None call reuses a result whichever mode it used, and
that True or False requires one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_apeglm_worker_count budgeted one batch of gene columns per worker, but
joblib's default pre_dispatch="2*n_jobs" keeps two more batches per
worker queued in the parent. Peak memory could reach about three times
the estimate, so shrink_lfc(method="full") could still exceed
memory_limit_gb. Each worker now accounts for three batches.

The count was also recomputed, with a probe of system memory, for every
perturbation. It is now computed once, for the largest comparison.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Executed in the pert environment (Python 3.11.9) from an empty
tutorial_outputs/, so every cell computes its result rather than
reloading one written by an earlier version. Apart from timings,
measured read speeds, progress-bar widget state and the last digit of
one PCA variance, every output is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization has no extra usage available to pay for this review.

If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.

Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.

@jaydu1
jaydu1 merged commit 0ff2879 into main Oct 5, 2026
3 checks passed
@jaydu1
jaydu1 deleted the library-fixes branch October 5, 2026 07:46
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