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

FIX/ENH: loading EDF annotations #1234

Merged
merged 25 commits into from
May 1, 2014

Conversation

mbillingr
Copy link
Contributor

I made sure that the stim channel does not get resampled, and that the int16 values (each containing two characters) are not clamped.

Parsing the stim channel to get events of the form [onset, duration, 'annotation'] remains to be done.

I'm not sure where to store these events, though. Currently I consider not to store the events at all. Instead, a member function would simply parse and return the event list. Any thoughts?

@agramfort
Copy link
Member

@t3on @christianbrodbeck I guess this is for you :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling a83f0c9 on kazemakase:fix-edf-annotations into 9a88a37 on mne-tools:master.

@mbillingr
Copy link
Contributor Author

I've added a parser as a private member of RawEDF. Not sure how to expose the functionality, though.
What is the correct format of the event list?

BTW, how do you get coveralls to automatically comment on PRs? :)

@agramfort
Copy link
Member

we need the help of our EDF experts to review :) ping @t3on @christianbrodbeck

I have no experience with this format...

in the MNE way events are used to generate a stim channel that can be parsed with find_events function

@mbillingr
Copy link
Contributor Author

in the MNE way events are used to generate a stim channel that can be parsed with find_events function

In that case I might have got it wrong. I thought the stim channel was specific to EDF.
Maybe it is necessary to convert the EDF annotation channel to an MNE compatible stim channel?

@agramfort
Copy link
Member

In that case I might have got it wrong. I thought the stim channel was
specific to EDF.
Maybe it is necessary to convert the EDF annotation channel to an MNE
compatible stim channel?

isn't that the case already?

find_events works on an EDF raw?

@mbillingr
Copy link
Contributor Author

It does not work on data where events are stored as EDF+ annotations: http://www.edfplus.info/specs/edfplus.html#edfplusannotations

@larsoner
Copy link
Member

@kazemakase Coveralls will automatically comment once the build is passing (it's currently not). But it's actually a bit better to rely on your own local nose/coverage output, though, since Travis only runs a subset of the full test suite.

@christianbrodbeck
Copy link
Member

@t3on ? I've never worked with EDF either...

@mbillingr
Copy link
Contributor Author

@eric89gxl I was just wondering because i can't get coveralls to comment on PRs in the SCoT repo even when the build is passing.

@larsoner
Copy link
Member

Ahh, for a different repo... you have to enable it on coveralls.io somewhere.

Assuming you've seen that option and enabled it, you also have to have .travis.yml configured properly. For some reason some of the repos I work on it works properly, and others it doesn't. I haven't tracked down what subtle differences between the repos cause some to get commented on, and others not. It appears to work for this repo, so you could try tracking down the differences between your travis config and ours.

@mbillingr
Copy link
Contributor Author

Yep, that option is enabled and coveralls receives coverage information from travis. Good to know it doesn't work everywhere. Maybe it doesn't like capital letters in the repo name? :) I'll have another look at .travis.yml.

@teonbrooks
Copy link
Member

sorry, just got notified. my email was silent on this ;) I am not clear on what the issue is. there is a private function, _read_annot, that takes care of the annotation file with the format described in the MNE manual. if the triggering is stored in the data, it is also capable of parsing it with the specification of the stim channel.

@mbillingr
Copy link
Contributor Author

@t3on I assume you refer to the stim channel and event files as described here: http://martinos.org/mne/stable/manual/browse.html#events-and-annotations

Everything is fine if the EDF files in question were created according to that specification. However, I am trying to load data recorded by someone who never even heard of MNE. There is no stim channel or annotation file. Instead, there is a channel that contains ascii encoded tags (see my link above), which the current implementation is unable to read correctly.

@@ -234,7 +234,12 @@ def _read_segment(self, start=0, stop=None, sel=None, verbose=None,
for i, samp in enumerate(n_samps):
chan_data = data[i::n_chan]
chan_data = np.hstack(chan_data)
if samp != max_samp:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be deleted. this is used for when the sampling across different channels aren't unified. it does a resampling of the lower sampled channels to match the max sampling rate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not delete it entirely (see line 242). Instead, I added separate handling of the annotation channel (which I thought was stim_channel).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, sorry, my mistake, didn't see the elif.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, what happens if you resample the stim channel (assuming it is a correct MNE stim channel)? Wouldn't the implicit filter hurt it?

@teonbrooks
Copy link
Member

Instead, there is a channel that contains ascii encoded tags (see my link above), which the current implementation is unable to read correctly.

do you have an example of a file that has the text annotation as channel in the data?

@mbillingr
Copy link
Contributor Author

Sure, I was working with that file: http://www.physionet.org/physiobank/database/eegmmidb/S001/S001R06.edf

@mbillingr mbillingr changed the title FIX: loading EDF annotations FIX/ENH: loading EDF annotations Apr 26, 2014
@mbillingr
Copy link
Contributor Author

If i got this right, MNE expects a certain stim_channel where events are encoded. There are two ways to obtain a stim_channel for an edf file:
a) it is stored in the edf
b) an annotation file is parsed that replaces the stim channel

I have added a parameter tal_channel which can be used to specify an EDF+ annotation channel. I plan to create the stim_channel from the tal_channel if no annotation file is specified.

Parsing the tal channel works already, but how are events encoded in the stim channel?

@agramfort
Copy link
Member

Parsing the tal channel works already, but how are events encoded in the stim channel?

Have a look at the find_events function for example

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4a96214 on kazemakase:fix-edf-annotations into 9a88a37 on mne-tools:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) when pulling 9c25cf7 on kazemakase:fix-edf-annotations into 9a88a37 on mne-tools:master.

@mbillingr
Copy link
Contributor Author

Is there a way to encode simultaneous events in the stim channel?

@agramfort
Copy link
Member

No. You need unique events

@larsoner
Copy link
Member

If you need to, you can conceptually do something like this using a binary representation. Each event number n becomes encoded as 2 ** (n - 1). So event type 3 is encoded as the number 4, while event number 5 is encoded as the number 16. Thus simultaneous 3 and 5 events would be encoded as 20 (their sum). It's not too tough to synthesize this representation, or use bit masking to get back the original values.

If this is too annoying, you can also include multiple stim channels in a FIFF file, they just need different channel names. IIRC usually STI101 is just a binary combination (as above) of several other channels (which only take on values of zero or one) named something like STI001, STI002 or so.

# don't resample tal_channel,
# pad with zeros instead.
chan_data = np.hstack([chan_data,
[0]*int(max_samp-samp)*blocks])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespaces missing between operators

@sawradip sawradip mentioned this pull request Feb 16, 2023
@MohammadAsiya MohammadAsiya mentioned this pull request May 5, 2023
@JD-Zhu JD-Zhu mentioned this pull request Oct 12, 2023
11 tasks
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.

9 participants