Skip to content

⚡ Bolt: optimize runica.py preprocessing and training loops#270

Open
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-runica-16203114544017163981
Open

⚡ Bolt: optimize runica.py preprocessing and training loops#270
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-runica-16203114544017163981

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

💡 What: Optimized the `runica` implementation in `src/eegprep/functions/sigprocfunc/runica.py`.

  • Vectorized the row mean subtraction step using NumPy broadcasting.
  • Optimized all four ICA training loops by moving `np.errstate` context managers outside the loops.
  • Replaced `_matmul(bias, onesrow)` with NumPy broadcasting (`+ bias`).
  • Replaced internal `_matmul` helper calls with the native `@` operator to eliminate function call and context management overhead.

🎯 Why: The previous implementation had significant overhead from tight Python loops and redundant matrix operations that could be handled efficiently by NumPy broadcasting and native operators.

📊 Impact: Reduces Standard ICA execution time by approximately 10% (from ~0.86s to ~0.78s for 10k frames/20 steps in local benchmarks). Improves readability of core numerical logic.

🔬 Measurement: Verified correctness using `uv run pytest tests/test_runica.py` (21 passed) and documented performance gains using a temporary benchmark script.


PR created automatically by Jules for task 16203114544017163981 started by @suraj-ranganath

- Vectorized row mean subtraction using broadcasting.
- Replaced redundant matrix-based bias addition with broadcasting.
- Moved np.errstate outside of training loops and switched to native @ operator to reduce overhead.
- Improved readability of core natural gradient formulas.

Co-authored-by: suraj-ranganath <14310165+suraj-ranganath@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @suraj-ranganath's task in 2m 42s —— View job


Code review

  • Overall assessment: Needs one small change (dead variable will fail CI lint). Numerics are correct.
  • Highest-risk area: Warning-suppression scope + leftover unused variable.
  • Merge recommendation: Needs changes (trivial).

I reviewed all four training loops, the mean-subtraction vectorization, and the _matmul@ replacements against origin/develop. The numerical logic is equivalent everywhere I checked:

  • data -= rowmeans[:, np.newaxis] is safe — data is forced to float64 at runica.py:230, so the in-place subtract can't raise a casting error or truncate.
  • + bias correctly reproduces _matmul(bias, onesrow)bias is (ncomps, 1) and broadcasts across the (ncomps, block) result exactly like bias @ onesrow.
  • signs @ y @ u.T, (1 - 2*y) @ u.T, and delta @ delta preserve the original left-to-right associativity and dot-product semantics of the nested _matmul calls.
  • _matmul is still used in the PCA/sphering/post-processing paths (runica.py:509, 525, 549, 1308+), so it is not dead.

Important

  • Dead variable onesrow (runica.py:595). After replacing both + _matmul(bias, onesrow) uses with + bias, onesrow = np.ones((1, block)) is only referenced in comments (lines 684, 871), so it's now assigned-but-never-used. AGENTS.md requires removing variables your change made unused, and ruff (F841) will flag this — so CI's uv run --no-sync ruff check . will likely fail. Fix: delete line 595 (the MATLAB-reference comments quoting bias*onesrow can stay as they document the MATLAB origin). Fix this →

Nits

  • Broadened np.errstate scope. Wrapping the whole while loop (e.g. runica.py:676, 863) suppresses divide/over/invalid warnings for all ops in the loop (np.exp, np.tanh, np.sum), not just the matmuls that _matmul previously guarded. Results are unchanged since errstate only affects warning emission, and this is arguably desirable given the existing blow-up checks — just noting the behavior change is slightly wider than "moving the context manager out."
  • .jules/bolt.md is a Jules-specific learning log, not EEGPrep source. Consider whether it belongs in the repo tree.

Test gaps

None. Existing tests/test_runica.py (21 cases per the PR) exercises the changed loops; the refactor is numerically identity-preserving so no new cases are required.

EEGLAB parity notes

None. The + bias broadcasting and @ operators still mirror MATLAB weights*data + bias*onesrow and the natural-gradient updates; float64 precision and update order are preserved.

Note: I couldn't run ruff/pytest in this environment (commands required approval that isn't available here), so the F841/CI claim is from static analysis of the diff — please confirm with ./pre-commit.py --changed-from origin/develop.
bolt-optimize-runica-16203114544017163981

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