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

[MRG] allow copyfile_brainvision to work with .dat extension #1008

Merged
merged 3 commits into from
May 16, 2022

Conversation

dominikwelke
Copy link
Contributor

PR Description

hi all.
the datafile in brainvision exchange format either has the extension .eeg or .dat.
mne-bids' copyfile_brainvision() function can only handle .eeg, though, and throws an error otherwise.

this PR fixes this.

so far I left the new filenames with .eeg extension, because I didn't find out whether one option is preferred.

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #1008 (fa6e0dd) into main (0984be2) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1008   +/-   ##
=======================================
  Coverage   95.15%   95.15%           
=======================================
  Files          25       25           
  Lines        3778     3778           
=======================================
  Hits         3595     3595           
  Misses        183      183           
Impacted Files Coverage Δ
mne_bids/copyfiles.py 98.07% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0984be2...fa6e0dd. Read the comment docs.

@sappelhoff
Copy link
Member

Hey @dominikwelke I have always wondered where the .dat files are coming from. Do you know?

@dominikwelke dominikwelke force-pushed the fix_brainvision_dat branch from dfe522d to 1efecfa Compare May 16, 2022 10:10
@dominikwelke
Copy link
Contributor Author

dominikwelke commented May 16, 2022

hi @sappelhoff

I know that at least eeglab creates .dat files if you export to brainvision format (also their BIDS tools).

..and I found this potentially outdated brainvision spec sheet on the fieldtrip webpage that mentions .dat files: http://old.fieldtriptoolbox.org/_media/getting_started/brainvisioncorefileformat_1.0_2018-08-02.pdf

@sappelhoff
Copy link
Member

Okay, thanks - we should clarify this with eeglab and brain products then. I'll send them a message.

No objection against this PR though.

And I think unless there is a good reason, eeglab should write .eeg and not .dat

@hoechenberger
Copy link
Member

@dominikwelke Could you please add a changelog entry?
Otherwise this is looking good to me!

@dominikwelke
Copy link
Contributor Author

sure @hoechenberger

bug fix or enhancement?

@dominikwelke dominikwelke changed the title allow copyfile_brainvision to work with .dat extension [MRG] allow copyfile_brainvision to work with .dat extension May 16, 2022
@dominikwelke
Copy link
Contributor Author

decided for fix.
hope I used the link and formatting magic correctly :)

@agramfort agramfort merged commit 772984d into mne-tools:main May 16, 2022
@agramfort
Copy link
Member

thx a lot @dominikwelke

@sappelhoff
Copy link
Member

thanks!

PS: I wrote the email to Brain Products and will update here once they reply. @dominikwelke regarding the "EEGLAB" export as ".dat", did you refer to this software? --> https://github.com/arnodelorme/bva-io

or did you have something else in mind?

I am beginning to think that ".dat" is perhaps the export from "Brain Vision Analyzer"

@dominikwelke
Copy link
Contributor Author

yes, sorry for being unprecise.

yes, the pop_writebva() function from the bva-io plugin stores data in .dat format.
this seems to be the standard plugin, though, if you want to export data in brain vision format.

@sappelhoff
Copy link
Member

okay thanks. I just emailed Arno to clarify this ... in the end it doesn't matter too much, but it's always good to know.

@sappelhoff
Copy link
Member

Here is their answer:

TLDR: .dat is a BrainVision analyzer export format ... I don't think it should be used too much

Dear Stefan,
Thank you for contacting the Scientific Support team. I am glad to help you with the questions.
When recording using our hardware, BrainVision Recorder automatically assigns .eeg as extension for the EEG data file according to the BrainVision Core file format, which has been officially adopted by BIDS-EEG. Among the three important points in compliance with BIDS-EEG standards are:

  • The BrainVision Core file format refers to raw data only (i.e. unprocessed data).
  • The EEG data file format is binary while the metadata (header and marker) are stored as text files.
  • The data file must have one of the following file extensions: “eeg”, “avg” or “seg”.

On the other hand, when exporting data in BrainVision Analyzer, one has the choice in Generic Data Export module - the field specifying the extension is editable and one can decide if one wants to use .dat or .eeg. In addition to Generic Data Export, one could also use Create New Dataset module to export data. But, this creates EEG and metadata files that are not complying to BIDS-EEG standards (i.e., .dat, .xhdr and .xmrk).
So, .dat extension is one of the BrainVision format, it is not outdated, it is simply a freely selectable option for the user when exporting the data, but it does not meet the specification of BIDS-EEG standards.
We hope the provided information clarifies your doubts. But if you have further concerns, please feel free to get back to us.

@hoechenberger
Copy link
Member

So we cannot guarantee that .dat is actually readable by our common BV readers?

@sappelhoff
Copy link
Member

indeed, that's how I read the message. And it reinforces my opinion to handle .dat with care ...

@hoechenberger
Copy link
Member

hoechenberger commented May 17, 2022

My concern is that we're allowing the creation of something that is not BIDS-compliant. We should at least emit a warning, don't you think?
Do we even pass the validator?

@sappelhoff
Copy link
Member

we are still writing as eeg vhdr vmrk with the changes here. We basically simply allow that a "dat" is passed (but it'll be converted to eeg)

A warning wouldn't hurt. And perhaps a quick check whether reading the file with mne is possible.

@hoechenberger
Copy link
Member

Ah ok. Sorry about the confusion then.

@dominikwelke
Copy link
Contributor Author

warning + test makes sense - I can do this if you reopen the PR..

as i wrote above, the context I encounter this in is derivatives (storing EEG after preprocessing pipelines), and from my poking in the BIDS standard, the regulations for derivatives arent fully decided, yet..
(and i think hence MNE-BIDS also doesnt support it yet, right?)

what s your opinion on that? it is processed EEG, but still a continuous full recording. as a user i dont see a reason why it shouldnt be possible to store it in standard BIDS format and then move to derivatives folder, as long as there is no official alternative.

@sappelhoff
Copy link
Member

warning + test makes sense - I can do this if you reopen the PR..

not easily possible :-) if you could submit a fresh follow up PR, that'd be appreciated.

what s your opinion on that? it is processed EEG, but still a continuous full recording. as a user i dont see a reason why it shouldnt be possible to store it in standard BIDS format and then move to derivatives folder, as long as there is no official alternative.

As long as nothing is specified, I think you can put into derivatives/ what you want 👍

@hoechenberger
Copy link
Member

Yes okay, since it's meant for derivatives, maybe we don't even need to warn (yet). No clue. :)

@sappelhoff
Copy link
Member

Yes okay, since it's meant for derivatives, maybe we don't even need to warn (yet). No clue. :)

only for the case when somebody wants to read .dat and write to .eeg for raw bids data 🤷 but yes, if we assert that reading the data is possible (preload=False), then that may be enough of a verification that we don't get really weird data

@dominikwelke dominikwelke deleted the fix_brainvision_dat branch May 24, 2022 07:58
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.

4 participants