Add support for BrainVision ahdr/amrk format, as produced by the V-AMP #10515

Lychfindel · 2022-04-12T09:16:28Z

Reference issue

Example: Fixes #6613

What does this implement/fix?

The PR implements the acquisition of BrainVision data in ahdr format. It also adjust the header regular expression to handle ahdr data saved with V-AMP.

Additional information

The ahdr format introduces a fake channel in the data as a kind of encryption, the rest of channels and unit can be trusted. Therefore, in the submitted code, the data are read considering one more channel, and after the initialization of the BrainVision object, this channel is dropped.

welcome · 2022-04-12T09:16:30Z

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

agramfort

@Lychfindel can you look into the flake8/pep8 errors?

agramfort

@Lychfindel can you add an entry in the file latest.inc to document the enhancement? 🙏

Lychfindel · 2022-04-13T08:34:51Z

Sure, here it is!

sappelhoff

great PR, looks good to me!

Maybe a (personal) nitpick -- but I would appreciate it in the docstr you wouldn't talk about v/amrk and v/ahdr files. This makes searching the code harder/worse and might also be confusing for people who have never heard about amrk / ahdr.

What do others think?

Finally, (trivia question) for vhdr/vmrk I always thought this would mean "vision header" or "vision marker", as abbreviated from "brain vision" (the system by Brain Products, the company) ... was I wrong? Because I don't know what the "a" could possibly stand for in "ahdr" / "amrk"

cbrnr

Other than my nitpicks LGTM!

cbrnr · 2022-04-13T08:54:16Z

mne/annotations.py

+    This function reads a .fif, .fif.gz, .vmrk, .amrk .edf, .txt, .csv .cnt,
+     .cef, or .set file and makes an :class:`mne.Annotations` object.


Suggested change

This function reads a .fif, .fif.gz, .vmrk, .amrk .edf, .txt, .csv .cnt,

.cef, or .set file and makes an :class:`mne.Annotations` object.

This function reads a .fif, .fif.gz, .vmrk, .amrk .edf, .txt, .csv, .cnt,

.cef, or .set file and makes an :class:`mne.Annotations` object.

cbrnr · 2022-04-13T08:56:19Z

mne/io/brainvision/brainvision.py

+        ahdr_format = False
+        if ext == '.ahdr':
+            ahdr_format = True


Suggested change

ahdr_format = False

if ext == '.ahdr':

ahdr_format = True

ahdr_format = True if ext == '.ahdr' else False

mne/io/brainvision/brainvision.py

cbrnr · 2022-04-13T08:58:25Z

mne/io/brainvision/brainvision.py

+    ahdr_format = False
+    if ext == '.ahdr':
+        ahdr_format = True


Same as above.

cbrnr · 2022-04-13T08:58:50Z

mne/io/brainvision/brainvision.py

        ch_names[n] = name
        if resolution == "":
-            if not(unit):  # For truncated vhdrs (e.g. EEGLAB export)
+            if not (unit):  # For truncated vhdrs (e.g. EEGLAB export)


Suggested change

if not (unit): # For truncated vhdrs (e.g. EEGLAB export)

if not unit: # For truncated vhdrs (e.g. EEGLAB export)

cbrnr · 2022-04-13T08:59:14Z

mne/io/brainvision/brainvision.py

+        ch_dict[_AHDR_CHANNEL_NAME] = _AHDR_CHANNEL_NAME
+        ch_names[-1] = _AHDR_CHANNEL_NAME
+        orig_units[_AHDR_CHANNEL_NAME] = 'V'
+        cals[-1] = float(1)


Suggested change

cals[-1] = float(1)

cals[-1] = 1.

Lychfindel · 2022-04-13T09:23:58Z

Thank you for the reviews!
I agree that the naming v/ahdris not good since it makes searching code worse. What do you suggest? Explicitly write vhdr or ahdr?
About naming there is also a problem in the functions like _read_vmrk, _aux_vhdr_info and _get_vhdr_info. I did not change it in order to avoid compatibility problems, but probably it would be more correct to rename these functions in just _read_mrk, _aux_hdr_info and _get_hdr_info.

About the naming I always thought that a stays for analyzer since officially it could be open only with the BrainVision Analyzer software… but it just my guess!

agramfort · 2022-04-13T09:54:55Z

@Lychfindel you need an update here too https://github.com/mne-tools/mne-python/blob/main/mne/io/_read_raw.py#L34

cbrnr · 2022-04-13T10:06:50Z

I agree that the naming v/ahdris not good since it makes searching code worse. What do you suggest? Explicitly write vhdr or ahdr?

I'd write vhdr/ahdr.

About naming there is also a problem in the functions like _read_vmrk, _aux_vhdr_info and _get_vhdr_info. I did not change it in order to avoid compatibility problems, but probably it would be more correct to rename these functions in just _read_mrk, _aux_hdr_info and _get_hdr_info.

Yes, good idea.

Lychfindel · 2022-04-13T12:40:48Z

About naming there is also a problem in the functions like _read_vmrk, _aux_vhdr_info and _get_vhdr_info. I did not change it in order to avoid compatibility problems, but probably it would be more correct to rename these functions in just _read_mrk, _aux_hdr_info and _get_hdr_info.

Yes, good idea.

Ok, then I will rename the functions.
Should I keep an alias of the functions with the old names for backward compatibility, or since they are just internal it is not needed?

cbrnr · 2022-04-13T13:04:24Z

Since these are not public functions you can change their names without keeping the old names around.

…ral hdr/mrk

Lychfindel · 2022-04-13T13:23:44Z

Ok great, I've updated all the names and the docs, except for the arguments of RawBrainVision and read_raw_brainvision since they are public

agramfort · 2022-04-14T20:16:04Z

@Lychfindel can you merge the main branch here and see why CIs complain? 🙏

mne/io/brainvision/tests/data/test_format.ahdr

larsoner

Let's not add a new file to tests/data, see above...

agramfort · 2022-04-21T14:27:27Z

@Lychfindel do you need help here?

Lychfindel · 2022-04-25T14:12:51Z

Sorry @agramfort , last week was pretty busy.
I'm working on it now, but I don't understand how to add a test function that uses the new test set.
Could you help me if I provide you a test set and a test function?

larsoner · 2022-04-25T14:24:15Z

Can you try following the instructions here on how to add your tiny test file(s) to mne-testing-data?

https://github.com/mne-tools/mne-testing-data#add-or-change-files-in-the-repo-for-use-with-mne

Then you should be able to update the tests here to use the files.

larsoner

Just a few tiny things (only one of which is strictly necessary to make CIs happy), otherwise LGTM! Will merge once you push a tiny commit to make CIs happy.

FYI if you want to make all of my suggested changes, on the "Files changed" screen you can "Add suggestion to batch" to all three of them and then commit all at once.

mne/io/brainvision/brainvision.py

mne/io/brainvision/tests/test_brainvision.py

doc/changes/latest.inc

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

agramfort

LGTM

+1 for MRG if CIs are green

thx @Lychfindel

larsoner · 2022-04-26T18:08:45Z

Thanks again @Lychfindel !

Lychfindel · 2022-04-27T09:10:31Z

Thanks to you! Happy to contribute to the package!

* upstream/main: MNT: dock area (mne-tools#10575) MNT: dialog box icon (mne-tools#10573) ENH: Add head_source option (mne-tools#10569) fix for broken subjdir (mne-tools#10574) MAINT: Add export functions to env (mne-tools#10563) Add support for BrainVision ahdr/amrk format, as produced by the V-AMP (mne-tools#10515)

Alessandro Tonin added 3 commits April 11, 2022 17:04

Support for BrainVision ahdr files

f7b15fe

Comments and basic documentation improvement to include ahdr format

1308e5e

Add test for ahdr format

551f320

Lychfindel requested a review from sappelhoff as a code owner April 12, 2022 09:16

agramfort reviewed Apr 12, 2022

View reviewed changes

Fixed flake8 errors

685be04

agramfort reviewed Apr 12, 2022

View reviewed changes

Add enhancement description in doc\changes

efc4168

sappelhoff reviewed Apr 13, 2022

View reviewed changes

cbrnr reviewed Apr 13, 2022

View reviewed changes

Add ahdr/amrk to general read_raw as supported formats

e4756e7

Minor style imporvements

1f76a7e

Name of functions and variables changed from vhdr/vmrk to a more gene…

8a7c051

…ral hdr/mrk

larsoner reviewed Apr 15, 2022

View reviewed changes

mne/io/brainvision/tests/data/test_format.ahdr Outdated Show resolved Hide resolved

larsoner requested changes Apr 15, 2022

View reviewed changes

sappelhoff changed the title ~~Ahdr~~ Add support for BrainVision ahdr/amrk format, as produced by the V-AMP Apr 18, 2022

sappelhoff added the ENH label Apr 18, 2022

sappelhoff added this to the 1.1 milestone Apr 18, 2022

Alessandro Tonin added 3 commits April 19, 2022 10:38

Merge branch 'main' into ahdr

edb9889

Removed tests for ahdr format

0aad94d

Fixed PyFlake error

4cbd79b

Removed double name

61f10fe

Update test using dataset from mne-testing-data

1b5e7e2

larsoner reviewed Apr 26, 2022

View reviewed changes

mne/io/brainvision/brainvision.py Outdated Show resolved Hide resolved

mne/io/brainvision/tests/test_brainvision.py Outdated Show resolved Hide resolved

doc/changes/latest.inc Show resolved Hide resolved

Apply suggestions from code review

f23d682

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

agramfort approved these changes Apr 26, 2022

View reviewed changes

larsoner approved these changes Apr 26, 2022

View reviewed changes

larsoner merged commit c4c100c into mne-tools:main Apr 26, 2022

		This function reads a .fif, .fif.gz, .vmrk, .amrk .edf, .txt, .csv .cnt,
		.cef, or .set file and makes an :class:`mne.Annotations` object.

	if not (unit): # For truncated vhdrs (e.g. EEGLAB export)
	if not unit: # For truncated vhdrs (e.g. EEGLAB export)

Uh oh!

Add support for BrainVision ahdr/amrk format, as produced by the V-AMP #10515

Add support for BrainVision ahdr/amrk format, as produced by the V-AMP #10515

Uh oh!

Conversation

Lychfindel commented Apr 12, 2022

Reference issue

What does this implement/fix?

Additional information

Uh oh!

welcome bot commented Apr 12, 2022

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Lychfindel commented Apr 13, 2022

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

cbrnr left a comment

Choose a reason for hiding this comment

Uh oh!

cbrnr Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

cbrnr Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbrnr Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

cbrnr Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

cbrnr Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

Lychfindel commented Apr 13, 2022

Uh oh!

agramfort commented Apr 13, 2022

Uh oh!

cbrnr commented Apr 13, 2022

Uh oh!

Lychfindel commented Apr 13, 2022

Uh oh!

cbrnr commented Apr 13, 2022

Uh oh!

Lychfindel commented Apr 13, 2022

Uh oh!

agramfort commented Apr 14, 2022

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

agramfort commented Apr 21, 2022

Uh oh!

Lychfindel commented Apr 25, 2022

Uh oh!

larsoner commented Apr 25, 2022

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Apr 26, 2022

Uh oh!

Lychfindel commented Apr 27, 2022

Uh oh!

Reviewers