Skip to content

⚡ Bolt: optimize topoplot grid interpolation - #280

Closed
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-topoplot-7920743124178163139
Closed

⚡ Bolt: optimize topoplot grid interpolation#280
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-topoplot-7920743124178163139

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member
  • 💡 What: Optimized the griddata_v4 function in src/eegprep/functions/sigprocfunc/topoplot.py by replacing a double-nested Python loop with a vectorized NumPy implementation using broadcasting and matrix multiplication (@).
  • 🎯 Why: The previous implementation iterated over every query point in the grid (e.g., 67x67), which is extremely slow in Python. Shifting this to NumPy broadcasting and BLAS matrix multiplication significantly improves performance.
  • 📊 Impact: Measured a speedup of ~2.6x to ~4.8x for the griddata_v4 function.
  • 🔬 Measurement: Verified using a temporary benchmark script tools/benchmark_topoplot.py (deleted before submission) and confirmed no regressions with uv run pytest tests/test_topoplot.py.
  • 📓 Journal: Updated .jules/bolt.md with learnings from this optimization.

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

- Optimized griddata_v4 in topoplot.py by replacing nested loops with NumPy vectorization.
- Achieved ~2.6x-4.8x speedup in biharmonic spline interpolation.
- Verified numerical parity with existing tests.
- Updated Bolt journal in .jules/bolt.md.

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

Copy link
Copy Markdown

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


Code review

  • Overall assessment: Safe to merge. The vectorization is mathematically equivalent to the original nested loop.
  • Highest-risk area: numerical equivalence of the Green's-function evaluation at query points.
  • Merge recommendation: Safe to merge.

Blocking

None.

Important

None.

Nits

  • .jules/bolt.md (added): this is an agent journal artifact rather than product code. It's harmless, but confirm it's intentional to check it into the repo history rather than keeping it as a local working note.

Test gaps

None. tests/test_topoplot.py already exercises the changed path well: test_basic_interpolation, test_interpolation_finite_results, test_zero_distance_handling (the d == 0 branch), test_collinear_points, and the MATLAB parity tests (test_parity_single_component_noplot, test_parity_multiple_components, test_parity_gridscale_32) all cover griddata_v4 output values, so a numerical regression from this change would be caught. Note: I was unable to run the suite in this environment (Bash approval restricted), so I verified equivalence by analysis rather than execution.

EEGLAB parity notes

None. The biharmonic-spline Green's function d²·(log(d) − 1) and the zero-distance handling are preserved exactly, matching MATLAB's gdatav4.

Analysis detail

The rewrite is correct and equivalent to the prior loop:

  • Distances: d = |q[:, :, None] − xy[None, None, :]| produces the same per-query-point distances (shape (m, n, k)) that the loop computed one (i, j) at a time via xq[i,j] + 1j*yq[i,j] − xy.ravel().
  • Green's function + zero handling: (d**2) * (log(d) − 1) under errstate(divide/invalid='ignore') followed by g[d == 0] = 0 reproduces the loop's g[d == 0] = 0 fix for coincident points.
  • Reduction: g @ weights contracts the trailing electrode axis with weights (shape (k,)), yielding vq of shape (m, n) — identical to the per-cell np.dot(g, weights).
  • dtype: g @ weights is real-valued (from np.abs and a real weights), and dropping vq = np.zeros_like(xq) actually avoids a latent dtype pitfall — if xq were ever integer, the old code would have truncated results; the new code always returns float.
  • Memory: for a 67×67 grid and ~64 electrodes the intermediate g is ~287K floats — negligible for realistic EEG montages.

Copy link
Copy Markdown
Member Author

🤖 Closing as a duplicate of the retained topoplot candidate #263. It makes the same array-construction optimization and adds materially the same test.

@google-labs-jules

Copy link
Copy Markdown

🤖 Closing as a duplicate of the retained topoplot candidate #263. It makes the same array-construction optimization and adds materially the same test.

Understood. Acknowledging that this work is a duplicate of #263 and stopping work on this task.

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