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

Add example on ERP #144

Merged
merged 11 commits into from Nov 5, 2021
Merged

Add example on ERP #144

merged 11 commits into from Nov 5, 2021

Conversation

qbarthelemy
Copy link
Member

@qbarthelemy qbarthelemy commented Oct 23, 2021

This PR:

  • adds plot_erp and complete test;
  • add an example on ERP display, especially with 2D histogram.

It also renames the ERP example on embedding, because the script selects MEG channels instead of EEG ones. @plcrodrigues

@plcrodrigues
Copy link
Member

I like this new functionality quite a lot. Thanks, @qbarthelemy

I will however play the devil's advocate and ask the following question: Should we really use the term Event-related potential ?. I know this is the correct terminology when we are dealing with EEG-related applications and neuroscience. However, what if we called it something like a "multi-trial" signal ? This would make the package a bit more agnostic regarding its applications on signal processing. What do you think ?

pyriemann/utils/viz.py Outdated Show resolved Hide resolved
@sylvchev
Copy link
Member

I like this new functionality quite a lot. Thanks, @qbarthelemy

Same here! Thanks @qbarthelemy

I will however play the devil's advocate and ask the following question: Should we really use the term Event-related potential ?. I know this is the correct terminology when we are dealing with EEG-related applications and neuroscience. However, what if we called it something like a "multi-trial" signal ? This would make the package a bit more agnostic regarding its applications on signal processing. What do you think ?

I agree on the general line of questioning, but for this example I don't have a specific term in mind for the general signal processing case, whereas ERP is quite specific of this situation. Thus I'm in favor of keep the plot_erp name.

pyriemann/utils/viz.py Outdated Show resolved Hide resolved
pyriemann/utils/viz.py Outdated Show resolved Hide resolved
@agramfort
Copy link
Member

agramfort commented Oct 25, 2021 via email

@qbarthelemy
Copy link
Member Author

This would make the package a bit more agnostic regarding its applications on signal processing. What do you think ?

I totally agree. With this function, we could plot QRS peaks from ECG.

I have updated documentation (ERP has been replaced by waveform), but also function name (otherwise, erp term is unexplained).

@qbarthelemy
Copy link
Member Author

why having both options? is it really necessary?

Ok, I will refactor the function to remove this parameter.

pyriemann/utils/viz.py Outdated Show resolved Hide resolved
tests/test_viz.py Show resolved Hide resolved
doc/whatsnew.rst Outdated Show resolved Hide resolved
@qbarthelemy
Copy link
Member Author

qbarthelemy commented Nov 4, 2021

@sylvchev , any idea about documentation build fail?
https://github.com/pyRiemann/pyRiemann/runs/4109284600?check_suite_focus=true#step:4:118

EDIT: solved defining mne==0.23.4 in doc/requirements.txt (problem in 0.24.0)

pyriemann/utils/viz.py Outdated Show resolved Hide resolved
@agramfort agramfort merged commit f06a33e into pyRiemann:master Nov 5, 2021
@agramfort
Copy link
Member

thx @qbarthelemy !

@qbarthelemy qbarthelemy deleted the example_erp branch November 5, 2021 20:33
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.

None yet

4 participants