[MRG] EDF stim_channels #5841

massich · 2019-01-17T17:13:10Z

This PR allows to use stim_channel again when calling mne.io.read_raw_edf.

Reference issue

Fixes #5738

What does this implement/fix?

Allows to use stim_channel
Updates the deprecation warnings
Updates docstrings

More

What to do with ints
Allow for multiple stim_channels
Warn 'auto' in channel names

Additional information

The test file has been created using this code.
Most probably we want to review all the files we are shipping and modify them to do the testing. Even some of the test files
we are not using them.

massich · 2019-01-17T17:20:42Z

After writting the thest I think that stim_channel should take a list of strings rather than string.
It has 2 advangages:
1 - If I accept a string, then I've to differenciate 'auto' from 'auto' as a really poor ch_name choice.
2 - If we load a bunch of files, they can have different stim names and I can load them with a single call.

and stim_channel=['my name'] is not that bad.

codecov · 2019-01-17T18:08:49Z

Codecov Report

Merging #5841 into master will increase coverage by 0.01%.
The diff coverage is 86.88%.

@@            Coverage Diff             @@
##           master    #5841      +/-   ##
==========================================
+ Coverage   88.63%   88.64%   +0.01%     
==========================================
  Files         373      373              
  Lines       69317    69468     +151     
  Branches    11665    11727      +62     
==========================================
+ Hits        61436    61580     +144     
- Misses       5030     5038       +8     
+ Partials     2851     2850       -1

cbrnr · 2019-01-17T21:20:27Z

I'm -1 on having to supply a list, because at least in my use cases I have only one stim channel. But since we also accept a list, we could resolve this ambiguity by issuing a warning if the user passes 'auto', saying that if the channel is really called 'auto' it must be passed in a list (i.e. ['auto']).

larsoner · 2019-01-17T21:22:31Z

I wouldn't worry about the 'auto' channel use case for now (YAGNI) and let's allow str->list of str automatically, we do this many other places

agramfort · 2019-01-17T21:30:08Z

eog, misc etc. can be lists so stim_channel could be a list and it would make explicit the default valid options that get mapped automatically to stim channels.

larsoner · 2019-01-17T21:50:13Z

eog, misc etc. can be lists so stim_channel could be a list and it would make explicit the default valid options that get mapped automatically to stim channels.

If we do stim_channel=('STATUS', 'Status', ...) default (what I think you are proposing?) there are a couple of drawbacks

we lose the ability to raise an error if something in stim_channel is not actually a channel, unless we special-case the default itself before checking for existence
the signature would become quite long

Neither 'auto' nor ('STATUS', 'Status', ...) default is perfect, and I'm fine with either. I lean +0.5 for 'auto' based on the relative drawbacks of both and what we've tended to do elsewhere. But in either case, I think we should treat stim_channel='foo' the same way as stim_channel=['foo'], i.e. have a if isinstance(stim_channel, str): stim_channel = [stim_channel] in there.

agramfort · 2019-01-17T22:09:12Z

Both work for me. No strong feelings

cbrnr · 2019-01-18T07:38:47Z

I agree that we should treat a string the same way as a list with a single string. But we need to make sure to issue a warning if there is a channel called "auto" in the data. In that case, the only way to specify that channel as the stim channel would be to use its index in the array (i.e. an int), right?

agramfort · 2019-01-18T08:37:46Z

if we allow for list than we can say that ['auto'] works if a channel is called 'auto' but stim_channel='auto' raises an error to suggest to pass a list instead. This way everything can be done.

cbrnr · 2019-01-18T09:47:09Z

That's what I was suggesting but @larsoner wanted to have consistent behavior for this case. I'm fine with anything as long as I can provide stim_channel='auto' (a simple string), which should also be the default behavior.

larsoner · 2019-01-18T12:27:49Z

I'm fine with the probably-never-happening corner case 'auto' channel name needing to be provided as ['auto']

ValueError - The impact of a user parameter is responsible to trigger it.

massich · 2019-01-21T15:27:59Z

This should be green, and do what we said.

At this point, the warning of 'auto' when 'auto' is a channel name is not tested. I don't think its worth covering it. But I'll do it if someone asks for it.

passing a list of integers stim_channel=[0, 3, -1] is not allowed either. I'll put it in if you ask for it.

mne/io/edf/edf.py

cbrnr · 2019-01-22T07:14:28Z

mne/io/edf/edf.py

+    #      the type of channel it is.
+    stim_name_ends_with_annotations = [ch for ch in valid_stim_ch_names
+                                       if ch.endswith('annotations')]
+    if len(stim_name_ends_with_annotations):


When does this evaluate to True? If someone has a channel ending with "annotations"? If so, shouldn't this channel be treated as a stim channel (i.e. set its type to stim and don't synthesize)?

This is true when someone passes 'EDF Annotations' or 'BDF Annotations', in that case the user should take it out from the stim_channel. Therefore the ValueError.

Then, if an annotations channel is present in the file it gets loaded as annotations, and there is no stim channel with such name.

Ah, of course, you are right!

However, this also includes other channels ending with "annotations", which is probably not what we want. I'd exclude only these 2 particular names.

Again, the right thing to do would be to actually get a list of TAL channels when _read_edf_header and raise the error based on that. But at this point, I think that we can do that in a different PR (if we ever get to it).

But sure, we can check for those specific names only.

cbrnr · 2019-01-22T07:17:28Z

mne/io/edf/edf.py

        if stim_channel == 'auto':
-            valid_stim_ch_names = DEFAULT_STIM_CH_NAMES
+            if 'auto' in ch_names:
+                warn(RuntimeWarning, 'Using `stim_channel=\'auto\'` when auto'


You could use a raw string here to avoid the backslashes. Also, the comma in the first sentence should be removed, and there should be a period after the last sentence.

larsoner · 2019-01-22T21:32:24Z

@cbrnr let me know when you are happy, and I'll take a look and merge

cbrnr · 2019-01-23T08:12:21Z

What about passing an int (or a list of ints)? Previously, I liked to do stim_channel=-1, because the stim channel is usually the last one. Does this work again?

massich · 2019-01-23T08:21:08Z

This raises error. I'll add it

…

On Wed, Jan 23, 2019, 09:12 Clemens Brunner ***@***.***> wrote: What about passing an int (or a list of ints)? Previously, I liked to do stim_channel=-1, because the stim channel is usually the last one. Does this work again? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5841 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGt-41Axzsz9NeQZ-1xZ7FSjUifWXDCeks5vGBlmgaJpZM4aGWvf> .

mne/io/edf/edf.py

larsoner

@cbrnr feel free to merge if you're happy

massich · 2019-01-23T15:32:51Z

I fixed a pep8 error that was there. The rest of the CI errors in travis are network timeouts when fetching physionet data.

cbrnr · 2019-01-24T07:26:23Z

Thanks @massich!

agramfort · 2019-01-24T07:45:32Z

cool thanks @massich

can you now backport this? maybe it's a simple as removing the deprecation warning on 0.17 branch and see how what's new needs to be adjusted?

thx

larsoner · 2019-01-24T13:08:58Z

This appears to have broken CircleCI:

...
  File "/home/circleci/project/tutorials/plot_sleep.py", line 90, in <module>
    raw_train.plot(duration=60, scalings='auto')
  File "/home/circleci/project/mne/io/base.py", line 1873, in plot
    event_id=event_id)
  File "/home/circleci/project/mne/viz/raw.py", line 265, in plot_raw
    scalings = _compute_scalings(scalings, raw)
  File "/home/circleci/project/mne/viz/utils.py", line 1634, in _compute_scalings
    data = inst._read_segment(tmin, tmax)
  File "/home/circleci/project/mne/io/base.py", line 561, in _read_segment
    cals, mult)
  File "<string>", line 2, in _read_segment_file
  File "/home/circleci/project/mne/utils.py", line 953, in verbose
    return function(*args, **kwargs)
  File "/home/circleci/project/mne/io/edf/edf.py", line 284, in _read_segment_file
    if ci in stim_channel:
TypeError: argument of type 'NoneType' is not iterable

Can you look into it? It seems like there is some bug, and some use case not caught by our tests (only by an example).

It should be fixed before backporting in #5861

massich · 2019-01-24T14:27:01Z

@larsoner yes I just saw it as well, what I don't understand is why circle CI was not red. I guess that we don't build the entire documentation.

* [Backport 5841] Modify docstring + add test * Remove stim_channel deprecation warning * update whatsnew

massich changed the title ~~EDF stim_channels~~ [WIP] EDF stim_channels Jan 17, 2019

massich added the backport-candidate on-merge: backport to maint/1.11 label Jan 17, 2019

Joan Massich added 5 commits January 19, 2019 14:55

TST: Add stim_channel behaviour

51796cf

TST: update deprecation warning

8b99df8

Fix: stim_channle (single stim)

3f1b05d

fix warnings and errors

1d3d95b

fix: raise the proper error.

1af05bd

ValueError - The impact of a user parameter is responsible to trigger it.

massich force-pushed the edf_stim branch from 17f0f45 to 1af05bd Compare January 20, 2019 16:25

Joan Massich added 6 commits January 20, 2019 18:59

clean up

7ad65fb

read multiple stim_channels

4bf9668

prefer list() to None to simplify the code

3aca164

wip multiple stim channels

243f2f4

fix multi stim channels

a5f892c

clean up + add warning when 'auto' is in ch_names

8694af2

massich changed the title ~~[WIP] EDF stim_channels~~ [MRG] EDF stim_channels Jan 21, 2019

docstring

b229e80

cbrnr reviewed Jan 22, 2019

View reviewed changes

mne/io/edf/edf.py Outdated Show resolved Hide resolved

cbrnr reviewed Jan 22, 2019

View reviewed changes

Joan Massich added 2 commits January 22, 2019 21:29

check TAL ch names with their full name

ebbe92d

maybe this TODO is more clear.

777df47

larsoner added this to the 0.18 milestone Jan 22, 2019

Joan Massich added 2 commits January 23, 2019 10:50

TST / ENH: Parametrize

b6fbc11

fix: add support for int indexing

d520dbd

cbrnr reviewed Jan 23, 2019

View reviewed changes

mne/io/edf/edf.py Outdated Show resolved Hide resolved

Joan Massich added 2 commits January 23, 2019 11:11

ups

bda3857

fix lower-upper case comparison

d45291e

agramfort reviewed Jan 23, 2019

View reviewed changes

mne/io/edf/edf.py Outdated Show resolved Hide resolved

update docstring

6a46354

larsoner approved these changes Jan 23, 2019

View reviewed changes

pep8:

3b04797

cbrnr merged commit 42cf687 into mne-tools:master Jan 24, 2019

massich deleted the edf_stim branch January 24, 2019 09:33

massich pushed a commit that referenced this pull request Jan 24, 2019

[DOC][FIX] update missing docstring in #5841

83d48ec

massich mentioned this pull request Jan 24, 2019

[Backport #5841] EDF stim_channel #5861

Merged

massich mentioned this pull request Jan 24, 2019

regression in plot_sleep.py due to #5841 #5864

Closed

massich mentioned this pull request Jan 24, 2019

[Hot fix] circle ci #5865

Merged

larsoner pushed a commit that referenced this pull request Jan 25, 2019

[Backport #5841] EDF stim_channel (#5861)

647c7d3

* [Backport 5841] Modify docstring + add test * Remove stim_channel deprecation warning * update whatsnew

cbrnr mentioned this pull request Aug 29, 2019

Can't save a slice of a bdf file without preloading full file #6343

Closed

larsoner removed the backport-candidate on-merge: backport to maint/1.11 label Apr 23, 2020

Uh oh!

[MRG] EDF stim_channels #5841

[MRG] EDF stim_channels #5841

Uh oh!

Conversation

massich commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference issue

What does this implement/fix?

More

Additional information

Uh oh!

massich commented Jan 17, 2019

Uh oh!

codecov bot commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cbrnr commented Jan 17, 2019

Uh oh!

larsoner commented Jan 17, 2019

Uh oh!

agramfort commented Jan 17, 2019 via email

Uh oh!

larsoner commented Jan 17, 2019

Uh oh!

agramfort commented Jan 17, 2019 via email

Uh oh!

cbrnr commented Jan 18, 2019

Uh oh!

agramfort commented Jan 18, 2019 via email

Uh oh!

cbrnr commented Jan 18, 2019

Uh oh!

larsoner commented Jan 18, 2019

Uh oh!

massich commented Jan 21, 2019

Uh oh!

Uh oh!

cbrnr Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

massich Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

cbrnr Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

cbrnr Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

massich Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

cbrnr Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

larsoner commented Jan 22, 2019

Uh oh!

cbrnr commented Jan 23, 2019

Uh oh!

massich commented Jan 23, 2019 via email

Uh oh!

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

massich commented Jan 23, 2019

Uh oh!

cbrnr commented Jan 24, 2019

Uh oh!

agramfort commented Jan 24, 2019

Uh oh!

larsoner commented Jan 24, 2019

Uh oh!

massich commented Jan 24, 2019

Uh oh!

Reviewers

Assignees

massich commented Jan 17, 2019 •

edited

Loading

codecov bot commented Jan 17, 2019 •

edited

Loading