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

[MRG] Add handling of epochs #45

Merged
merged 22 commits into from
Jul 11, 2019
Merged

[MRG] Add handling of epochs #45

merged 22 commits into from
Jul 11, 2019

Conversation

TyWR
Copy link
Contributor

@TyWR TyWR commented Jul 5, 2019

  • Add an epoching window (to select the events & the length of the interval around the events)
  • Add handling of epochs data type (reading and displaying on mnelab)

This is in reference to #42

There is still a lot to do as I think some functions won't be correctly handing epochs, however it is possible to display the regular plot, the psd_plot, and normally, to compute ICA on epochs.

@cbrnr
Copy link
Owner

cbrnr commented Jul 5, 2019

Can you rebase please?

@TyWR
Copy link
Contributor Author

TyWR commented Jul 5, 2019

@cbrnr Should be ok now :)

mnelab/model.py Outdated Show resolved Hide resolved
mnelab/model.py Show resolved Hide resolved
@cbrnr
Copy link
Owner

cbrnr commented Jul 8, 2019

@TyWR @vferat I've pushed several changes - can you both test it please?

@cbrnr
Copy link
Owner

cbrnr commented Jul 8, 2019

Also @mmagnuski you said you needed epoching, could you also take a look?

@mmagnuski
Copy link

@cbrnr
I am not sure I will be able to test this PR hands-on, so far only the conda installation worked for me.

@hoechenberger
Copy link
Contributor

hoechenberger commented Jul 8, 2019

@mmagnuski

I am not sure I will be able to test this PR hands-on, so far only the conda installation worked for me.

Try:

$ conda create -y -c conda-forge -n mnelab mnelab
$ conda activate mnelab
$ conda remove --force -y mnelab
$ pip install --no-deps https://github.com/fcbg-hnp/mnelab/archive/epochs.zip
$ python -m mnelab

@cbrnr
Copy link
Owner

cbrnr commented Jul 9, 2019

@TyWR I hope it's OK if I add both you and me to the CHANGELOG (since I've changed quite a bit)

@cbrnr cbrnr changed the title Add handling of epochs [MRG] Add handling of epochs Jul 9, 2019
@cbrnr
Copy link
Owner

cbrnr commented Jul 9, 2019

Good to go from my end, I've tested if everything works correctly if an epochs object is active. @TyWR @vferat @mmagnuski let me know if you want to give it another round of review or if you spot anything fishy. Otherwise, I'd like to merge this ASAP.

@TyWR
Copy link
Contributor Author

TyWR commented Jul 9, 2019

For me it's ok !

@mmagnuski
Copy link

@cbrnr I'll review tomorrow as fast as I can! I mostly wanted to try it out in a few ways.

@mmagnuski
Copy link

@hoechenberger Thanks, I'll try that!

@mmagnuski mmagnuski mentioned this pull request Jul 10, 2019
5 tasks
@mmagnuski
Copy link

Works very well for me, great work! I would also add info about tmin and tmax somewhere in the main window data information (it shows Length: 4 x 0.704 s for example, but the tmin and tmax limits would be useful too). But this could be dealt in subsequent PRs.
There is a slight inconsistency in how Plot -> Power spectral density works for raw (all channels as separate lines) and epochs (one line + error), but this is most likely an issue stemming from mne inconsistency.

@mmagnuski
Copy link

After setting a wider baseline than epoching period mnelab closes with the expected error:

Traceback (most recent call last):
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mnelab\mainwindow.py", line 607, in epoch_data
    self.model.epoch_data(events, tmin, tmax, baseline)
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mnelab\model.py", line 38, in wrapper
    f(*args, **kwargs)
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mnelab\model.py", line 499, in epoch_data
    baseline=baseline, preload=True)
  File "<C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\externals\decorator.py:decorator-gen-204>", line 2, in __init__
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\utils\_logging.py", line 89, in wrapper
    return function(*args, **kwargs)
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\epochs.py", line 1812, in __init__
    verbose=verbose)
  File "<C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\externals\decorator.py:decorator-gen-195>", line 2, in __init__
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\utils\_logging.py", line 89, in wrapper
    return function(*args, **kwargs)
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\epochs.py", line 344, in __init__
    _check_baseline(baseline, tmin, tmax, info['sfreq'])
  File "C:\Users\mmagn\Continuum\anaconda3\envs\mnelab\lib\site-packages\mne\epochs.py", line 1598, in _check_baseline
    "data (tmin = %s)" % (baseline_tmin, tmin))
ValueError: Baseline interval (tmin = -0.5) is outside of epoch data (tmin = -0.30000000000000004)

However, I don't think it should close without warning the way it does. It should catch such error and let the user know what went wrong.

@mmagnuski
Copy link

another minor issue is when epoching on file twice (for example with different events or different time limits) - the names in the "data explorer" list on the left in the main window are the same so it may be hard to discern multiple Epochs coming from the same file. But that's also something for another PR.

@cbrnr
Copy link
Owner

cbrnr commented Jul 10, 2019

Thanks @mmagnuski I'll add a check for baseline limits and then do the rest in another PR.

@mmagnuski
Copy link

@cbrnr I was thinking to maybe catch ValueErrors and display in some kind of a warning window. This would:

  • take into account other possible problems like epoch limits going outside data bounds
  • familiarize users with mne error messages

@cbrnr
Copy link
Owner

cbrnr commented Jul 10, 2019

I will do both, because in this case it's pretty straightforward to validate in the dialog.

@cbrnr
Copy link
Owner

cbrnr commented Jul 11, 2019

Regarding the different PSD plots - yes, this is an MNE inconsistency. I don't think there even is an option to plot individual channel PSDs for Epochs...

@cbrnr cbrnr merged commit db7f5f9 into cbrnr:master Jul 11, 2019
@cbrnr
Copy link
Owner

cbrnr commented Jul 11, 2019

Thanks @TyWR!

@cbrnr cbrnr mentioned this pull request Aug 12, 2019
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

5 participants