Skip to content

[Fix] Forward custom amplitude_min through eda_process (#1197) - #1203

Open
SyedAshhadIbrar wants to merge 1 commit into
neuropsychology:devfrom
SyedAshhadIbrar:fix_eda_process_amplitude_min
Open

[Fix] Forward custom amplitude_min through eda_process (#1197)#1203
SyedAshhadIbrar wants to merge 1 commit into
neuropsychology:devfrom
SyedAshhadIbrar:fix_eda_process_amplitude_min

Conversation

@SyedAshhadIbrar

Copy link
Copy Markdown

Description

Fixes #1197.

eda_process() advertised **kwargs forwarding but then called eda_peaks(..., amplitude_min=0.1, **kwargs_peaks). The hardcoded keyword always won, and a custom amplitude_min never reached peak detection. eda_methods() also stored user kwargs under a nested "kwargs" key, so get_kwargs() could not pick them up (unlike rsp_methods(), which unpacks **kwargs into report_info).

The default remains 0.1 — that value still lives on eda_peaks() — but callers can now do:

signals, info = nk.eda_process(eda, sampling_rate=250, amplitude_min=0.03)

amplitude_min is a relative threshold (fraction of the largest SCR), not an absolute conductance in siemens.

Proposed Changes

  • Unpack user kwargs in eda_methods() the same way rsp_methods() does.
  • Stop hardcoding amplitude_min=0.1 in eda_process(); let eda_peaks() supply the default.
  • Document amplitude_min on eda_process().
  • Add a regression test that the kwarg is forwarded and changes detected peak counts.
  • Note the fix in NEWS.rst.

Checklist

  • I have read the CONTRIBUTING file.
  • My PR is targeted at the dev branch (and not towards the master branch).
  • I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • I have added the newly added features to News.rst (if applicable)

Hardcoded amplitude_min=0.1 prevented a custom relative SCR
threshold from reaching eda_peaks. Unpack user kwargs in
eda_methods (same pattern as rsp_methods) and keep the 0.1
default on eda_peaks.

Fixes neuropsychology#1197
@SyedAshhadIbrar
SyedAshhadIbrar changed the base branch from master to dev August 31, 2026 02:16
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Aug 31, 2026
@SyedAshhadIbrar SyedAshhadIbrar changed the title [Fixed] Forward custom amplitude_min through eda_process (#1197) [Fix] Forward custom amplitude_min through eda_process (#1197) Aug 31, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.18%. Comparing base (9f3a1a5) to head (53d85d2).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1203   +/-   ##
=======================================
  Coverage   58.17%   58.18%           
=======================================
  Files         310      310           
  Lines       15702    15702           
=======================================
+ Hits         9135     9136    +1     
+ Misses       6567     6566    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hardcoded value for min amplitude in eda.eda_process

2 participants