Skip to content

[auto-optimize]: Speed up modisco motifs with byte-identical output - #138

Merged
chang-m-yun merged 40 commits into
masterfrom
auto-optimize/2026-10-12-iter-17
Oct 16, 2026
Merged

chang-m-yun merged 40 commits into
masterfrom
auto-optimize/2026-10-12-iter-17

Conversation

@jmschrei

Copy link
Copy Markdown
Collaborator

This optimization was found with auto-optimize, a tool developed by the Programmable Genomics Laboratory at UMass Chan Medical School: the modisco motifs pipeline is parallelized across processes, its numba kernels are compiled in background processes and rewritten, and its clustering, merging, polishing and start-up stages are reworked, cutting a 50,000-seqlet benchmark on real attribution scores from 1745 s to 119.1 s with output byte-identical to upstream's under leidenalg 0.11.0, which pyproject.toml now pins.

Changes, in order

  • A1 (MISC): brings in the result of two earlier searches (5,000 and 20,000 seqlets), 30 commits replayed without conflict: Leiden seeds, per-pattern subclustering, per-cluster pattern building, the collapser's pair statistics and the negative metacluster run in forked processes; numba kernels compile in background child processes; the cosine kNN, the Jaccard kernels, density adaptation, the correlation filter and seqlet extraction are rewritten or vectorized; and pyproject.toml pins leidenalg==0.11.0. It also removes the tests that failed only because of those changes' signatures and APIs: the conftest.py warm-up call to _sparse_mm_dot, the three _sparse_mm_dot tests (the pipeline now calls _sparse_knn, which returns the same values on all their inputs) and test__jaccard_raises.
  • B2 (OPTIMIZATION): the cosine kNN kernel selects each row's top k with a histogram over the values' bit patterns, keeping the heap scan for rows with a negative value, a NaN or -0.0, and runs as a serial @njit(nogil=True) kernel over blocks of 16 rows on a Python thread pool instead of a prange kernel.
  • B5 (OPTIMIZATION, re-merges B3): when the largest pattern's share is expected to finish last, its affinity matrix is computed with the whole thread budget before anything is forked, in the refine stage and in the final subclustering; in the refine stage the other patterns are refined in one forked helper, which also computes the final collapser's first-iteration pair statistics among them.
  • B10 (OPTIMIZATION, re-merges B8, B9): the cosine kNN kernel reads one merged forward/reverse posting list per column and prefetches columns ahead, and the gapped k-mer rows use unsigned casts; the Jaccard kernel prefetches neighbours and uses 64-byte-aligned accumulators; merging and polishing work on seqlet coordinates, create the seqlets once and reuse interior sums.
  • B13 (OPTIMIZATION, re-merges B11): the two npz inputs are read and inflated on two threads while the main thread imports scikit-learn, igraph and leidenalg; the indices of seqlet extraction's subsample are computed in a forked child; and the negative half of the Laplacian null's percentiles runs on a thread.
  • B17 (MISC): adds numpy's BSD-3-Clause notice above modiscolite.util._pairwise_sum, a numba translation of numpy's pairwise summation; comments only.

Translated code

modiscolite.util._pairwise_sum is a translation into numba of pairwise_sum in numpy's numpy/_core/src/umath/loops_utils.h.src (numpy 2.0.1), so that sums over rows of the density-adapted affinity matrix are added in numpy's order. It is under numpy's BSD-3-Clause license, whose notice is reproduced above the function. No other code is copied or translated from another project; routines that reproduce scikit-learn's ROC AUC and scipy's sparse operations are written from their documented behaviour.

Where this was measured

This branch is the measured code, unchanged: master at 5b37ef7 plus the 39 commits of the search's path. The same
change is also proposed to jmschrei/tfmodisco-lite as a port onto its main
(jmschrei/tfmodisco-lite#78), where modiscolite/ and tests/ are byte-identical to this
branch.

Checks outside the search

  • Clean room. The measured branch merged into a fresh clone of kundajelab/tfmodisco and installed the ordinary way,
    then timed against upstream installed the same way, three runs each on the 50,000-seqlet benchmark at 16 threads:
    upstream 1726 s (1723, 1726, 1731), this branch 118.7 s (123.4, 118.5, 118.7). Outputs identical to upstream's on
    the benchmark and on 20,000-seqlet, 5,000-seqlet and 480-sequence inputs in every run, and unit tests passing in both
    environments. The verdict of the check is differs: upstream ran 19 s (1.1%) faster than the search measured it
    (1745 s, on another machine with numpy 2.0.1 and numba 0.67.0), so the gain differs from the search's by 17.9 s,
    more than the 5.8 s noise band; this branch's time matched the search's (119.1 s).

  • CI, run locally on this branch's head: tests.yml on Python 3.9, 3.10, 3.11, 3.12 and 3.13 (numpy 2.0.2 to 2.5.4,
    numba 0.60.0 to 0.68.0): 1,450 passed and 28 skipped on each; publish.yml's build: the sdist and the wheel build and
    pass twine check. The suite has 18 tests fewer than main's 1,468: the ones A1 removes.

  • Every command and flag. Both versions, installed, ran 4,972 jobs: every subcommand (motifs, report,
    report-simple, meme, seqlet-bed, seqlet-fasta, convert, convert-backward) with every flag one at a time,
    invalid values included; motifs at default options on every attribution file (60 models; gradients, DeepLIFT/SHAP,
    integrated gradients, ISM) of a test-data collection's 10-, 1,000- and 10,000-locus tiers, and the twelve downstream
    commands on each output; noise and constant attribution tracks; 40 malformed inputs (NaN, inf, wrong shapes, dtypes,
    layouts, keys, truncated and empty files, both legacy h5 layouts); 49 malformed MEME files; and -n 1000, 10000 and
    100000 on 44 whole-experiment attribution sets of 3,502 to 242,924 peaks. h5 outputs were compared by content,
    every other output (HTML, logos, MEME, BED, FASTA), stdout and exit codes by bytes, and failures by their error line.
    Every job matched, or failed the same way in both versions, except 19:

    • 3 outputs number two equal-size subpatterns of one pattern the other way round (for example subpattern_1 and
      subpattern_2 exchanged, with identical contents). Two Leiden seeds find the same partition, numbered differently;
      their modularities tie exactly in upstream's arithmetic, and this branch's Leiden graph, which adds each
      undirected edge once with weight w + w instead of twice, breaks the tie by one unit in the last place.
    • 8 runs with -n 1 fail in both versions with exit 1 and a ValueError, from different lines: upstream's
      "need at least one array to concatenate" while building the Leiden graph, this branch's "max() iterable argument
      is empty" one step later.
    • 8 runs with -g -1 (a negative flank) fail in both versions with exit 1 and a ValueError, at different stages:
      upstream's "negative dimensions are not allowed" in a later polish, this branch's broadcast error in its first,
      coordinate-based polish.

    In the same run (up to 20 jobs side by side at 8 threads each, so a single job's time includes contention), the
    median time of motifs at -n 100000 across the 44 whole-experiment sets was 3,165 s for upstream and 197 s for this
    branch; on the four largest sets upstream took 3.2 to 3.4 hours and this branch 9 to 10 minutes.

What the command-line evaluation covered

Both versions were installed in their own environments and each job ran every command once per version, in the same
working directory, on js031 (2 x AMD EPYC 9575F). Counts are from the job results (coverage_stats.py).

Inputs. Attributions from 60 models and model heads in 17 families (BPNet, ChromBPNet, Cherimoya, ProCapNet, Sei,
Beluga, Malinois, DeepSTARR, LegNet, ENIGMA, Puffin, Puffin-D, CLIPNET, seq2PRINT, Decima, EPInformer, gReLU), by four
attribution methods: gradients (410 motifs jobs), DeepLIFT/SHAP (507), integrated gradients (372) and in silico
saturation mutagenesis (200 over the whole window, 331 over its central 50 to 1,000 bp). 410 attribution sets: 151 of
10 loci, 151 of 1,000, 64 of 10,000, and 44 whole experiments (3,502 to 242,924 peaks) each run at -n 1,000,
10,000 and 100,000. 31 locus lists over 13 biosamples (cell lines, tissues, a fly embryo and yeast) and synthetic
sequence, on hg38, hg19, dm6 and sacCer3.

Controls.

control jobs what it checks
noise and constant attribution tracks (Gaussian, exponential and uniform at five scales, and three constants, on three sequence sets) 162 motifs jobs, with their downstream commands behaviour where no motif exists
sequences with motifs planted at known positions (uniform and dinucleotide-shuffled backgrounds) 750 motifs jobs a known answer
malformed inputs (NaN, inf, float64, float16, int32, N bases, soft probabilities, bool and float32 one-hot, .npy, both legacy h5 layouts, transposed, 2-D, mismatched counts, lengths and channels, wrong or extra npz keys, 0 and 1 loci, odd width, Fortran order, truncated, empty and text files, an h5 with neither layout) 40 cases, their 2 unmodified parents, and 4 legacy files given with -s/-a too that both refuse, or both load, the same way
malformed MEME files 49, each through report, report -l and report-simple motif database parsing
command-line errors and every --help 27 click's handling
each version run twice on the same input 16 determinism

Comparisons. 4,972 jobs, 19,234 command pairs (38,468 processes), 439,585 output files compared.

subcommand pairs both succeeded both failed, same exit code and error exit code or error line differs
motifs 2,806 1,874 (1,871 outputs identical; 3 with two subpatterns numbered the other way round) 916 16
meme 6,589 6,551, outputs identical 38 0
report 2,889 2,753, outputs identical 136 0
seqlet-fasta 1,456 1,362, outputs identical 94 0
seqlet-bed 1,445 1,388, outputs identical 57 0
report-simple 1,433 1,407, outputs identical 26 0
convert-backward 1,312 1,299, outputs identical 13 0
convert 1,300 1,287, outputs identical 13 0

Of the 1,873 motifs runs on data that both versions completed, 1,121 output at least one pattern: 13,010 patterns
with 3,480,920 seqlets, identical apart from the three subpattern numberings. The other 752 hold none (almost all from the
10-locus sets), so they compare empty results.

Not covered. 750 downstream jobs did not run, because their motifs run failed in both versions; 916 motifs
pairs show only that both fail the same way. Flags were swept one at a time on four representative sets per size,
with four combinations. Everything ran on Linux with leidenalg 0.11.0, the version pyproject.toml pins; other numpy
and numba versions were exercised only by the unit tests (Python 3.9 to 3.13 above). The Leiden tie behind the three
numbering differences comes from summation order, and on other inputs it could in principle pick a seed with a
different partition; that was not observed.

Usage benchmarks

Machine: AMD EPYC 9575F (2 x 64 cores, 256 hardware threads), Ubuntu 24.04.5 (Linux 7.0.0), Python 3.13.5, numpy 2.5.4,
numba 0.68.0, scipy 1.18.1, scikit-learn 1.9.1, leidenalg 0.11.0, igraph 1.0.0; base is master at 5b37ef7 and
winner this branch (B17), each installed with uv pip install . pytest leidenalg==0.11.0 from a git archive export
into its own environment. Synthetic
inputs: none were generated for this sweep; the inputs are real DeepLIFT/SHAP attributions from public ENCODE-trained
models in a test-data collection, the search's own Acinar data, and one negative control whose attributions are
Gaussian noise (sd 1, drawn with a seed fixed by the collection). Each row is one modisco motifs run per version,
one configuration at a time on an otherwise idle machine, each a fresh process with an empty numba cache (start-up and
compilation included), NUMBA_NUM_THREADS and the BLAS thread counts set to the row's threads. Peak is the whole
process tree's memory.peak from its own cgroup (forked workers included, page cache included). Gate: the winner's
h5 equals upstream's in every group, dataset and attribute (dtype, shape and bytes).

Input class Input Threads Base (s) Winner (s) Speedup Base peak (MB) Winner peak (MB) Gate
tiny 10 K562 ATAC peaks (ChromBPNet), -n 2000 1 2.1 1.4 1.6x 200 180 pass
tiny 10 K562 ATAC peaks (ChromBPNet), -n 2000 2 2.1 1.2 1.8x 200 251 pass
tiny 10 K562 ATAC peaks (ChromBPNet), -n 2000 4 2.1 1.2 1.8x 202 379 pass
tiny 10 K562 ATAC peaks (ChromBPNet), -n 2000 8 2.1 1.2 1.7x 204 503 pass
tiny 10 K562 ATAC peaks (ChromBPNet), -n 2000 16 2.2 1.2 1.9x 207 502 pass
small 1,000 K562 ATAC peaks (ChromBPNet), -n 2000 1 35.6 11.8 3.0x 507 378 pass
small 1,000 K562 ATAC peaks (ChromBPNet), -n 2000 2 25.5 7.6 3.4x 511 435 pass
small 1,000 K562 ATAC peaks (ChromBPNet), -n 2000 4 21.1 5.2 4.1x 524 446 pass
small 1,000 K562 ATAC peaks (ChromBPNet), -n 2000 8 18.8 3.5 5.4x 540 566 pass
small 1,000 K562 ATAC peaks (ChromBPNet), -n 2000 16 17.6 3.4 5.1x 588 569 pass
degenerate Gaussian noise (sd 1) as attributions, 1,000 ATAC peaks, -n 2000 1 70.9 16.7 4.3x 568 370 pass
degenerate Gaussian noise (sd 1) as attributions, 1,000 ATAC peaks, -n 2000 2 48.3 10.3 4.7x 566 434 pass
degenerate Gaussian noise (sd 1) as attributions, 1,000 ATAC peaks, -n 2000 4 36.4 9.4 3.9x 573 459 pass
degenerate Gaussian noise (sd 1) as attributions, 1,000 ATAC peaks, -n 2000 8 30.7 5.7 5.4x 593 583 pass
degenerate Gaussian noise (sd 1) as attributions, 1,000 ATAC peaks, -n 2000 16 27.8 5.3 5.3x 630 668 pass
irregular Malinois K562, 200 bp windows, 10,000 DNase peaks, -n 20000 -w 200 1 256.5 30.7 8.4x 963 605 pass
irregular Malinois K562, 200 bp windows, 10,000 DNase peaks, -n 20000 -w 200 2 159.8 17.8 9.0x 967 699 pass
irregular Malinois K562, 200 bp windows, 10,000 DNase peaks, -n 20000 -w 200 4 109.1 12.0 9.1x 1,031 626 pass
irregular Malinois K562, 200 bp windows, 10,000 DNase peaks, -n 20000 -w 200 8 83.4 9.6 8.7x 1,060 749 pass
irregular Malinois K562, 200 bp windows, 10,000 DNase peaks, -n 20000 -w 200 16 70.4 9.0 7.8x 1,081 758 pass
medium BPNet CTCF, 10,000 ChIP-seq peaks, -n 20000 1 3,283.8 206.1 15.9x 3,456 1,432 pass
medium BPNet CTCF, 10,000 ChIP-seq peaks, -n 20000 2 1,925.1 112.4 17.1x 3,038 1,660 pass
medium BPNet CTCF, 10,000 ChIP-seq peaks, -n 20000 4 1,208.0 92.6 13.0x 2,919 1,668 pass
medium BPNet CTCF, 10,000 ChIP-seq peaks, -n 20000 8 804.2 83.6 9.6x 3,059 1,737 pass
medium BPNet CTCF, 10,000 ChIP-seq peaks, -n 20000 16 589.6 81.8 7.2x 3,084 1,709 pass
real, the search's data 18,000 Acinar regions (Cherimoya), -n 20000 1 2,581.7 153.1 16.9x 2,636 1,333 pass
real, the search's data 18,000 Acinar regions (Cherimoya), -n 20000 2 1,528.1 85.3 17.9x 2,645 1,490 pass
real, the search's data 18,000 Acinar regions (Cherimoya), -n 20000 4 941.6 57.4 16.4x 2,652 1,629 pass
real, the search's data 18,000 Acinar regions (Cherimoya), -n 20000 8 662.3 47.8 13.9x 2,664 1,659 pass
real, the search's data 18,000 Acinar regions (Cherimoya), -n 20000 16 514.6 43.6 11.8x 2,686 1,772 pass
very big 242,924 GM12878 ATAC peaks (Cherimoya, float16), -n 10000 1 1,294.8 125.1 10.3x 23,255 9,374 pass
very big 242,924 GM12878 ATAC peaks (Cherimoya, float16), -n 10000 2 835.0 74.2 11.3x 17,078 9,504 pass
very big 242,924 GM12878 ATAC peaks (Cherimoya, float16), -n 10000 4 602.7 61.5 9.8x 17,078 9,497 pass
very big 242,924 GM12878 ATAC peaks (Cherimoya, float16), -n 10000 8 480.0 50.0 9.6x 17,079 9,481 pass
very big 242,924 GM12878 ATAC peaks (Cherimoya, float16), -n 10000 16 418.2 47.2 8.9x 17,083 9,481 pass

The outputs passed the gate in every configuration.

Path from the baseline

iteration category change status runs seconds (s) Δ vs parent h5_mismatches_vs_base h5_mismatches_vs_base_20k h5_mismatches_vs_base_5k h5_mismatches_vs_base_tiny peak_rss_mb peak_cgroup_mb seconds_20k peak_rss_mb_20k peak_cgroup_mb_20k seconds_5k peak_rss_mb_5k peak_cgroup_mb_5k seconds_tiny peak_rss_mb_tiny peak_cgroup_mb_tiny n_pos_patterns n_neg_patterns n_seqlets_pos_patterns n_seqlets_neg_patterns unit tests
A0 ROOT baseline ok 3 (confirmed) 1745 0 0 0 0 6251 6583 523.4 2390 2492 87.67 991.9 995.5 10.63 446 419 34 11 2.696e+04 814 pass
A1 MISC 20k:B28 replayed onto master 5b37ef7 (30 path commits), plus removing tests that fail only by signature/API change ok 3 (confirmed) 125.4 -1.62e+03 0 0 0 0 2636 3570 45.05 1320 1753 11.74 588 799.7 2.682 309.9 573.5 34 11 2.696e+04 814 pass
B2 OPTIMIZATION cosine kNN stage at 50k: where the 5.1 s goes, and a faster exact path ok 3 (confirmed) 123.8 -1.58 0 0 0 0 2642 3467 44.57 1326 1724 11.47 595.4 757.9 2.675 307.3 565.1 34 11 2.696e+04 814 pass
B5 OPTIMIZATION re-merge: B3 (largest pattern's affinity first, other patterns in one helper) onto B2 ok 3 (confirmed) 123.4 -0.368 0 0 0 0 2647 3469 44.52 1334 1704 11.65 587.3 743.3 2.662 309.3 565.7 34 11 2.696e+04 814 pass
B10 OPTIMIZATION batched re-merge: B9 (B6 + B7: kNN and Jaccard kernels) and B8 (seqlets on coordinates in merging and polishing) onto B5 ok 3 (confirmed) 122.7 -0.724 0 0 0 0 2645 3475 43.84 1337 1765 11.62 589.1 738.9 2.647 309.1 570.2 34 11 2.696e+04 814 pass
B13 OPTIMIZATION re-merge: B11 (threaded npz inflation beside the imports, forked subsample, threaded null percentiles) onto B10 ok 3 (confirmed) 119.4 -3.27 0 0 0 0 2647 3482 43.32 1351 1753 11.44 592.8 764.4 2.636 309.6 577 34 11 2.696e+04 814 pass
B17 MISC numpy's BSD-3-Clause notice above _pairwise_sum, a translation of numpy's pairwise_sum ok 3 (confirmed) 119.1 -0.311 0 0 0 0 2646 3493 42.9 1361 1761 11.38 603.1 765.6 2.679 308.9 580.2 34 11 2.696e+04 814 pass

Every value is the median over that iteration's runs (one seed per run); Δ is against the row above. Gates: h5_mismatches_vs_base == 0, h5_mismatches_vs_base_20k == 0, h5_mismatches_vs_base_5k == 0, h5_mismatches_vs_base_tiny == 0.

🤖 Generated with Claude Code

Jacob Schreiber and others added 30 commits October 4, 2026 19:10
git diff autoopt/1 autoopt/4 | git apply -3; clean in affinitymat.py and
gapped_kmer.py.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
git diff autoopt/2 autoopt/6 | git apply -3; affinitymat.py clean,
precompile.py conflicted in the kernel list. Resolved: _fwd_rev_best takes
_jaccard_best's child, and _jaccard_best is queued behind B3's density
kernels (B6 queued it behind binary_search_perplexity, which B3 folded into
_perplexity_betas). Five groups, as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
git diff autoopt/8 autoopt/14 | git apply -3; clean in affinitymat.py,
core.py and precompile.py. B15 (B12 + B9) does not touch these three
beyond B12, so each is byte-identical to B18's (B14 on B12).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…o B15 + B14

git diff autoopt/12 autoopt/16 | git apply -3; tfmodisco.py clean. The
applied +/- lines are B16's own, and the file differs from B16's by exactly
B9's lines (B15), both compared as line multisets.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…15 + B14 + B16

git diff autoopt/12 autoopt/17 | git apply -3; clean in cli.py, cluster.py,
extract_seqlets.py and tfmodisco.py. Then cluster._leiden is put back as in
B15: find_partition gets weights=weights, not weights.tolist(), and the
docstring lines that explained the list are dropped. B17's README measured
that hunk at ~200 MB more in each main Leiden clustering for 0.01-0.02 s.
The rest of B11's cluster.py change is kept; cli.py and extract_seqlets.py
are byte-identical to B17's.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
git diff autoopt/26 autoopt/27 | git apply -3; clean in affinitymat.py,
aggregator.py, core.py, precompile.py and util.py. The tree is identical to
autoopt/27's.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
git diff autoopt/21 autoopt/25 | git apply -3; clean in affinitymat.py and
precompile.py. The applied +/- lines are B25's own (line multisets), and
every name B25 added or changed in affinitymat.py is byte-identical to
B25's; jaccard_from_seqlets and _fwd_rev_best are the same on B21 and B26.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Jacob Schreiber and others added 9 commits October 4, 2026 19:14
…anges

PR 136's tests, merged into master, call private functions B28 changed:

- conftest.py's session warm-up and test_sparse_mm_dot (15 cases),
  test_sparse_mm_dot_max and test_sparse_mm_dot_ties call
  affinitymat._sparse_mm_dot with its old 7 arguments; it now takes 10
  and the pipeline calls _sparse_knn. On all 17 of those inputs,
  _sparse_knn returns similarities and neighbours byte-identical to
  upstream's _sparse_mm_dot.
- test__jaccard_raises expects _jaccard to reject float64 and int64
  inputs; B28 dropped its eager numba signature, so they compile instead.
  The test checks no value.

Removed under the user's rule: tests that fail only because of a change
in signature or API are removed; a test that returns an incorrect value
is reported instead (none did). The _sparse_mm_dot import, used only by
these tests, goes with them.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nd B6 (Jaccard prefetch, aligned accumulators) onto B5

(cherry picked from commit 28c0d64ea2bd45b2b6260fadfccf37cdd4d3c816)
…polishing

merge_in_seqlets_filledges holds the parent, once it has been expanded, as a
"shell": a SeqletSet whose seqlets are coordinate tuples, with the counts and
running sums the SeqletSet would have. Its expansions compute the new
coordinates and keep them as SeqletSet would (first of each string), and add
up only the new flanks when every seqlet was kept and the sums are the
kernel's (the interior positions take the same additions as before). Its
seqlets are made once, when it is returned. polish_pattern takes its two
trims and its first expansion the same way and makes only the last
expansion's seqlets. Anything these paths do not cover (a kernel that does
not apply, a NaN, a window that does not fit) runs the code as it was.

Also: TrackSet._coordinate_sums (the seqlet sums kernel from coordinates,
which _add_seqlet_sums now calls), SeqletSet._support_window (trim_to_support's
indices), Seqlet.string as an f-string of str() of each coordinate, and no
super().__init__() call in Seqlet.__init__.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 444ef0d4ecfe6888619bbd0b8331bb9984397ba4)
…e kernel

SeqletSet(seqlets, track_set=...) at the places that build patterns from
seqlets the track set made: each subpattern (_set_subclusters, through
compute_subpatterns' new optional track_set), the patterns a forked child
built (_patterns_from_clusters' unpack_share) and the negative metacluster's
patterns (_unpack_patterns). TrackSet._add_seqlet_sums then adds the sums up
from the coordinates in seqlet order, as B23 did for the expansions.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
(cherry picked from commit f023fcee683bb42a92d8b3f1ad123ab35a822d49)
@chang-m-yun

Copy link
Copy Markdown
Contributor

Summary: Three (potential) minor issues?

  1. P1: Graph optimization changes clustering. A normalized 13-node graph produces 4 communities on base and 5 on the PR. Restoring the original graph construction restores the original partition.
  2. P2: Runtime thread limits are ignored. With numba.set_num_threads(1), the scheduler still uses a budget of 4 and starts three compilation children.
  3. P2: Python 3.8 installation breaks. The Leiden pin requires an igraph version supporting Python ≥3.9, while package metadata still advertises ≥3.7. The base dependency resolves on 3.8; the PR dependency fails.

[Below: Codex review 🤖] I compared PR head 7e4377e against base 5b37ef7 on Linux/Python 3.10.18. All 1,450 runnable tests passed, with 28 existing bug skips, and the real-input cases I tested produced matching outputs. I found three issues below.

The first two examples use the PR checkout with these dependency versions:

python -m pip install .
numpy==2.0.1 numba==0.62.1 scipy==1.15.3
scikit-learn==1.7.2 leidenalg==0.11.0 igraph==1.0.0
memelite==0.2.0

  1. Collapsing parallel edges can change the actual Leiden partition

The PR description already acknowledges this possibility. The example below demonstrates it: a normalized, symmetric graph with finite positive weights and diagonal entries produces four communities using upstream’s graph construction and five using the PR’s construction.

Both calls use the same PR clustering code, dependency versions, and seeds; only the graph builder changes.

NUMBA_NUM_THREADS=4 python - <<'PY'
from unittest.mock import patch
import igraph as ig
import numpy as np
import scipy.sparse as sp
from modiscolite import cluster

rng = np.random.RandomState(5)
x = np.triu(rng.uniform(0, 1, (13, 13)), 1)
x[x < 0.7] = 0
x = np.where(
    x == 0, 0,
    1 + rng.randint(-4, 5, x.shape) * np.finfo(float).eps,
)
x = x + x.T
np.fill_diagonal(x, 1)
affinity = sp.csr_matrix(x / x.sum())

def upstream_graph(a):
    sources = np.repeat(np.arange(a.shape[0]), np.diff(a.indptr))
    g = ig.Graph()
    g.add_vertices(a.shape[0])
    g.add_edges(zip(sources, a.indices))
    return g, a.data

with patch.object(cluster, "_graph", upstream_graph):
    old = cluster.LeidenCluster(affinity, n_seeds=2)
new = cluster.LeidenCluster(affinity, n_seeds=2)

print("Upstream community sizes:", np.bincount(old).tolist())
print("PR community sizes:", np.bincount(new).tolist())
print(
    "Same partition:",
    np.array_equal(
        old[:, None] == old[None, :],
        new[:, None] == new[None, :],
    ),
)
PY

Output:

Upstream community sizes: [5, 4, 2, 2]
PR community sizes: [4, 3, 2, 2, 2]
Same partition: False

The pairwise membership comparison confirms changed communities beyond label renumbering. Preserving upstream’s ordered parallel-edge representation would avoid this compatibility change.

  1. The process scheduler ignores runtime Numba thread limits

forked.budget() returns numba.config.NUMBA_NUM_THREADS, the configured maximum, even after the caller reduces the effective limit with numba.set_num_threads(). On Linux with the fork-safe workqueue backend, limiting Numba to one thread still starts three compilation processes:

NUMBA_NUM_THREADS=4 NUMBA_THREADING_LAYER=workqueue python - <<'PY'
import numba
from modiscolite import forked, precompile

numba.set_num_threads(1)
print("Effective Numba threads:", numba.get_num_threads())
print("Scheduler budget:", forked.budget())

try:
    precompile.start()
    print("Compilation children started:", len(precompile._children))
finally:
    precompile._stop()
PY

Output:

Effective Numba threads: 1
Scheduler budget: 4
Compilation children started: 3

This budget also controls Leiden workers and other process task plans. The effective budget should be captured at run entry, before temporary stage-specific reductions, and respected throughout the run.

  1. The dependency pin prevents Python 3.8 installation

The new regular dependency leidenalg==0.11.0 requires igraph>=1.0.0,<2.0, whose available releases require Python ≥3.9. The package still declares requires-python = ">=3.7".

The following commands ask pip to resolve Python 3.8 wheels from a newer interpreter; --dry-run leaves the environment unchanged:

for requirement in 'leidenalg==0.11.0' 'leidenalg>=0.8.10'; do
  python -m pip install \
    --dry-run --ignore-installed --only-binary=:all: \
    --python-version 3.8 \
    --target /tmp/tfmodisco-py38-check \
    "$requirement"
done

The PR’s pinned requirement fails:

ERROR: No matching distribution found for igraph<2.0,>=1.0.0

The previous requirement resolves successfully:

Would install igraph-0.11.8 leidenalg-0.10.2 texttable-1.7.1

If dropping Python 3.8 is intentional, please update requires-python and document the change. Otherwise, the dependency selection needs to remain compatible with Python 3.8.

@chang-m-yun

Copy link
Copy Markdown
Contributor

Suggested the following:

The reason for pinning is sound, but it doesn’t explain issue 1. PR #136 reports different clustering with Leiden 0.11.0 and 0.12.0. Our counterexample uses Leiden 0.11.0 and igraph 1.0.0 for both calls in the same process, with identical inputs and seeds. Changing only graph construction changes four communities into five. PR #136 (#136)

I suggest these fixes:

  1. Preserve upstream’s graph representation. Keep every CSR entry as an undirected edge, including both parallel edges for symmetric off-diagonal entries. Remove the upper-triangle filtering and weight
    doubling. Keep the streaming edge construction and parallel Leiden runs.

    I verified that streaming the original edges in their original order restores the exact upstream membership. Keep the Leiden pin as well, and add this counterexample as a regression test. This addresses
    compatibility with the claimed identical output.

  2. Honor the caller’s effective thread budget. The minimum correction to forked.budget() is:

   def budget():
      if _budget is None:
            return numba.get_num_threads()
      return _budget

NUMBA_NUM_THREADS is the maximum; get_num_threads() reflects the runtime setting. Numba documentation (https://numba.readthedocs.io/en/stable/user/threading-layer.html#setting-the-number-of-threads)

For scheduling that needs a stable total budget, capture that effective value at run entry before precompilation changes thread masks, then divide it among compiler children and execution workers. Restore
the caller’s settings afterward. I verified that using the existing budget_of() scope with a captured limit of one starts zero compiler children and preserves the caller’s limit.

  1. Make the supported Python versions explicit. The simplest solution, preserving the author’s Leiden pin, is:
   requires-python = ">=3.9"

Document the compatibility change and retain CI coverage for supported versions.

If Python 3.8 must remain supported, use interpreter-specific Leiden dependencies:

   "leidenalg==0.10.2; python_version < '3.9'",
   "leidenalg==0.11.0; python_version >= '3.9'",

Update the test extra too, and replace the new os.waitstatus_to_exitcode() calls—it was introduced in Python 3.9. For the current zero-exit checks, this compatible predicate suffices:

   os.WIFEXITED(status) and os.WEXITSTATUS(status) == 0

Then test installation and the fork paths on Python 3.8. Python 3.9 release notes (https://docs.python.org/3.9/whatsnew/3.9.html#os)

@chang-m-yun
chang-m-yun merged commit 28154f1 into master Oct 16, 2026
5 checks passed
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.

2 participants