Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] SCR_Peaks_Amplitude_Mean should be only computed on peaks #917

Merged
merged 5 commits into from
Oct 9, 2023
Merged

[Fix] SCR_Peaks_Amplitude_Mean should be only computed on peaks #917

merged 5 commits into from
Oct 9, 2023

Conversation

mperreir
Copy link
Contributor

@mperreir mperreir commented Oct 2, 2023

Description

For the moment eda_intervalrelated() computes the mean peaks amplitude based on the whole signal (where most of the amplitude values are zero). This leads to very low (and wrong) SCR_Peaks_Amplitude_Mean values.

Proposed Changes

Compute the mean peaks amplitude only on the peaks (and not on the whole signal). For that we first select rows where SCR_Peaks has a value of 1.

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • 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)

Compute the mean peaks amplitude only on the peaks (and not on the whole signal)
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (44959c6) 55.00% compared to head (984194a) 55.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #917   +/-   ##
=======================================
  Coverage   55.00%   55.01%           
=======================================
  Files         303      303           
  Lines       14210    14213    +3     
=======================================
+ Hits         7816     7819    +3     
  Misses       6394     6394           
Files Coverage Δ
neurokit2/eda/eda_intervalrelated.py 78.26% <87.50%> (+1.51%) ⬆️

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

@DominiqueMakowski
Copy link
Member

@mperreir
Copy link
Contributor Author

mperreir commented Oct 2, 2023

Yes, it seems. I was not expecting that. It might be fixed with something like this:

only_peaks = data["SCR_Peaks"] == 1
if only_peaks.sum() > 0:
    output["SCR_Peaks_Amplitude_Mean"] = np.nanmean(data[data["SCR_Peaks"] == 1]["SCR_Amplitude"].values)
else:
    output["SCR_Peaks_Amplitude_Mean"] = np.nan

I'll try to push this tomorrow.

mperreir and others added 3 commits October 3, 2023 09:41
Added a test to avoid a "RuntimeWarning: Mean of empty slice" for intervals where no peak is detected
@pull-request-size pull-request-size bot added size/M and removed size/XS labels Oct 3, 2023
@DominiqueMakowski DominiqueMakowski merged commit 484dd7f into neuropsychology:dev Oct 9, 2023
8 checks passed
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.

3 participants