Add support for reference channel specification #75

hoechenberger · 2021-06-19T16:44:10Z

Closes #74

pybv/io.py

hoechenberger · 2021-06-20T08:01:17Z

I will extend this to support different references for all channels too, as is permitted by the file format spec.

pybv/io.py

hoechenberger · 2021-06-20T08:38:46Z

Ok I think this is ready for review

sappelhoff

thanks @hoechenberger, this LGTM in general except for the thing with "None=CAR" (see comment on that).

And of course checking against MNE, FT, EEGLAB, Brainstorm ... and BVAnalyzer (which we can hopefully spread across several people)

sappelhoff · 2021-06-21T08:48:57Z

pybv/io.py

+def write_brainvision(*, data, sfreq, ch_names, ref_ch_names=None, fname_base,
+                      folder_out, events=None, resolution=0.1, unit='µV',


honest question: is it good practice to have default args and non-default args intermixed? It seems to work here due to the leading * arg, but it looks a bit odd and without the leading * it would raise a SyntaxError.

honest question: is it good practice to have default args and non-default args intermixed? It seems to work here due to the leading * arg, but it looks a bit odd and without the leading * it would raise a SyntaxError.

I don't know if it's considered good practice, but I think it's one of the big advantages of using kw-only params to be able to do this :) I think it makes a lot of sense to have ref_ch_names directly after ch_names in both the signature and the docstring, even though it's not a required parameter.

agreed, let's do it this way then 👍

I've never seen this either, it will certainly cause some confusion. Is there any alternative other than putting all default args at the end? And yes, because of the * there is no "end" because these are all kw-only args.

Is there any alternative other than putting all default args at the end?

None that wouldn't be even more confusing: assign a default value like None to parameters that need to be set. 🤷‍♂️

pybv/io.py

sappelhoff · 2021-06-21T08:54:39Z

pybv/io.py

+        The name of the channel used as a reference during the recording. If
+        references differed between channels, you may supply a list of
+        reference channel names corresponding to each channel in ``ch_names``.
+        If ``None`` (default), assume the common average reference.


If None (default), assume the common average reference.

I don't think we can do that without breaking compatibility. Specifying None and writing nothing is not the same as the CAR - it just means that the information is not present.

For example, This is the beginning from a VHDR file I recorded using the standard BrainVision equipment, software, and workflow (Reference is on position FCz, but does not occur as a channel in the data):

[Channel Infos] ; Each entry: Ch<Channel number>=<Name>,<Reference channel name>, ; <Resolution in "Unit">,<Unit>, Future extensions.. ; Fields are delimited by commas, some fields might be omitted (empty). ; Commas in channel names are coded as "\1". Ch1=Fp1,,0.1,µV Ch2=Fp2,,0.1,µV Ch3=F7,,0.1,µV

The ref channel is FCz, not a CAR.

Specifying None and writing nothing is not the same as the CAR - it just means that the information is not present.

The specs say otherwise:

But I do see where the misunderstanding comes from, I shouldn't have written "common average" but "reference-free" (although my understanding of looking at the circuits of the ActiCHamp amplifier we had in our old lab was that internally it would reference to the mean across all channels):

In any case, I propose to change the wording to "If None, assumes reference-free recording", WDYT?

This is the beginning from a VHDR file I recorded using the standard BrainVision equipment, software, and workflow (Reference is on position FCz, but does not occur as a channel in the data):

This seems to be violating their own file spec 🙈😱
Unless … I believe in BV Recorder there's the option to do a reference-free recording, BUT still select a reference electrode merely for online data viz. Could that have been the case here?

Good point you make in #74 though, maybe we should reach out to BP for clarification.

I am pretty sure BV recorder doesn't have a "reference free" button 🤔 but yes, there seems to be some sort of conflict :-(

I am going to write to BP support about this and will put you in cc / post the history here.

btw: history is posted in #74

sappelhoff · 2021-06-21T08:56:09Z

pybv/io.py

+        .. note:: The reference channel name specified here does not need to
+                  appear in ``ch_names``; that is, it is okay to specify the
+                  name of a reference channel that was not saved along with
+                  the data.


I agree in principle, but before the next release, I'd like to check what BrainVision analyzer ... and mne/ft/eeglab/... do when they find a ref channel in the VHDR that is not present in the data.

I have some data that looks exactly like this and was recorded with PyCorder in our old lab: for all channels, it specifies REF as the reference, and REF doesn't appear anywhere in the data.

(I believe we simply renamed Cz to REF in the settings)

okay, that's already good to know.

I believe MNE ignores this information anyway? AFAIK the reference is not stored anywhere within the Raw object.

I believe MNE ignores this information anyway? AFAIK the reference is not stored anywhere within the Raw object.

Yep. MNE doesn't do anything with this, as we cannot store the reference anyway.

sappelhoff · 2021-06-21T08:56:39Z

pybv/io.py

@@ -147,6 +157,33 @@ def write_brainvision(*, data, sfreq, ch_names, fname_base, folder_out,
    if len(set(ch_names)) != nchan:
        raise ValueError("Channel names must be unique, found duplicate name.")

+    # Ensure we have a list of strings as reference channel names
+    if ref_ch_names is None:
+        ref_ch_names = [''] * nchan  # common average reference


see my comment above, I don't think we can/should do this.

sappelhoff

I think we should proceed as follows (copied from #74 (comment))

If user passes reference_channel=None (the default), pybv's old behavior is retained: The docstring will say that this corresponds to assuming a reference channel that is common to all channels, but which is not further specified.
If user passes reference_channel=<some channel name that is in the data>, we error if the channel is not all zeros ... else we accept it
If user passes reference_channel=<some channel name that is **NOT** in the data>, we accept it. In future extensions we might think about extending our API so that such cases can be documented in the [Comment] section - but for now I feel that this is overkill.
If a list of reference channels is passed, the entries MUST NOT be None - we simply don't allow it. Otherwise, the rules from above apply

hoechenberger · 2021-06-22T17:14:28Z

Sounds great!

sappelhoff · 2021-07-20T07:10:28Z

@hoechenberger do you want to finish this?

hoechenberger · 2021-07-20T07:40:38Z

Yes … when I have time 😅😅😅
Are you asking because you'd want to take over otherwise? Feel free to move ahead and I can help test & review. Otherwise you'll need to be a little more patient with me 😁

sappelhoff · 2021-07-20T07:45:02Z

Are you asking because you'd want to take over otherwise? Feel free to move ahead and I can help test & review. Otherwise you'll need to be a little more patient with me

no worries, I just intended it as showing that I still think it's worth it 👍 I'll be patient :)

hoechenberger · 2021-07-20T08:54:01Z

I'll be patient :)

Please feel free to ping me again in a few weeks should you not see any activity here!

codecov · 2021-08-07T06:51:35Z

Codecov Report

Merging #75 (f24fb87) into main (f80452c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #75   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          386       421   +35     
=========================================
+ Hits           386       421   +35

Impacted Files	Coverage Δ
pybv/io.py	`100.00% <100.00%> (ø)`
pybv/tests/test_bv_writer.py	`100.00% <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 f80452c...f24fb87. Read the comment docs.

sappelhoff · 2021-08-07T06:54:56Z

to clarify: my last 3 commits were intended only to resolve the conflicts I introduced by making some commits to main in the meantime.

cbrnr · 2021-08-09T08:49:31Z

Can you maybe rebase and force push? That way changes from other PRs (namely #78) won't show up.

sappelhoff · 2021-08-09T09:15:58Z

Can you maybe rebase and force push? That way changes from other PRs (namely #78) won't show up.

I think they aren't 🤔 the diff of this PR is just relatively big itself

see for example this screenshot:

The change from #78 does not show up in the diff

cbrnr · 2021-08-09T09:19:35Z

Maybe I need to get another coffee, I'm pretty sure I was seeing diffs related to that other PR. Anyway, now it seems to work!

sappelhoff

@hoechenberger I took some time to implement my requests from #75 (review)

I think this is ready to be merged now, feel free to check if my commits were in line with your ideas as well.

cbrnr

LGTM!

sappelhoff · 2021-08-14T12:57:53Z

In it goes, thanks for the work @hoechenberger

hoechenberger · 2021-08-17T09:13:28Z

Amazing, thanks @sappelhoff!

Add support for reference channel specification

8a6a864

hoechenberger force-pushed the hoechenberger/issue74 branch from f91beb3 to 8a6a864 Compare June 19, 2021 16:44

hoechenberger requested review from sappelhoff and cbrnr June 19, 2021 16:47

Change param order

d530bfb

hoechenberger commented Jun 19, 2021

View reviewed changes

pybv/io.py Outdated Show resolved Hide resolved

hoechenberger marked this pull request as draft June 20, 2021 08:00

Allow for different reference channels in the same recording

cf0fab8

hoechenberger marked this pull request as ready for review June 20, 2021 08:33

hoechenberger commented Jun 20, 2021

View reviewed changes

pybv/io.py Outdated Show resolved Hide resolved

Update pybv/io.py

7f36a19

hoechenberger commented Jun 20, 2021

View reviewed changes

pybv/io.py Outdated Show resolved Hide resolved

Update pybv/io.py

b2ca6e4

Ref channel does not need to exist in data

66dc229

sappelhoff reviewed Jun 21, 2021

View reviewed changes

sappelhoff added the enhancement New feature or request label Jun 21, 2021

sappelhoff added this to the 0.6.0 milestone Jun 21, 2021

sappelhoff mentioned this pull request Jun 21, 2021

Specify reference channel(s) #74

Closed

strings -> str

319822e

sappelhoff reviewed Jun 22, 2021

View reviewed changes

sappelhoff force-pushed the main branch from e903f61 to e68158d Compare August 6, 2021 08:23

sappelhoff added 2 commits August 7, 2021 08:30

Merge branch 'main' into hoechenberger/issue74

f4643e7

_write_vhdr_file is keyword only now

ae5eada

fix linter

7b483e1

sappelhoff added 5 commits August 10, 2021 16:35

collab

34fab23

update docstr

bf3aeff

fix, enh: check that ref_ch in data is zero

3ae8558

check: do not permit empty strs as ref ch names

18c537b

add test for empty str as ref ch name

f24fb87

sappelhoff approved these changes Aug 10, 2021

View reviewed changes

cbrnr approved these changes Aug 10, 2021

View reviewed changes

sappelhoff merged commit 456d014 into bids-standard:main Aug 14, 2021

hoechenberger deleted the hoechenberger/issue74 branch August 17, 2021 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reference channel specification #75

Add support for reference channel specification #75

hoechenberger commented Jun 19, 2021

hoechenberger commented Jun 20, 2021

hoechenberger commented Jun 20, 2021

sappelhoff left a comment

sappelhoff Jun 21, 2021

hoechenberger Jun 21, 2021 •

edited

sappelhoff Jun 21, 2021

cbrnr Jun 21, 2021

hoechenberger Jun 21, 2021

sappelhoff Jun 21, 2021

hoechenberger Jun 21, 2021 •

edited

hoechenberger Jun 21, 2021

sappelhoff Jun 21, 2021

sappelhoff Jun 21, 2021

sappelhoff Jun 21, 2021

hoechenberger Jun 21, 2021 •

edited

sappelhoff Jun 21, 2021

cbrnr Jun 21, 2021

hoechenberger Jun 21, 2021

sappelhoff Jun 21, 2021

sappelhoff left a comment

hoechenberger commented Jun 22, 2021

sappelhoff commented Jul 20, 2021

hoechenberger commented Jul 20, 2021 •

edited

sappelhoff commented Jul 20, 2021

hoechenberger commented Jul 20, 2021

codecov bot commented Aug 7, 2021 •

edited

sappelhoff commented Aug 7, 2021

cbrnr commented Aug 9, 2021

sappelhoff commented Aug 9, 2021

cbrnr commented Aug 9, 2021

sappelhoff left a comment

cbrnr left a comment

sappelhoff commented Aug 14, 2021

hoechenberger commented Aug 17, 2021

		def write_brainvision(*, data, sfreq, ch_names, ref_ch_names=None, fname_base,
		folder_out, events=None, resolution=0.1, unit='µV',

Add support for reference channel specification #75

Add support for reference channel specification #75

Conversation

hoechenberger commented Jun 19, 2021

hoechenberger commented Jun 20, 2021

hoechenberger commented Jun 20, 2021

sappelhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger Jun 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger Jun 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger Jun 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sappelhoff left a comment

Choose a reason for hiding this comment

hoechenberger commented Jun 22, 2021

sappelhoff commented Jul 20, 2021

hoechenberger commented Jul 20, 2021 • edited

sappelhoff commented Jul 20, 2021

hoechenberger commented Jul 20, 2021

codecov bot commented Aug 7, 2021 • edited

Codecov Report

sappelhoff commented Aug 7, 2021

cbrnr commented Aug 9, 2021

sappelhoff commented Aug 9, 2021

cbrnr commented Aug 9, 2021

sappelhoff left a comment

Choose a reason for hiding this comment

cbrnr left a comment

Choose a reason for hiding this comment

sappelhoff commented Aug 14, 2021

hoechenberger commented Aug 17, 2021

hoechenberger Jun 21, 2021 •

edited

hoechenberger Jun 21, 2021 •

edited

hoechenberger Jun 21, 2021 •

edited

hoechenberger commented Jul 20, 2021 •

edited

codecov bot commented Aug 7, 2021 •

edited