follow up: allow copyfile_brainvision to work with .dat extension

[dominikwelke]

PR Description

this PR asserts that brainvision files can be read by MNE
..raises a warning if .dat file extension is used instead of .eeg in brainvision format

follow up for #1008

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>"

[dominikwelke]

hi @sappelhoff @hoechenberger
here is a draft for the requested assertion.

if you prefer some other place let me know.
e.g. it could also become a higher level assertion applied to all files (not only brainvision)

I am not convinced though, that this will catch all cases. for example when writing the code i renamed a random .json file to replace the binary .eeg file in an example dataset, and mne read if without any errors

..and i didnt raise a warning so far, let me know if you want that

[codecov]

Codecov Report

Merging #1010 (990034e) into main (fa7a94b) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1010      +/-   ##
==========================================
- Coverage   95.20%   95.17%   -0.03%     
==========================================
  Files          25       25              
  Lines        3772     3774       +2     
==========================================
+ Hits         3591     3592       +1     
- Misses        181      182       +1     

Impacted Files	Coverage Δ
mne_bids/copyfiles.py	97.61% <50.00%> (-0.46%)	⬇️

---

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 fa7a94b...990034e. Read the comment docs.

[sappelhoff]

The .eeg file remains untouched by MNE, if you pass preload=False --> so this is to be expected in this case.

I think this check is good to have, it will give us some safety that at least the vhdr and vmrk can be properly read.

I think a warning if ".dat" is copied may be good -- especially because we are rewriting it as ".eeg", right? And then maybe some info along the lines: ".dat" is not necessarily raw data, please double check that everything is fine.

not sure if we should add this to all data formats -- but for BrainVision we have a good reason to do so IMHO

thanks!

[dominikwelke]

The .eeg file remains untouched by MNE, if you pass preload=False --> so this is to be expected in this case.

no, I tried with preload=True

[sappelhoff]

oh, that's weird, did you also rename the file references in VHDR and VMRK (what bv_copyfile does)?

[dominikwelke]

yes, weird.. you can reproduce if you want :)

Archive.zip
(it's simply the *_eeg.json renamed to *_eeg.eeg)

raw.get_data() returns an array with one datapoint per channel.
this is obviously a problem of base MNE, if a problem at all (in the end one should trust the users that they provide actual data, and dont simply rename random files 😃 )

in this context it's relevant, though:
if we dont really assert the integrity of the data, i dont see the point in including an assertion at all.. basically the way it is (preload=False) only checks if the header is meaningfull, but not the whole set..

what do you think?

[sappelhoff]

wow, I can reproduce -- that's crazy. probably because MNE just opens that json file as if it was binary and is then reading those bytes interpreting them as numbers, or something like it?

Maybe not an important problem, but something that would be nice to have fixed with a good error message. 🤷

I think you are right then, we might as well close this PR and hope for the best / wait for user reports if people run into issues. (I hope not)

[dominikwelke]

wow, I can reproduce -- that's crazy. probably because MNE just opens that json file as if it was binary and is then reading those bytes interpreting them as numbers, or something like it?

this is what i thought, too.
i didnt do a lot of testing with other files.. not surprisingly an empty textfile renamed to .eeg doesnt work. (some general error along "couldnt read line 1")

Maybe not an important problem, but something that would be nice to have fixed with a good error message. 🤷

I'll create an issue in the MNE repo soon, at least people are aware then
not sure whether it's really possible to catch this, though.

I think you are right then, we might as well close this PR and hope for the best / wait for user reports if people run into issues. (I hope not)

ok with me..
but actually I only meant the assertion. raising a warning if .dat is provided would still make sense IMO (high likelihood that the files have been manipulated, and might not adhere to BIDS standards). i didnt have much time last week, but will create a commit soon

[dominikwelke]

i changed it to the warning @sappelhoff

dont think the ubuntu CIs are unhappy due to my PR?

[sappelhoff]

yes I think these issues are due to an issue in pybv that I just now fixed with a new release.

[sappelhoff]

thanks!