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.

Sign up for GitHub

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

Jump to bottom

FIX : EDF+ Annotation Timestamps missing sub-second accuracy #7875

Merged
merged 5 commits into from
Jun 8, 2020
Merged

FIX : EDF+ Annotation Timestamps missing sub-second accuracy #7875

merged 5 commits into from
Jun 8, 2020

Conversation

agramfort
Copy link
Member

closes #7872

@agramfort agramfort mentioned this pull request Jun 7, 2020
Closed
@agramfort
Copy link
Member Author

@greydongilmore the onsets are now as you expect them but I am wondering if I should not also update the info['meas_date'] as it can contain miliseconds

wdyt?

@greydongilmore
Copy link

This information would be useful to have access to upon heading the EDF+ file header. However, the header block in the EDF+ file will not contain the millisecond value information, and adding it to the info dict would require moving outside the header block to decode the TAL channel. Would this cause an issue for any other EDF methods?

If implemented, a conditional statement could be added based on the EDF type (whether EDF or EDF+). A zero millisecond value could be added to the info['meas_date'] if EDF file is detected, otherwise read the first block of the TAL channel to get the value.

From the EDF spec documentation:

Onset must start with a '+' or a '-' character and specifies the amount of seconds by which the onset of the annotated event follows ('+') or precedes ('-') the startdate/time of the file, that is specified in the header.

So the subsecond part of the starttime must be added to the onset time of the events
at the moment of writing them into the file. The subsecond part of the starttime must be subtracted from the onset time of the events at the moment of reading them from the file.

@agramfort
Copy link
Member Author

agramfort commented Jun 7, 2020 via email

@greydongilmore
Copy link

@agramfort I have tested on a few other files locally and it performs as expected now.

cbrnr
cbrnr reviewed Jun 8, 2020
View reviewed changes
cbrnr
cbrnr reviewed Jun 8, 2020
View reviewed changes
@cbrnr
Copy link
Contributor

cbrnr commented Jun 8, 2020

A test would be nice. I've cropped the file linked by @greydongilmore to 5s (and removed most channels as well as downsampled to 256Hz). If @greydongilmore gives us permission to use this cropped file (17kB) I'd upload it to mne-testing-data so that @agramfort can add a test. There are only 2 annotations in it (XLSpike and Clip Note).

@cbrnr cbrnr changed the title FIX : EDF+ Annotation Timestamps missing submillisecond accuracy FIX : EDF+ Annotation Timestamps missing sub-second accuracy Jun 8, 2020
@agramfort agramfort force-pushed the edf_subms_accuracy_annotations branch from 9239b9d to 649b4d1 Compare June 8, 2020 06:48
@agramfort
Copy link
Member Author

@cbrnr ok for you?

@cbrnr
Copy link
Contributor

cbrnr commented Jun 8, 2020

Yes, perfect! Now let's wait for permission to use the small test file.

@agramfort
Copy link
Member Author

agramfort commented Jun 8, 2020 via email

@agramfort
Copy link
Member Author

agramfort commented Jun 8, 2020 via email

@cbrnr
Copy link
Contributor

cbrnr commented Jun 8, 2020

I cropped the file that @greydongilmore shared with you. It's 17kB and has the sub-second start time.

@agramfort
Copy link
Member Author

agramfort commented Jun 8, 2020 via email

@cbrnr
Copy link
Contributor

cbrnr commented Jun 8, 2020

subsecond_starttime.edf.zip

@agramfort
Copy link
Member Author

agramfort commented Jun 8, 2020 via email

@agramfort
Copy link
Member Author

@cbrnr merge if you're happy

cbrnr
cbrnr reviewed Jun 8, 2020
View reviewed changes
@cbrnr cbrnr merged commit b2615a3 into mne-tools:master Jun 8, 2020
@cbrnr
Copy link
Contributor

cbrnr commented Jun 8, 2020

Thanks @agramfort!

@greydongilmore
Copy link

@cbrnr @agramfort thanks for quick action on this! 👍

@agramfort
Copy link
Member Author

agramfort commented Jun 8, 2020 via email

@greydongilmore greydongilmore mentioned this pull request Jun 8, 2020
Closed
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 17, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into refactor
59db30a 
* upstream/master: (24 commits)
  WIP: Fix Travis (mne-tools#7906)
  WIP: Prototype of notebook viz (screencast) (mne-tools#7758)
  MRG, FIX: Speed up I/O tests, mark some slow (mne-tools#7904)
  Proper attribution for Blender tutorial (mne-tools#7900)
  MAINT: Check usage [ci skip] (mne-tools#7902)
  Allow find_bad_channels_maxwell() to return scores (mne-tools#7845)
  Warn if NIRx directory structure has been modified from original format (mne-tools#7898)
  Pin pvyista to 0.24.3 (mne-tools#7899)
  MRG: Add support for reading and writing sufaces to .obj (mne-tools#7824)
  Fix _auto_topomap_coords docstring. (mne-tools#7895)
  MRG, FIX: Ensure Info H5-writeable (mne-tools#7887)
  Website contents (mne-tools#7889)
  MRG, ENH: Add mri_resolution="sparse" (mne-tools#7888)
  MRG, ENH: Allow disabling FXAA (mne-tools#7877)
  remove "and and" [ci skip] (mne-tools#7882)
  fix evoked nave → inverse guidance (mne-tools#7881)
  ENH: Better error messages (mne-tools#7879)
  FIX : EDF+ Annotation Timestamps missing sub-second accuracy (mne-tools#7875)
  FIX: Fix get_channel_types (mne-tools#7878)
  MRG, BUG: Fix combine evokeds (mne-tools#7869)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbrnr cbrnr cbrnr left review comments

@larsoner larsoner larsoner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EDF+ Annotation Timestamps missing submillisecond accuracy
4 participants
@agramfort @greydongilmore @cbrnr @larsoner