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

[ENH] Added discontinuous datatype for EEG and iEEG #286

Merged
merged 12 commits into from
Aug 28, 2019

Conversation

wouterpotters
Copy link
Contributor

@wouterpotters wouterpotters commented Jul 29, 2019

Problem: EDF+ files (and other file types like BrainVision) can be continuous (EDF+C, BrainVision without discontinuous boundary markers) or discontinuous (EDF+D, BrainVision with discontinuous boundary markers). This can be essential information in processing of data (i.e. not all EDF readers support EDF+D or discontinuous BrainVision data).

See also https://www.edfplus.info/specs/edfplus.html and https://pressrelease.brainproducts.com/markers/#4

Suggestion: mark data as continuous or discontinuous (or epoched)

Note: discontinuous data is not the same as epoched data!

Please let me know what you think!

Added `discontinuous` recordingtype: epoched IS discontinuous (usually), but discontinuous IS NOT epoched.
Added `discontinuous` recordingtype
@robertoostenveld
Copy link
Collaborator

BrainVision files can also include markers that indicate the time of discontinuous boundaries between otherwise continuous segments, e.g. when pausing the recording. I don't think it applies to the other formats.

@robertoostenveld robertoostenveld marked this pull request as ready for review August 22, 2019 07:54
Copy link
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

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

the difference between discontinuous (variable length segments) and epoched (identical length segments) should be explained in the text, not only be mentioned in the table.

@sappelhoff
Copy link
Member

@wouterpotters our linter complains that the table pipe characters in the markdown (|) are not aligned properly, see: https://travis-ci.com/bids-standard/bids-specification/builds/121065179

Can you fix this, please? See here for a Markdown cheat sheet on tables: https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet#tables

Apart from this I agree with @robertoostenveld that a short part added in the text will be helpful.

Updated spacing of table |
Added text on epoched, discontinuous and continuous data.
added text explanation on discontinuous vs. epoched.
@wouterpotters
Copy link
Contributor Author

wouterpotters commented Aug 22, 2019

Dear @robertoostenveld and @sappelhoff, thank you for the useful suggestions and your time. Tried to incorporate all of the suggestions. The checks now pass successfully (didn't check them when I submitted this; will do so for future pull requests).
Do not know about the BrainVision format, but as far as I can tell this pull request could apply to both edf and brainvision without explicitly naming them in the text.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @wouterpotters I am fine with the changes made 👍

One more request: Could you please add yourself to the contributors file? --> https://github.com/bids-standard/bids-specification/blob/master/src/99-appendices/01-contributors.md

We use this list to acknowledge people who have helped the BIDS standard to progress :-)

@wouterpotters
Copy link
Contributor Author

Thanks for the updates @wouterpotters I am fine with the changes made 👍

One more request: Could you please add yourself to the contributors file? --> https://github.com/bids-standard/bids-specification/blob/master/src/99-appendices/01-contributors.md

We use this list to acknowledge people who have helped the BIDS standard to progress :-)

Done.

sappelhoff
sappelhoff previously approved these changes Aug 24, 2019
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

great, I'll leave this open for a bit more to give @robertoostenveld a chance to approve or request changes.

Note: You might also have to rebase this PR (see the conflicting file)

Copy link
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

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

please merge my suggestion, after that it can be included in the release.

@wouterpotters
Copy link
Contributor Author

Included all suggestions by @robertoostenveld and @sappelhoff. Thank you for your time and effort into the BIDS standard. Great initiative!

As far as I can see it's now ready to merge into the main branch.

@sappelhoff
Copy link
Member

Thanks for your contribution @wouterpotters!

@sappelhoff sappelhoff merged commit 0083a5c into bids-standard:master Aug 28, 2019
@wouterpotters wouterpotters deleted the discontinuous_datatype branch November 20, 2019 21:48
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.

3 participants