[Feature] Add optional parallel processing support for computationally intensive functions - #1187
feruzoripov wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces optional parallel processing support across several computationally intensive functions in NeuroKit2, including bio_process, ecg_findpeaks (ProMAC), entropy_multiscale, eeg_power, and microstates_segment, as well as internal thread-based parallelism for DFA in hrv_nonlinear. The review feedback highlights several areas for improvement: ensuring robust error handling in the parallel ProMAC implementation to prevent total failure on individual method errors, optimizing resource allocation in ProcessPoolExecutor by limiting workers to the task count, reducing overhead by batching pandas concatenation operations, and refactoring duplicated logic between sequential and parallel execution paths to improve maintainability and performance.
| results = joblib.Parallel(n_jobs=n_jobs)( | ||
| joblib.delayed(_run_method)(method, signal, sampling_rate, gaussian_sd, **kwargs) for method in promac_methods | ||
| ) |
There was a problem hiding this comment.
The parallel implementation of ProMAC does not handle exceptions for individual methods, unlike the sequential version. If one peak detection method fails, the entire joblib.Parallel call will raise an exception, causing ecg_findpeaks to fail. In the sequential version, errors are caught and stored in error_list, allowing the process to continue with other methods. You should wrap the logic inside the worker in a try...except block to maintain this robustness.
| # Process signals (parallel or sequential) | ||
| results = {} | ||
| if parallel and len(_tasks) > 1: | ||
| with concurrent.futures.ProcessPoolExecutor() as executor: |
There was a problem hiding this comment.
When using ProcessPoolExecutor without specifying max_workers, it defaults to the number of processors on the machine. Since the number of tasks is fixed and small (at most 6), spawning a large pool is inefficient. It is recommended to limit the number of workers to the number of tasks to reduce overhead.
| with concurrent.futures.ProcessPoolExecutor() as executor: | |
| with concurrent.futures.ProcessPoolExecutor(max_workers=len(_tasks)) as executor: |
| for name, _, _ in _tasks: | ||
| signals_df, info_dict = results[name] | ||
| bio_info.update(info_dict) | ||
| bio_df = pd.concat([bio_df, signals_df], axis=1) |
There was a problem hiding this comment.
Repeatedly calling pd.concat in a loop is computationally expensive because it involves copying the entire DataFrame at each step. A more efficient approach is to collect all DataFrames in a list and perform a single concatenation after the loop.
# Assemble results in consistent order
to_concat = [bio_df]
for name, _, _ in _tasks:
signals_df, info_dict = results[name]
bio_info.update(info_dict)
to_concat.append(signals_df)
bio_df = pd.concat(to_concat, axis=1)| if n_jobs == 1: | ||
| # Sequential execution (original behavior) | ||
| info["Value"] = np.array( | ||
| [ | ||
| _entropy_multiscale( | ||
| signal, | ||
| scale=scale, | ||
| coarsegraining=coarsegraining, | ||
| algorithm=algorithm, | ||
| dimension=dimension, | ||
| tolerance=info["Tolerance"], | ||
| refined=refined, | ||
| **kwargs, | ||
| ) | ||
| for scale in info["Scale"] | ||
| ] | ||
| ) | ||
| else: | ||
| # Parallel execution via joblib | ||
| try: | ||
| import joblib | ||
| except ImportError as e: | ||
| raise ImportError( | ||
| "NeuroKit error: entropy_multiscale(): the 'joblib' module is required " | ||
| "for parallel execution. Please install it first (`pip install joblib`).", | ||
| ) from e | ||
|
|
||
| info["Value"] = np.array( | ||
| joblib.Parallel(n_jobs=n_jobs)( | ||
| joblib.delayed(_entropy_multiscale)( | ||
| signal, | ||
| scale=scale, | ||
| coarsegraining=coarsegraining, | ||
| algorithm=algorithm, | ||
| dimension=dimension, | ||
| tolerance=info["Tolerance"], | ||
| refined=refined, | ||
| **kwargs, | ||
| ) | ||
| for scale in info["Scale"] | ||
| ) | ||
| for scale in info["Scale"] | ||
| ] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The sequential and parallel execution paths contain duplicated logic for calling _entropy_multiscale. This redundancy increases the maintenance burden. You can refactor this by defining a helper function or a task generator and then choosing the execution method based on n_jobs.
def _run(scale_factor):
return _entropy_multiscale(
signal,
scale=scale_factor,
coarsegraining=coarsegraining,
algorithm=algorithm,
dimension=dimension,
tolerance=info["Tolerance"],
refined=refined,
**kwargs,
)
if n_jobs == 1:
# Sequential execution (original behavior)
info["Value"] = np.array([_run(s) for s in info["Scale"]])
else:
# Parallel execution via joblib
try:
import joblib
except ImportError as e:
raise ImportError(
"NeuroKit error: entropy_multiscale(): the 'joblib' module is required "
"for parallel execution. Please install it first (`pip install joblib`).",
) from e
info["Value"] = np.array(
joblib.Parallel(n_jobs=n_jobs)(
joblib.delayed(_run)(s) for s in info["Scale"]
)
)| def _run_method(method_name, sig, sr, gsd, **kw): | ||
| func = _ecg_findpeaks_findmethod(method_name) | ||
| peaks = func(sig, sampling_rate=sr, **kw) | ||
| mask = np.zeros(len(sig)) | ||
| mask[peaks] = 1 | ||
| sd = sr * gsd / 1000 | ||
| shape = scipy.stats.norm.pdf(np.linspace(-sd * 4, sd * 4, num=int(sd * 8)), loc=0, scale=sd) | ||
| return np.convolve(mask, shape, "same") |
There was a problem hiding this comment.
The _run_method helper duplicates the convolution and Gaussian kernel logic found in _ecg_findpeaks_promac_addconvolve. Additionally, the Gaussian shape is recalculated for every method, even though it only depends on the sampling rate and gaussian_sd. It is better to calculate the kernel once and reuse the existing convolution logic to ensure consistency.
There was a problem hiding this comment.
all comments were addressed in new commits
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1187 +/- ##
==========================================
+ Coverage 57.85% 59.02% +1.16%
==========================================
Files 310 310
Lines 15701 15757 +56
==========================================
+ Hits 9084 9300 +216
+ Misses 6617 6457 -160 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Should I request a review from someone? |
|
Hi feruzoripov, thank you for the detailed PR with benchmarks for your changes! Also excuse the late response. This is very beneficial for the embarrassingly parallel sections of the code like in Now a few things that are open to discussion from my side:
Again, thank you for taking the time for your contribution! I look forward to your responses. I would also be happy if others posted their opinions. |
|
Thanks for the thorough review @DerAndereJohannes! All four points addressed:
And yes, fully agree that defaults should always be sequential, that's been the case throughout (n_jobs=1 / parallel=False). |
Description
This PR adds opt-in parallel/concurrent execution to several computationally intensive functions in NeuroKit2. All changes are backward-compatible, default behavior remains sequential, and no new hard dependencies are introduced (
joblibis already an optional dependency).Motivation
NeuroKit2 currently runs all computations sequentially on a single core. For researchers working with long recordings, multi-channel EEG, or batch processing pipelines, this leaves significant performance on the table. Several functions contain embarrassingly parallel workloads (independent loop iterations, independent signal pipelines) that can be sped up with minimal API changes.
Proposed Changes
New parameters
bio_process()parallel=Falseconcurrent.futures.ProcessPoolExecutorecg_findpeaks()(ProMAC)n_jobs=1joblib.Parallelentropy_multiscale()n_jobs=1joblib.Paralleleeg_power()n_jobs=1joblib.Parallelmicrostates_segment()n_jobs=1joblib.ParallelInternal optimization
_hrv_dfa()inhrv_nonlinear.py: monofractal and multifractal DFA now run concurrently viaconcurrent.futures.ThreadPoolExecutor(always-on, with sequential fallback on failure).Other additions
tests/tests_parallel.py: 4 new tests verifying parallel results match sequential results forbio_process,ecg_findpeaksProMAC, andentropy_multiscale.benchmarks/bench_parallel.py: Benchmark script comparing sequential vs parallel performance.README.rst: Added "Parallel Processing" section with usage examples.Benchmark Results
Measured on a 12-core machine (AMD/Intel), Python 3.12, averaged over 3 runs:
Speedups scale with workload size; longer recordings and more channels yield larger gains.
Design Decisions
n_jobs=1as default everywhere to preserve existing behavior and avoid surprises.bio_processusesparallel=False(bool) instead ofn_jobsbecause it spawns one process per signal type (up to 6), not an arbitrary worker pool.joblibis used for then_jobspattern (consistent with scikit-learn convention).concurrent.futuresis used where stdlib is sufficient (bio_process, HRV DFA).Checklist