Skip to content

⚡ Bolt: optimize griddata_v4 in topoplot - #279

Closed
suraj-ranganath wants to merge 1 commit into
developfrom
bolt/optimize-topoplot-v4-634771604121603696
Closed

⚡ Bolt: optimize griddata_v4 in topoplot#279
suraj-ranganath wants to merge 1 commit into
developfrom
bolt/optimize-topoplot-v4-634771604121603696

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

💡 What: Vectorized the evaluation of query points in griddata_v4 using NumPy broadcasting and matrix multiplication (@).

🎯 Why: The previous implementation used a double-nested loop to iterate over every grid point and channel, which was a significant performance bottleneck in topographic plotting.

📊 Impact: Achieved a ~6x speedup (from ~0.065s to ~0.011s per call) for a standard 67x67 grid with 64 channels. This makes topographic maps feel much more responsive in interactive workflows.

🔬 Measurement: Verified with a temporary benchmark script tools/benchmark_topoplot.py and ensured numerical correctness and parity with tests/test_topoplot.py.


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

Vectorized the evaluation of query points in griddata_v4 using NumPy
broadcasting and matrix multiplication (@).

- Replaced double-nested loop with vectorized NumPy operations.
- Achieved ~6x speedup (0.065s -> 0.011s) for standard 67x67 grids.
- Maintained numerical parity with the original implementation.
- Updated .jules/bolt.md with learnings.

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

Copy link
Copy Markdown

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


Code Review: griddata_v4 vectorization

Verdict: Looks correct and is a clean, well-scoped optimization.

I traced the numerical equivalence of the old nested loop vs. the new vectorized evaluation in src/eegprep/functions/sigprocfunc/topoplot.py (lines 48–54).

Correctness ✅

  • The old code reassigned xy = xy[:, None] before the loop; the new code correctly drops that reassignment and keeps xy as the 1D array x + 1j*y (shape (n_chan,)).
  • Broadcasting is sound: xy_q[..., np.newaxis] has shape (m, n, 1), and subtracting xy gives (m, n, n_chan). Then g_q @ weights is a batched matmul (m, n, n_chan) @ (n_chan,) → (m, n), exactly matching the per-point np.dot(g, weights) of the old loop.
  • The Green's-function singularity handling is preserved: g_q[d_q == 0] = 0 replicates the old g[d == 0] = 0, and the np.errstate guard still suppresses the log(0) warning.
  • Result is real-valued (d_q is a real magnitude, weights is real), matching the old np.zeros_like(xq) output.

So this is a faithful, allocation-light rewrite. The ~6x speedup claim is plausible — eliminating a 67×67 = 4489-iteration Python loop in favor of a single broadcast + matmul.

Minor notes (non-blocking)

  1. Dtype subtlety: the old code seeded vq with np.zeros_like(xq), so if xq/yq were ever integer arrays the old output would have been truncated to int. The new code always returns float. In practice xq/yq come from np.meshgrid(np.linspace(...)) (float), so this is a non-issue and arguably an improvement — just flagging it.
  2. Memory: d_q / g_q now materialize (m, n, n_chan) intermediates (~2.3 MB for 67×67×64 float64). Negligible for typical montages; would only matter for very large grids × very high channel counts. No action needed.
  3. .jules/bolt.md is added to the repo as a bot "learning" note. Worth confirming with the maintainers whether that file belongs in version control — it reads like agent-internal state rather than project content. The temporary tools/benchmark_topoplot.py mentioned in the PR body was correctly left out of the diff. 👍

Suggested verification

I was unable to run the test suite in this environment (command execution is sandboxed here). Please confirm parity locally:

EEGPREP_SKIP_MATLAB=1 uv run pytest tests/test_topoplot.py

and run ./pre-commit.py --changed-from origin/develop for lint/format.


Nice, focused optimization — the interpolation math is preserved and the code is more readable than the double loop.

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