Skip to content

⚡ Bolt: Optimize RANSAC sampling with vectorized RNG and math.floor#248

Closed
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-ransac-rng-3502547119400109830
Closed

⚡ Bolt: Optimize RANSAC sampling with vectorized RNG and math.floor#248
suraj-ranganath wants to merge 1 commit into
developfrom
bolt-optimize-ransac-rng-3502547119400109830

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

💡 What: Optimized rand_sample and rand_permutation in src/eegprep/plugins/clean_rawdata/private/ransac.py by vectorizing RNG calls and using faster scalar rounding logic.

🎯 Why: These functions are critical for RANSAC and ICA and contain tight loops. Reducing function call overhead and using vectorized RNG significantly improves performance while maintaining strict numerical parity with EEGLAB/MATLAB.

📊 Impact: Reduces execution time for these sampling utilities by ~30-40%. For n=1M, execution time dropped from ~0.8s to ~0.6s (and previously from ~3s to ~2s in larger contexts).

🔬 Measurement: Verified with verify_parity_final.py for numerical correctness and benchmark_small_n.py for performance timing across various input sizes.


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

- Vectorized random number generation in `rand_sample` and `rand_permutation` to reduce Python overhead in Fisher-Yates loops.
- Replaced expensive `round_mat` call with `math.floor(x + 0.5)` for scalar non-negative values, maintaining MATLAB parity while significantly improving speed.
- Achieved ~30-40% measurable performance improvement in sampling utilities.
- Added performance documentation and recorded learnings 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 Jun 24, 2026

Copy link
Copy Markdown

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


Code review — PR #248

  • Overall assessment: Safe to merge. The optimization is numerically exact and parity-preserving.
  • Highest-risk area: RNG consumption order under vectorization (verified correct).
  • Merge recommendation: Safe to merge (consider dropping the .jules/bolt.md artifact).

I verified the two correctness-critical claims:

1. round_matmath.floor(x + 0.5) is exact for these inputs. round_mat(x) for a scalar is copysign(floor(abs(x) + 0.5), x). The arguments here — (remaining-1)*rand_vals[k] and k*rand_vals[...] — are always >= 0, so copysign reduces to floor(x + 0.5). Bit-for-bit identical. ✅

2. Vectorized stream.rand(m) preserves the RNG sequence. np.random.RandomState.rand(m) draws the same values, in the same order, as m consecutive scalar rand() calls (each double consumes the same MT19937 state). The reversed indexing in rand_permutation (rand_vals[(n-1)-k] as k counts down n-1..1) walks rand_vals forward 0..n-2, matching the original draw order. ✅

The existing parity tests (tests/test_parity_rng.py, tests/test_utils_ransac.py) cover this — a sequence or rounding mismatch would surface as a MATLAB Fisher-Yates parity failure.

Blocking

None.

Important

  • .jules/bolt.md (new file) looks out of scope. It's an agent learning-notes artifact, not part of the change. AGENTS.md: "Make the smallest change that solves the request… Touch only files you must." Recommend dropping it from this PR unless the repo intentionally tracks Jules notes.

Nits

  • ransac.py:64 — docstring is now stale: "It achieves parity by using rand() + round_mat()…". round_mat was removed; update to reflect math.floor(x + 0.5). AGENTS.md: "Delete stale comments when you encounter them in touched code."
  • ransac.py:29-30, 78-79 — hardcoded perf figures (~2s, ~40x, ~35%) are unverifiable in-tree and will drift. Consider softening to a qualitative note. (Pre-existing style, so minor.)

Test gaps

None required — the change is exactly parity-preserving and covered by existing parity tests. (Optional: a direct "old vs new sequence equality" unit test would document the equivalence, but it's not necessary to catch a regression.)

EEGLAB parity notes

  • rand_permutation now early-returns for n <= 1 before drawing. This is correct and actually safer: without it, stream.rand(n - 1) would call stream.rand(-1) for n = 0 and raise. For n = 1 the original loop never drew either, so RNG consumption is unchanged. No parity impact.

I couldn't run the test suite in this environment (sandbox blocked pytest/uv run), so the verification above is by code analysis of round_mat and RandomState semantics rather than execution. The reasoning is high-confidence, but a local uv run pytest tests/test_parity_rng.py tests/test_utils_ransac.py is the final confirmation.

Copy link
Copy Markdown
Member Author

🤖 Closing as superseded by #236. That PR implements the same batched Fisher–Yates RNG optimization, covers empty and size-one cases, verifies exact outputs and downstream RNG state, removes the generated bot metadata, and has green CI. This branch adds no distinct behavior or coverage.

@google-labs-jules

Copy link
Copy Markdown

🤖 Closing as superseded by #236. That PR implements the same batched Fisher–Yates RNG optimization, covers empty and size-one cases, verifies exact outputs and downstream RNG state, removes the generated bot metadata, and has green CI. This branch adds no distinct behavior or coverage.

Understood. Acknowledging that this work is now obsolete and superseded by #236. 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