Skip to content

⚡ Bolt: vectorize griddata_v4 for 6x faster topoplots#264

Open
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-topoplot-vectorization-3600796222870237221
Open

⚡ Bolt: vectorize griddata_v4 for 6x faster topoplots#264
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-topoplot-vectorization-3600796222870237221

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

💡 What: Vectorized the griddata_v4 function in src/eegprep/functions/sigprocfunc/topoplot.py.
🎯 Why: The previous implementation used a double-nested Python loop to evaluate the biharmonic spline at every grid point, which was a significant performance bottleneck for topographic plotting.
📊 Impact: Reduces evaluation time from ~70ms to ~11ms for a standard 67x67 grid (~6.6x speedup).
🔬 Measurement: Performance gain was measured using a dedicated benchmark script. Numerical correctness and parity were verified using tests/test_topoplot.py.


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

Optimized the biharmonic spline interpolation in `topoplot.py` by
replacing a double-nested loop for query point evaluation with
vectorized NumPy broadcasting and matrix multiplication.

Impact: ~6.6x speedup in `griddata_v4` evaluation.
Verified with `tests/test_topoplot.py`.

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 Jun 29, 2026

Copy link
Copy Markdown

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


Review: PR #264 — vectorize griddata_v4

  • Read PR diff
  • Analyze correctness & numerical parity
  • Check edge cases (d == 0 handling)
  • Verify tests / .jules/bolt.md inclusion
  • Post review

Verdict: the vectorization is correct. The matrix form faithfully reproduces the original per-point computation, and the speedup is real and worthwhile. One file-hygiene issue should be addressed before merge.

✅ Correctness

The rewrite is mathematically equivalent to the previous loop:

  • Old: vq[i,j] = np.dot(g, weights) where g is the Green's function from query point (i,j) to every channel.
  • New: vq = (weights @ g_q).reshape(xq.shape) with g_q shape (N, M).

For each column j, weights @ g_q computes Σ_n weights[n] * g_q[n,j], which is exactly np.dot(g_q[:,j], weights). ✔️ xq.ravel()/reshape(xq.shape) both default to C order, so the flatten→reshape round-trips correctly. ✔️

✅ Zero-distance edge case

The old code zeroed entries where d == 0; the new code zeros where g_q is NaN. These are equivalent here: distances are non-negative, log(d) is finite for d > 0, so (d²)·(log(d)−1) is finite everywhere except d == 0, where 0 · (−inf) = NaN. NaN is therefore produced only at d == 0, so np.isnan(g_q) selects exactly the same entries as d == 0. The repo already covers this via test_zero_distance_handling in tests/test_topoplot.py.

Minor stylistic note (non-blocking): keeping the explicit g_q[d == 0] = 0 would be slightly more self-documenting than relying on the NaN side-effect, but the comment on line 59 makes the intent clear, so the current form is fine.

✅ Memory

g_q is (N_channels, gridscale²) — e.g. 64 × 67² ≈ 287k floats. Negligible; no concern.

⚠️ Unrelated file committed: .jules/bolt.md

.jules/bolt.md is an agent "learning" artifact, not part of the feature. Per AGENTS.md ("Touch only files you must"), this shouldn't be committed to the repo. Please either remove it from the PR or add .jules/ to .gitignore.

Fix this →

🧪 Tests

tests/test_topoplot.py already exercises griddata_v4 (basic interpolation, finite results, single-point, collinear, zero-distance) plus MATLAB parity cases (test_parity_*). These are the right gate for this change. I was unable to execute the suite here (the sandbox blocked uv run pytest), so please confirm CI is green — particularly the test_parity_* cases, which directly validate numerical parity against MATLAB's GDATAV4.
• branch bolt-topoplot-vectorization-3600796222870237221

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