Repository navigation
0.1.8: tied genes untested in wilcoxon_test, safer result reuse and resume - #58
Merged
Merged
Conversation
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>
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
0.1.8 fixes findings from a code review of 0.1.7 and consolidates the test suite.
Behaviour changes
wilcoxon_test:NaNin every derived column, with or withouttie_correctand with or withoutbatch_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_bothis removed from the DE functions. Usemin_pct_ctrlandmin_pct_pert.Fixes
Nonenow resumes. Before, every 0.1.7 checkpoint was deleted silently aftermin_pct_bothwas removed. A resume that can't use the checkpoint it finds now warns and says why.Noneno longer invalidates a finished result.groupby/referenceandperturbation_column/control_labelnow count as the same call.cx.tl.rank_genes_groupsand a direct DE call on the same file now reuse each other's results.wilcoxon_testchecks for reuse before reading the input's labels.wilcoxon_test(scanpy_format=True)writesuns["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")honoursmemory_limit_gb, including the batches joblib queues per worker. It sizes its workers once instead of once per perturbation.nb_glm_testrecords inuns["frozen_control"]whether it fitted with frozen control statistics.Cleanup
Commits
b335574…4459878: test-suite consolidation.c0856acresolvesrank_genes_groupsaliases with the shared helper.64d873dhonoursmemory_limit_gbinshrink_lfc(method="full").7221270stopsNonearguments from invalidating cached results.e032cedremovesmin_pct_both.cf6012creports all-tied genes as untested inwilcoxon_test.4caa9abtreats all-tied genes as untested whatevertie_correctis, and bumps the DE result schema.98033c2deletes the unused Wilcoxon kernels.2df193dresumes checkpoints that differ only in arguments left atNone.effff10lets aliases andrank_genes_groupsshare DE results.5165f6awrites the Wilcoxon Scanpy structure and stamp together with the result.5aa2078records nb_glm's frozen-control mode.1c57938budgets joblib's queued batches in the apeGLM worker count.c50e245bumps the version to 0.1.8.bf4eaccre-executes the tutorials on 0.1.8.Notes for review
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. PassTrueorFalseto require one mode.Testing
pertenvironment (Python 3.11.9) and in base (Python 3.10.12).cf6012c. Those for the stratifiedtie_correct=Falsecase, checkpoint matching, aliases, the large-result Scanpy structure and the stamp ordering fail there and pass here.pertfrom an emptytutorial_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