PERF: Speed up permutation cluster tests via Numba union-find + compact graph - #13731
Conversation
|
this is a pretty large diff. Before we invest time in a review, a few questions:
tip: next time, if you name the changelog |
My bad, I meant to publish it as draft. Yes it's still in progress. Tests are all passing. AI was used to generate the code which was checked over manually to catch bugs. I'm going to try to reduce the amount of code as much as possible while still maintaining the main speedups. |
50c67c1 to
9b7edfd
Compare
Restructured this into 6 incremental commits ordered roughly by bang-for-buck, so you can review (and accept/reject) each optimization independently. Quick summary of what each one does and what it costs:
Commits 1-4 are small, pure NumPy/SciPy, and basically free in terms of complexity. They mainly help the global-adjacency path rather than the spatio-temporal one, so they don't show up much in the benchmark above. Commit 5 is where the first big measurable win kicks in (3.2×), and it's still straightforward NumPy. Commit 6 is the heavy one. It adds a Numba JIT union-find kernel with CSR precomputation and a bincount shortcut for cluster sums. It's responsible for most of the remaining speedup (3.2× → 10.3×), but it's also +215 lines and adds a Numba dependency to this code path. I have a stripped-down scipy-only version that gets ~3.8× in about +84 lines if you'd prefer to keep it simpler. Happy to swap that in or simplify further if the Numba approach feels like too much for this module. Would appreciate your guidance on where you'd want to draw that line. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update: Restructured from the initial 6 commits:
PR body updated accordingly. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The PR has been restructured extensively since that initial comment and is now ready for review. All 55 relevant cluster-level tests pass locally (both Numba and NumPy variants). |
|
Looking at it quickly, commits 1-4 don't really make much of an impact in speed while slightly increasing the code complexity. However, commits 5 and especially 6 look interesting. Speeding up the cluster permutation test by 10x is a huge improvement! Since this is AI generated, could the AI generate some more information in the docstring about the role of |
|
Thanks for taking a look! Commits 1-4: Agreed — they don't measurably speed up the spatio-temporal path and add unnecessary complexity. Dropped all four; the PR is now 7 commits focused on the two performance wins (precomputed sum-of-squares + Numba union-find) plus cleanup, bugfix, docs, and test. Union-find documentation: Added an expanded docstring to |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Can you speak to the cost to speed when If that ends up being true, then:
We should probably have a |
|
The scipy fallback is implemented. I did another benchmark and honestly it might not be worth it to have a Numba path. Per-call in the clustering function, for 200 iterations, numba is the clear winner.
But using an actual workflow,
Keep in mind that the numba path is about 100 lines of extra code, compared to the 50 line scipy path. |
|
200 extra lines for a 4x speedup in real-world use cases seems worth it to me |
|
Ohhh wait the actual use case is the second where where it's ~10% speedup. Agreed that's probably not worth it. My vote would be to remove the |
|
I agree, I've removed the code. Here's a permalink to the Numba path commit. |
|
We can look at the speed up, but could a human please look at this and not just an AI? Currently the new code does not integrate nicely with the existing code, and the naming and abstraction boundaries are not good. Hence my earlier question: what is the core idea of the actual speedup? If we know that, we can implement it much cleaner I think. |
Precompute sum-of-squares for sign-flip t-tests (s²=1 invariant) and replace Python BFS with SciPy connected_components in _get_clusters_st. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
|
Apologies for the issues. I understand it must be frustrating to deal with AI-generated PRs. I have cleaned up the code to reduce complexity. As for correctness: the two optimizations are pretty easy to understand.
The sample variance formula can be written as
|
larsoner
left a comment
There was a problem hiding this comment.
It would indeed be great to move to using connected_components! I also like the speedup for ttest_1samp, but have some ideas for a cleaner implementation
FYI at the MNE level we have decided to allow AI-facilitated PRs, and now that this is simple/small enough to be understandable, my sense is that it's worth seeing if we can get this over the finish line given the performance improvements. Okay with you @wmvanvliet ?
| if not np.all(np.equal(np.abs(signs), 1)): | ||
| raise ValueError("signs from rng must be +/- 1") |
There was a problem hiding this comment.
We shouldn't need this, as order is something that we construct
| t_obs_surr = stat_fun(X) | ||
| # Set X back to previous state (trade memory eff. for CPU use) | ||
| X *= signs |
There was a problem hiding this comment.
| t_obs_surr = stat_fun(X) | |
| # Set X back to previous state (trade memory eff. for CPU use) | |
| X *= signs | |
| try: | |
| t_obs_surr = stat_fun(X) | |
| finally: | |
| # Set X back to previous state (trade memory eff. for CPU use) | |
| X *= signs |
| # For sign-flips s²=1, so sum(X²) is constant across permutations. | ||
| # Precompute once and derive t-statistics via algebra instead of | ||
| # calling stat_fun each iteration. | ||
| use_fast_ttest = stat_fun is ttest_1samp_no_p |
There was a problem hiding this comment.
Instead of doing this, I think it would be nicer to have some pattern that uses a class. So something like
a class TTestReorder that we instantiate, and stores inv_n, neg_n, etc. We won't be able to use signs @ X but instead later still X *= signs followed by stat_fun(X) but it should be almost as fast, and require only one conditional at the beginning like:
class TTestReordered:
def __init__(self, X):
self.sum_sq = ...
self.sqrt_n_nm1 = ...
...
def __call__(self, X):
dot = np.sum(X, axis=...)
...
and here
if stat_fun is TTestReordered:
stat_fun = TTestReordered(X)
and then the code below gets simplified (no extra conditional on use_fast_ttest
| for start, end in zip(lims[:-1], lims[1:]): | ||
| keepers[row[start]] = np.sort(col[start:end]) | ||
| if max_step == 1: | ||
| return _get_clusters_st_1step(keepers, neighbors) |
There was a problem hiding this comment.
The only place _get_clusters_st_1step is used is in this function, so now that its usage is gone, it should be deleted entirely. See on main:
$ git grep _get_clusters_st_1step
mne/stats/cluster_level.py:def _get_clusters_st_1step(keepers, neighbors):
mne/stats/cluster_level.py: return _get_clusters_st_1step(keepers, neighbors)
Same is true for _get_clusters_st_multistep
| if len(active) == 0: | ||
| return [] | ||
|
|
||
| # Convert neighbor lists to CSR for vectorized expansion |
There was a problem hiding this comment.
Our neighbor lists are usually themselves derived from adjacency which itself is typically a sparse matrix or array. So maybe there is another optimization to be done where we don't pass neighbors here but instead just the adjacency itself
* upstream/main: (206 commits) Improve type checks (mne-tools#14036) Make scrollbar handlers draggable (mne-tools#14040) [pre-commit.ci] pre-commit autoupdate (mne-tools#14052) Warn when Epochs events fall outside the raw data range (mne-tools#12989) (mne-tools#14004) Ensure epochs being concatenated have compatible event ids (mne-tools#14051) Widen main content area (mne-tools#14015) MAINT: remove dead gain/bits/value_range fields from _read_header in … (mne-tools#14047) ENH: replace `_get_blocks` binary reader with mffpy Reader API (mne-tools#14043) MAINT: Update dependency specifiers (mne-tools#14048) [dependabot]: Bump the actions group with 2 updates (mne-tools#14049) Simplify doc building with more refleak (mne-tools#14045) ENH: add overlay Brain GUI (mne-tools#14031) ENH: support multiple simultaneous overlays in Brain.add_data (mne-tools#13995) Add option to show a zero line in browser (mne-tools#14018) FIX: pass cmap name string not tuple to interactive topomap slider kwargs (mne-tools#14039) Doc/add ai policy pointer (mne-tools#14037) Allow subclasses of FigureClass to be passed to plot_raw/plot_epochs (mne-tools#13979) MAINT: Replace manual PNS binary block reader in `_read_segment_file` with `mffpy` (mne-tools#14030) Fix transition bandwidth reported in 'filter too short' error (mne-tools#11406) (mne-tools#14005) ENH: Show the current time as a vertical line in plot_evoked_topo (mne-tools#14032) ...
|
Okay I've gone through and made my suggested edits. I also wrote some unit tests (drafted with the help of Claude Opus 4.8) that caught a real bug in Marking for merge-when-green, thanks for iterating on this @sharifhsn ! |
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Reference issue
Related: #5439, #7784, #8095, #12609
What does this implement/fix?
Speeds up
spatio_temporal_cluster_1samp_test(and the otherpermutation_cluster_*functions) by ~5-10x on realistic data. The PR is split into 7 incremental commits. Maintainers can accept or reject each layer independently.Commit 1 — Precompute sum-of-squares for sign-flip t-test (+29/−9 lines, 3.2x)
For the default
ttest_1samp_no_p, s²=1 meanssum(X²)is constant across permutations. Each permutation becomes a singlesigns @ Xdot product instead of callingstat_fun. Also skipsbuffer_sizeverification for built-in stat functions.Commit 2 — Numba union-find for spatio-temporal CCL (+226/−11 lines, 10.3x cumulative)
JIT-compiled union-find kernel (
_st_fused_ccl) with path compression and union-by-rank, replacing the Python BFS in_get_clusters_st. Bundles tightly-coupled pieces: pre-computed CSR adjacency arrays,_sums_onlyflag to skip cluster list construction (usesnp.bincountinstead), and_csr_dataparameter threading. These are bundled because_sums_onlyonly fires insideif has_numba:and CSR data is only consumed by the Numba kernel.Commit 3 — Extract _union helper + simplify (+36/−72 lines)
Extract duplicated find+union logic into
_union()withinline="always", simplify_sum_cluster_data, trim docstrings/comments to match codebase style.Commit 4 — Fix step-down reshape (+1/−1 lines)
Pre-existing bug:
adjacency is None and adjacency is not Falsewas equivalent to justadjacency is None, missing theadjacency is Falsecase wherestep_down_includestill needs reshaping.Commit 5 — Changelog entries
Commit 6 — Test fixture (+1 line)
Patch
has_numbainnumba_conditionalfixture so the "NumPy" test variant actually exercises the Python BFS fallback path for spatio-temporal clustering.Commit 7 — Docstring (+18/−1 lines)
Expand
_st_fused_ccldocstring with algorithm description, complexity analysis, and Wikipedia reference, per reviewer request.Commits 3-7 are cleanup, bugfix, docs, and tests — they don't affect performance. All optimizations fall back to the original code paths when Numba is not installed. No public API changes.
Benchmarks
Per-commit cumulative speedup (local, Apple M-series,
spatio_temporal_cluster_1samp_test, ico-5, 15 subjects x 15 timepoints x 20,484 vertices, threshold=3.0, 512 permutations, median of 3 runs):AWS HPC end-to-end (AMD EPYC 7R13, same data dimensions):
Per-permutation cost: 15.8 ms → 3.1 ms (5.2x). Projected 10,000 permutations: 31 s vs 159 s.
Reproduce benchmarks locally
Additional information
threshold=dict(...)) correctly falls back to the original code path