Skip to content

⚡ Bolt: vectorize trial loop in jointprob#272

Open
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-jointprob-vectorization-3768410077254852708
Open

⚡ Bolt: vectorize trial loop in jointprob#272
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-jointprob-vectorization-3768410077254852708

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

💡 What: The optimization implemented

Vectorized the inner loop over trials in the jointprob function within src/eegprep/functions/popfunc/_rejection.py. Instead of iterating through each trial and slicing the probability array, we now reshape the entire probability vector into a 2D array of (trials, points) and perform the log-summation across the points axis in a single NumPy operation.

🎯 Why: The performance problem it solves

The previous implementation used a nested Python loop to iterate over trials for every channel. For EEG datasets with a large number of trials (epochs), this Python loop overhead and repeated slicing became a significant bottleneck in rejection scoring.

📊 Impact: Expected performance improvement

Expected performance improvement of ~40% for standard EEG datasets (e.g., 64 channels, 500 points, 200 trials).

🔬 Measurement: How to verify the improvement

The optimization was verified using a benchmark script comparing the original logic with the vectorized approach. Correctness was confirmed by a parity test ensuring identical results for the joint probability scores.

# Benchmark summary
Original average:   0.3083s
Vectorized average: 0.1814s
Improvement: 41.2%

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

Vectorize the inner trial loop in the \`jointprob\` function by reshaping the
probabilities array and using NumPy axis-based aggregation. This reduces
the overhead of Python loops and slice-based indexing.

Benchmark results (64 channels, 500 points, 200 trials):
- Original: 0.3083s
- Vectorized: 0.1814s
- Improvement: ~41%

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 4, 2026

Copy link
Copy Markdown

Claude finished @suraj-ranganath's task in 1m 25s —— View job


Code review

  • Overall assessment: Safe to merge.
  • Highest-risk area: Numerical equivalence of the reshape vs. the original per-trial slicing.
  • Merge recommendation: Safe to merge.

I verified the vectorization is numerically identical to the original loop, not just approximately equal:

In jointprob, probabilities = _realproba(arr[row].reshape(-1, order="F")). Since arr[row] has shape (points, trials), the Fortran-order flatten lays the array out trial-major with points contiguous within each trial, so it has length trials * points. The original code sliced probabilities[t*P : (t+1)*P] for trial t. The new probabilities.reshape(arr.shape[2], arr.shape[1]) uses default C order, whose row t is exactly probabilities[t*P : (t+1)*P] — same slices. Summing with axis=1 reproduces the per-trial sums. -np.sum(np.log(np.maximum(..., tiny))) is unchanged, and hoisting tiny = np.finfo(float).tiny out of the loop is a harmless, correct micro-optimization.

Existing coverage in tests/test_rejection_workflows.py (test_jointprob_global_marks_match_eeglab_trial_rows_for_duplicate_channels, test_jointprob_global_threshold_can_reject_when_local_threshold_does_not) exercises jointprob/jointprob_marks directly and against EEGLAB expectations, so the equivalence is guarded — no new test required.

Blocking

None.

Important

None.

Nits

  • .jules/bolt.md (ADDED) is agent scratch/learning notes rather than package content. Not blocking, but consider whether it belongs in the repo tree per the "touch only files you must" guidance in AGENTS.md.

Test gaps

None. Changed behavior is byte-for-byte equivalent and already covered.

EEGLAB parity notes

None. Scores are unchanged, so pop_jointprob parity with EEGLAB is preserved.

Checked for correctness bugs, EEGLAB parity, data-structure compatibility, changed-behavior tests, and CLAUDE.md/AGENTS.md compliance. (Note: I could not run the test suite in this environment due to sandbox command restrictions, but the equivalence is provable by inspection of the array layout.)

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