inconsistencies in edf reader and annotation readers

[jasmainak]

I have a couple of issues here but bunching them together as they seem related.

1. EDF reader should work with preload=False

If I do:

import mne
from mne.datasets import eegbci
edf_path = eegbci.load_data(subject=1, runs=2)[0]
raw = mne.io.read_raw_edf(edf_path, preload=False)

I get

RuntimeError: EDF+ Annotations (TAL) channel needs to be parsed completely on loading. You must set preload parameter to True.

However, I can actually do:

import mne
from mne.datasets import eegbci
from mne.io.edf.edf import read_annotations_edf

edf_path = eegbci.load_data(subject=1, runs=2)[0]
raw = mne.io.read_raw_edf(edf_path, preload=False, stim_channel=None)

annot = read_annotations_edf(edf_path)
raw.set_annotations(annot)

and it gives me the same result as:

raw2 = mne.io.read_raw_edf(edf_path, preload=True)
assert raw.annotations.duration[0] == raw2.annotations.duration[0]

So, it seems to me that it should already be possible to load EDF files with preload=False as all the necessary components already exist.

2. find_edf_events and mne.events_from_annotations don't give same results

Here is the code to reproduce (continuing from code above):

events, event_id = mne.events_from_annotations(raw)
events2 = raw.find_edf_events()
print(events)
print(events2)

3. namespaces of read_annotations_xxx

I have to do:

from mne import read_annotations  # for fif files
from mne.io import read_annotations_brainvision  # for brainvision
from mne.io import read_annotations_eeglab  # for eeglab
from mne.io.edf.edf import read_annotations_edf  # for edf

This is quite confusing. In fact, I don't see a good rational for having different functions as all these different file formats (debatable) since all you need is a filename for most of these readers. So you should be able to triage based on the extension. In any case, having them in the same namespace would already be a start ! (and maybe read_annotations -> read_annotations_fif ?)

4. documentation issues

read_annotations_edf says I need a .vmrk file. This looks incorrect, right?

Happy to help out with these after the mne-bids release. If someone else has time to work before (@massich ?), I am happy to review the code :)

[agramfort]

thanks heaps @jasmainak for the details report. Yes @massich should be the first person to start addressing these issues.

Just regarding 2: raw.find_edf_events() should be deprecated unless it gives you something you cannot do with events_from_annotations

regarding 4:
yes it should be from mne.io import read_annotations_edf and I agree that read_annotations should / could triage based on file extension, especially since we'll soon have IO for CSV files.

[cbrnr]

These are all excellent points. @massich I'd be happy to help address these issues. Regarding 3, I've brought this up once for the loading functions (read_raw_XXX), but most people didn't like the idea. I still do, having one function to load the data and one function to read annotations seems much simpler, given that we can determine the file type by its extensions (or if that doesn't work we can still parse the magic bytes).

[cbrnr]

Re 2, actually raw.find_edf_events gives you back the annotations as they are stored in the EDF file (no stim chan is synthesized). It is deprecated because all the information is contained in raw.annotations. I don't know the plan about what is going to happen with events - will mne.find_events still be an option? Or will it be replaced by mne.events_from_annotations? In this example, they contain the same events, only in a different format. Is that what you meant by different?

[agramfort]

Re 2, actually raw.find_edf_events gives you back the annotations as they are stored in the EDF file (no stim chan is synthesized). It is deprecated because all the information is contained in raw.annotations. I don't know the plan about what is going to happen with events - will mne.find_events still be an option? Or will it be replaced by mne.events_from_annotations? In this example, they contain the same events, only in a different format. Is that what you meant by different?

yes the plan is to keep mne.find_events for raw with a stim channel and use events_from_annotations otherwise. we want to get rid of raw vendors peculiarities.

[massich]

re 1

Yes, that's expected. What it could be good is to set stim_channel=None as default. And warn. But I'm not sure how this plays in mne-python's timeline for deprecationg stim channel.

The other thing, is that you cannot read the TAL channel in the edf_file on preload=False unless you parse the entire file. So I'm not sure. If you supply a file with only annotations, then it would make sense.

re 2

this is weird, I thought we had a test for that.

re 3

I also dislike this _XXXX in the readers. But for now I would not touch it.

re 4

my bad.

[jasmainak]

The other thing, is that you cannot read the TAL channel in the edf_file on preload=False unless you parse the entire file. So I'm not sure. If you supply a file with only annotations, then it would make sense.

If you know which memory segments to access for the TAL channel, wouldn't this be fast? How fast is read_annotations_edf for really long recordings like say, sleep data?

[massich]

If you know which memory segments to access for the TAL channel, wouldn't this be fast? How fast is read_annotations_edf for really long recordings like say, sleep data?

yes it should be plenty doable. The only thing is that this was not present in the EDF reader, and we did not implement it.

[agramfort]

agreed regarding TAL channel parsing without preload it should be doable but it's presently not implemented. To be done at some point. No rush on my side.

Regarding 4 as discussed with @massich I would suggest to:

• make private : read_annotations_brainvision read_annotations_eeglab read_annotations_edf

• add triage to the private functions based on file extension for mne.read_annotations

[larsoner]

FYI to me it is weird to have a read_annotations that triages by file type but no read_raw that does the same. The more consistent fix would be to have read_annotations renamed to read_annotations_fif. But I can live with the inconsistency.

[agramfort]

well for me it's much simpler in terms of API to have read_annotations. We don't do it for raw objects because we have a lot of custom parameters and a single read_raw would end up with 12 parameters or more.

[larsoner]

I suspect that after this annot refactoring we should have many fewer parameters

[massich]

Maybe I've merged it too soon. I did not see that the conversation was still alive.