mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.66k stars 1.31k forks source link

ERP Measurement Tool for MNE #7848

Open jdkoen opened 4 years ago

jdkoen commented 4 years ago

The Evoked object only has methods to return peak amplitude and latency at present. Is there any interest in the development of additional amplitude and latency measures (e.g., average amplitude in a time window, fractional peak latency)? My thought would be to port the some of the measures implemented in ERPLAB's ERP Measurement Tool to MNE to help facilitate ERP analysis using MNE.

I am happy to lead development of this. These features could be added as methods to the Evoked object, or alternatively be included as a separate library. Is there a preference on which route is best?

Also, while I have coded in Python before, this will be my first time contributing to any Python software development. Thus, I would likely need some help and guidance on navigating development of unit tests and other aspects of contributing.

agramfort commented 4 years ago

hi @jdkoen my suggestion would be to start small with what you consider the most key feature of ERPLAB. What would that be? We'll be happy to support you.

jdkoen commented 4 years ago

Thanks @agramfort! One of the key features (at least to me) is pulling out mean amplitude in a user defined time window. I will start with that, and post here if I have questions or need help.

agramfort commented 4 years ago

I would do this with a .mean() method on Evoked like we have a .mean() method on stc

jdkoen commented 4 years ago

Thank you for the suggestion.

I just noticed that the .get_peak() method returns the peak amplitude/time over an entire class of channels (i.e., the peak across all channels). Would it be useful to have a different function that extracts the peak amplitude/voltage for a user-specified set of channels? I was unsure of the rationale for taking a 'global' peak value/latency.

The reason I ask the above question, and more generally, is I wonder if it will be useful to create a new class to deal with extracting data from evoked objects. From what I have in mind, it would require ~5-6 new methods on the Evoked class (with associated private functions).

Currently familiarizing myself with the code structure, but wanted to ask this now before I start to code.

On Fri, May 29, 2020 at 3:19 AM Alexandre Gramfort notifications@github.com wrote:

I would do this with a .mean() method on Evoked like we have a .mean() method on stc

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/7848#issuecomment-635806453, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVF5ZEBID2KUPEH6NPLRT5OZNANCNFSM4NNL24NQ .

drammock commented 4 years ago

@jdkoen for a peak over a subset of channels, there are methods evoked.pick, evoked.pick_channels, and evoked.pick_types; these return an evoked instance with a reduced number of channels, so they can be chained with get_peak like so: evoked.copy().pick_channels(['Cz', 'Fz', 'C3', 'C4']).get_peak()

Note the use of copy() because otherwise the various pick methods will modify evoked in place.

jdkoen commented 4 years ago

Thanks @drammock for that feedback. That approach just slipped my mind. Makes a lot more sense now for how to format the mean signal measure I'm trying to add.

mmagnuski commented 4 years ago

One of the key features (at least to me) is pulling out mean amplitude in a user defined time window.

Chaining can be useful here too:

evoked.copy().crop(tmin=0.09, tmax=0.11).pick_channels(channel_list).data.mean()

will give you average over specified time window and channel_list.

Maybe a good first step would be to add a short example/tutorial dealing with just a few of these measures and then we'll see if some turn out to be too complex to obtain with what is available in mne now.

larsoner commented 4 years ago

Maybe a good first step would be to add a short example/tutorial dealing with just a few of these measures and then we'll see if some turn out to be too complex to obtain with what is available in mne now.

I think we should make a tutorial on ERP-style analyses that contain 90% of the analyses people would be interested in. Even if it's long, a .. contents:: at the top will make it easy for people to find what they're looking for. This exercise would make it easy to see what we should turn into a separate function in MNE, if anything.

larsoner commented 4 years ago

@sappelhoff or @mmagnuski up for coding this in the next couple of weeks by any chance?

sappelhoff commented 4 years ago

I think we should make a tutorial on ERP-style analyses that contain 90% of the analyses people would be interested in. Even if it's long, a .. contents:: at the top will make it easy for people to find what they're looking for.

I am pretty packed so would be grateful if somebody else can take the time. But I agree that this would be very useful to have.

mmagnuski commented 4 years ago

@larsoner Sorry, I won't be able to do this for the next release. But I will have more time in about 1.5 months - then I could help with such tutorial. :)

larsoner commented 4 years ago

Sounds good. Or @jdkoen if you've made some progress, you could also take a stab at it if you want!

jdkoen commented 4 years ago

Thanks. I’m happy to take a go at it. Sorry for the delay getting back to this.

On Tue, Aug 25, 2020 at 7:23 PM Eric Larson notifications@github.com wrote:

Sounds good. Or @jdkoen https://github.com/jdkoen if you've made some progress, you could also take a stab at it if you want!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/7848#issuecomment-680316951, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVE4BYDFCSVXVP2JHILSCRBWNANCNFSM4NNL24NQ .

jdkoen commented 4 years ago

Here are my thoughts on what measures to add for a tutorial:

1) Peak amplitude/latency for a single channel 2) Mean amplitude in a time interval 3) Fractional Area Latency (for an example, 50% area) 4) Fractional Peak Latency (to measure onsets)

There are some other measures (e.g., integral measures) that I am not looking at here. Also, for peak measures (1, 3, and 4 above) there should likely be options to find absolute, positive, and negative peaks.

Any thoughts on this?

On Tue, Aug 25, 2020 at 7:25 PM Joshua Koen koen.joshua@gmail.com wrote:

Thanks. I’m happy to take a go at it. Sorry for the delay getting back to this.

On Tue, Aug 25, 2020 at 7:23 PM Eric Larson notifications@github.com wrote:

Sounds good. Or @jdkoen https://github.com/jdkoen if you've made some progress, you could also take a stab at it if you want!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/7848#issuecomment-680316951, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVE4BYDFCSVXVP2JHILSCRBWNANCNFSM4NNL24NQ .

larsoner commented 4 years ago

I would start with some minimal set and we can always add more later. No need to be feature-complete from the start. So this seems fine.

One other one I see/use from time to time is area under the curve, but again we can add that later

larsoner commented 4 years ago

Just a cross-ref here that we should bring up the "head" coordinate frame standard in MNE in this tutorial as well (see #7455 for example)

larsoner commented 4 years ago

Pushing the milestone to 0.22 for this one so it's not a blocker, but @jdkoen there is still time to get this in for 0.21 if you have time to make a push in the next few days

jdkoen commented 3 years ago

I will have to get to it for the 0.22 release. Hopefully I will have a little more spare time at the end of this upcoming week.

On Wed, Sep 9, 2020 at 9:32 AM Eric Larson notifications@github.com wrote:

Pushing the milestone to 0.22 for this one so it's not a blocker, but @jdkoen https://github.com/jdkoen there is still time to get this in for 0.21 if you have time to make a push in the next few days

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/7848#issuecomment-689565409, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVAV4J4GL2XOXTG4AODSE57WFANCNFSM4NNL24NQ .

larsoner commented 3 years ago

@jdkoen we might push 0.22 up to a December rather than March release, feel free to give a PR a shot in the next week or so if you can, otherwise 0.23 is also useful!

jdkoen commented 3 years ago

Will do. Going to start work on it tonight.

On Tue, Dec 1, 2020 at 1:55 PM Eric Larson notifications@github.com wrote:

@jdkoen https://github.com/jdkoen we might push 0.22 up to a December rather than March release, feel free to give a PR a shot in the next week or so if you can, otherwise 0.23 is also useful!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/7848#issuecomment-736750649, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVGPA5W4K7JFBMBXMVTSSU33DANCNFSM4NNL24NQ .

hasibagen commented 3 years ago

Hello, I was recently reading the chapter about erp peak in the luck's book, and I am very interested in erplab's peak finding tool. But mne's peak finding tool may indeed be a bit unusable for me. When I used mne to analyze eeg data, I exported evoke to pandas after all the analysis was completed to do time slices and find peaks. I feel that this method does have a big problem. I am very interested in your work. If there is any progress or toolkit code, I will be very willing to try it.

drammock commented 2 years ago

I think we should make a tutorial on ERP-style analyses that contain 90% of the analyses people would be interested in. Even if it's long, a .. contents:: at the top will make it easy for people to find what they're looking for. This exercise would make it easy to see what we should turn into a separate function in MNE, if anything.

this was done in #8655 (thanks @jdkoen !) and the resulting tutorial section is here. @jdkoen were there any computations in there that were complicated / unintuitive enough for end users that you think we should add a dedicated method? (If not, we can close this issue I think)

jdkoen commented 2 years ago

The tutorial I wrote only focused on two common methods (peak amplitude/latency and mean amplitude) but does not include other methods such as fractional peak onset latency, fractional area latency, and signed area latency. These other measures are growing in use and would be of interest to ERP researchers using MNE python.

There are a couple of new methods/features that I think can be implemented:

  1. I think the get_peak() method should be updated a little bit. At present, it only detects global peak voltages in a time window of interest, but does not detect local peaks which might be more ideal. We could add this using scipy.signal.find_peaks, which I have been using lately.
  2. A mean amplitude method would be useful IMHO. While it is fairly straightforward to do with stringing methods together, this may not be as easy for users with less experience in programming.

As for the other measures that are currently not in the tutorial, I do agree that a tutorial for these methods will be useful. I think a tutorial can include ways to go about obtaining these measures (such as one off functions in the tutorial) would benefit the users of MNE. I have some functions drafted for all of these from a recent study from my lab (which I'm happy to share), and this could lay the groundwork for determining what new methods (or classes) may be needed.

I do wonder if a new class (e.g., ERPMeasurer) that incorporates these methods, as well as a viewer to inspect the results, would be useful. This would be similar to the ERP Measurement Tool in ERPLAB which has an outstanding interactive viewer.

I'm very much willing to help with creating this tutorial, and any new potential methods/classes that result from the tutorial. Do others feel this would be beneficial to have a more advanced tutorial?

On Thu, Oct 14, 2021 at 11:13 AM Daniel McCloy @.***> wrote:

I think we should make a tutorial on ERP-style analyses that contain 90% of the analyses people would be interested in. Even if it's long, a .. contents:: at the top will make it easy for people to find what they're looking for. This exercise would make it easy to see what we should turn into a separate function in MNE, if anything.

this was done in #8655 https://github.com/mne-tools/mne-python/pull/8655 (thanks @jdkoen https://github.com/jdkoen !) and the resulting tutorial section is here https://mne.tools/dev/auto_tutorials/evoked/30_eeg_erp.html#amplitude-and-latency-measures. @jdkoen https://github.com/jdkoen were there any computations in there that were complicated / unintuitive enough for end users that you think we should add a dedicated method? (If not, we can close this issue I think)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/7848#issuecomment-943455537, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVDOVXMR3VRA4KX7Y2LUG3XSPANCNFSM4NNL24NQ .

hasibagen commented 2 years ago

The tutorial I wrote only focused on two common methods (peak amplitude/latency and mean amplitude) but does not include other methods such as fractional peak onset latency, fractional area latency, and signed area latency. These other measures are growing in use and would be of interest to ERP researchers using MNE python. There are a couple of new methods/features that I think can be implemented: 1. I think the get_peak() method should be updated a little bit. At present, it only detects global peak voltages in a time window of interest, but does not detect local peaks which might be more ideal. We could add this using scipy.signal.find_peaks, which I have been using lately. 2. A mean amplitude method would be useful IMHO. While it is fairly straightforward to do with stringing methods together, this may not be as easy for users with less experience in programming. As for the other measures that are currently not in the tutorial, I do agree that a tutorial for these methods will be useful. I think a tutorial can include ways to go about obtaining these measures (such as one off functions in the tutorial) would benefit the users of MNE. I have some functions drafted for all of these from a recent study from my lab (which I'm happy to share), and this could lay the groundwork for determining what new methods (or classes) may be needed. I do wonder if a new class (e.g., ERPMeasurer) that incorporates these methods, as well as a viewer to inspect the results, would be useful. This would be similar to the ERP Measurement Tool in ERPLAB which has an outstanding interactive viewer. I'm very much willing to help with creating this tutorial, and any new potential methods/classes that result from the tutorial. Do others feel this would be beneficial to have a more advanced tutorial? On Thu, Oct 14, 2021 at 11:13 AM Daniel McCloy @.***> wrote: I think we should make a tutorial on ERP-style analyses that contain 90% of the analyses people would be interested in. Even if it's long, a .. contents:: at the top will make it easy for people to find what they're looking for. This exercise would make it easy to see what we should turn into a separate function in MNE, if anything. this was done in #8655 <#8655> (thanks @jdkoen https://github.com/jdkoen !) and the resulting tutorial section is here https://mne.tools/dev/auto_tutorials/evoked/30_eeg_erp.html#amplitude-and-latency-measures. @jdkoen https://github.com/jdkoen were there any computations in there that were complicated / unintuitive enough for end users that you think we should add a dedicated method? (If not, we can close this issue I think) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7848 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXXPVDOVXMR3VRA4KX7Y2LUG3XSPANCNFSM4NNL24NQ .

Yes, I'm very much looking forward to it. Looking forward to more detailed and advanced tutorials on how to do this. Looking forward to your sharing.

SimonErnesto commented 2 years ago

Hi everyone. I was recently working on implementing these functions in Python and I found this thread while researching. I have written a small function implementing fractional peak and fractional area methods. I thought you may find this useful, so I opened this repo: https://github.com/SimonErnesto/find_latency

Implementing these methods wasn't particularly difficult and they seem to work fine as far as I can tell, but surely this little function can be improved in many ways. Any feedback is very welcome and if I may be of use just let me know.

agramfort commented 2 years ago

would it add new functions to the public API of MNE? can you do the addition by adding one parameter on an existing method?

can you share the paper explaining this?

Message ID: @.***>

SimonErnesto commented 2 years ago

I'm sorry, I haven't collaborated on development before, so I'm not sure whether I understood your first two questions. Do you mean whether the function I wrote could be integrated into an existing MNE method?

Sadly, I wasn't able to find a paper explaining these methods explicitly. Papers (e.g. Kiesel et al., 2008: https://pubmed.ncbi.nlm.nih.gov/17995913/ ) or books (e.g. Luck, 2014: https://books.google.co.uk/books?id=y4-uAwAAQBAJ ) reporting these measurement techniques tend to keep the math and code hidden (though I may be wrong, and I'm not searching properly). However, I have added two links that explain how these methods are implemented in Matlab in the repo. The areas computed by the fractional area method are very similar to the ones I obtained using integration (numpy's trapz: trapezoidal composite rule). I think that small differences may be due to the corrections applied to the methods, based on the code here: https://lindeloev.net/hej-verden/ (this is the second link in the repo shared in the previous comment). Sorry if I missed something.

agramfort commented 2 years ago

@jdkoen do you have an opinion on this?

jdkoen commented 2 years ago
  1. Thanks @SimonErnesto for sending this along. I think those functions are great! I've put some functions together as well and just made this gist with the code to share them. We definitely implemented things differently, and I will apply them to the example data you included in your repo. Note I have not tested out all of the methods yet as doing a write up of the data I'm applying some of them too took precedence (so it is a work in progress that might have bugs).

@agramfort to get at your question about implementation, one way would be to add methods to the Evoked class. I don't think these can just be added as optional inputs to existing methods (but I may be missing something). I can see three methods to do all of this, which I list below (names of methods are just meant to be descriptive for now).

  1. get_peak: This method already exists, but it does find a global maximum. This might not be ideal in all cases. I would recommend adding an option to find local peaks (using something like scipy.signal.find_peaks). This can be an option that is added. This can return both the peak and
  2. area_amplitude: This is meant to find the mean (or area) amplitude within a given time window. Mean amplitude would be straightforward, whereas area amplitude would use @SimonErnesto approach or the scipy.integrate.trapezoid. The latter is what is used in the ERP Measurement tool and is most similar to the Matlab implementation.
  3. fractional_latencies: This would use the above two methods to return the fractional time points (specified by the user).

Ideally, there would also be some other functions as well to plot the results easily so that the data can be inspected.

@agramfort does this approach fit within the scope/organization framework of MNE-Python development?

Another approach is to make a plugin for MNE python with different classes that could have a .fit() method to apply the computations to an instance/list of evoked objects.

agramfort commented 2 years ago

for now we just a nice tutorial in https://mne.tools/dev/auto_tutorials/evoked/30_eeg_erp.html#amplitude-and-latency-measures and no public functions. I would not support adding more public methods to Evoked but rather have (if more and more people are interested in this) a dedicated module maybe in mne.stats?

Message ID: @.***>

withmywoessner commented 7 months ago

@agramfort @jdkoen I would like to start working on this soon. Do you still suggest creating an entirely new module?

larsoner commented 7 months ago

@withmywoessner sure, I suggest adding a mne/stats/erp/_erp.py and in mne/stats/__init__.py add a from . import erp line, and in mne/stats/erp/__init__.py add a from ._erp import ... line. That should get the ball rolling.

Please start small in terms of actual additions. For example the first PR could add just #10647. Or maybe starting with the four measures you mention in #12406 wouldn't be too onerous to review assuming they are mathematically very simple (and well documented in some paper).

drammock commented 7 months ago

and in mne/stats/erp/__init__.py add a from ._erp import ... line.

nowadays this should go in the __init__.pyi file, not __init__.py. Also anything imported in __init__.pyi should also get added to __all__ variable in that .pyi file.

larsoner commented 7 months ago

Ahh right didn't realize we had lazy-loader-ized mne/stats/!

withmywoessner commented 7 months ago

Hey @drammock @larsoner, do you it would be best to have separate functions for mean amplitude and area? Or should I just include a setting in get_area (or some other name) since mean amp is just the area divided by the interval.

larsoner commented 7 months ago

I'm torn because it doesn't seem useful to have two different functions because their functionalities are so similar, but it's hard to come up with a good name. Maybe get_area with a normalize=None | 'interval' and we say mean amplitude is just get_area(normalize='interval') or something like that.

drammock commented 7 months ago

I think I actually disagree here; we could think of get_mean as a convenience wrapper. It's such a common thing to want, it seems a shame to make people type all of get_area(normalize='interval') just to get the mean.

withmywoessner commented 7 months ago

What about naming it something like get_integral then having a mode parameter mode='mean' | 'positive' | 'negative' | 'absolute' | None (standard integral)

I think I may be leaning more towards one function because it means documentation is in one place. I will ask my coworker who doesn't use Python.

EDIT: I Asked my coworker and he said he didn't care.

larsoner commented 7 months ago

No strong feeling from me, get_mean plus get_area (or a get_integral) is fine with me

withmywoessner commented 7 months ago

Okay, @drammock. I will defer to you if you think it would be better.