Skip to content

fix units in channels.tsv for EEG (& other Volt-based) channels #1477

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

fix units in channels.tsv for EEG (& other Volt-based) channels #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f15ac0f
correct the test (make it fail)
drammock Nov 10, 2025
d36a43b
make it pass
drammock Nov 10, 2025
f186271
improve logic and comment
drammock Nov 10, 2025
f6a8e3a
fix logic: only for BV/EDF/.set
drammock Nov 10, 2025
b3b19e9
changelog
drammock Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/whats_new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ The following authors contributed for the first time. Thank you so much! 🤩
The following authors had contributed before. Thank you for sticking around! 🤘

* `Stefan Appelhoff`_
* `Daniel McCloy`_


Detailed list of changes
Expand Down Expand Up @@ -60,6 +61,7 @@ Detailed list of changes
- Updated MEG/iEEG writers to satisfy the stricter checks in the latest BIDS validator releases: BTi/4D run folders now retain their ``.pdf`` suffix (falling back to the legacy naming when an older validator is detected), KIT marker files encode the run via the ``acq`` entity instead of ``run``, datasets lacking iEEG montages receive placeholder ``electrodes.tsv``/``coordsystem.json`` files, and the ``AssociatedEmptyRoom`` entry stores dataset-relative paths by `Bruno Aristimunha`_ (:gh:`1449`)
- Made the lock helpers skip reference counting when the optional ``filelock`` dependency is missing, preventing spurious ``AttributeError`` crashes during reads, by `Bruno Aristimunha`_ (:gh:`1469`)
- Fixed a bug in :func:`mne_bids.read_raw_bids` that caused it to fail when reading BIDS datasets where the acquisition time was specified in local time rather than UTC only in Windows, by `Bruno Aristimunha`_ (:gh:`1452`)
- Fixed bug in :func:`~mne_bids.write_raw_bids` where incorrect unit was sometimes written into ``channels.tsv`` file when converting data to BrainVision, EDF, or EEGLAB formats, by `Daniel McCloy`_ (:gh:`1477`)

⚕️ Code health
^^^^^^^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion mne_bids/tests/test_write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3431,7 +3431,7 @@ def test_convert_eeg_formats(dir_name, fmt, fname, reader, tmp_path):
# load channels.tsv; the unit should be Volts
channels_fname = bids_output_path.copy().update(suffix="channels", extension=".tsv")
channels_tsv = _from_tsv(channels_fname)
assert channels_tsv["units"][0] == "V"
assert channels_tsv["units"][0] == "µV"

if fmt == "BrainVision":
assert Path(raw2.filenames[0]).suffix == ".eeg"
Expand Down
40 changes: 36 additions & 4 deletions mne_bids/write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def _should_use_bti_pdf_suffix() -> bool:
return use_pdf_suffix


def _channels_tsv(raw, fname, overwrite=False):
def _channels_tsv(raw, fname, *, convert_fmt, overwrite=False):
"""Create a channels.tsv file and save it.

Parameters
Expand All @@ -145,6 +145,10 @@ def _channels_tsv(raw, fname, overwrite=False):
The data as MNE-Python Raw object.
fname : str | mne_bids.BIDSPath
Filename to save the channels.tsv to.
convert_fmt : str | None
Which format the data are being converted to (determines what gets written in
the "unit" column). If ``None`` then we assume no conversion is happening (the
raw data files are being copied from the source tree into the BIDS folder tree).
overwrite : bool
Whether to overwrite the existing file.
Defaults to False.
Expand Down Expand Up @@ -193,11 +197,32 @@ def _channels_tsv(raw, fname, overwrite=False):
ch_type.append(map_chs[_channel_type])
description.append(map_desc[_channel_type])
low_cutoff, high_cutoff = (raw.info["highpass"], raw.info["lowpass"])
if raw._orig_units:
# If data are being *converted*, unit is determined by destination format:
# - `eeglabio.raw.export_set` always converts V to µV, cf:
# https://github.com/jackz314/eeglabio/blob/3961bb29daf082767ea44e7c7d9da2df10971c37/eeglabio/raw.py#L57
#
# - `mne.export._edf_bdf._export_raw_edf_bdf` always converts V to µV, cf:
# https://github.com/mne-tools/mne-python/blob/1b921f4af5154bad40202d87428a2583ef896a00/mne/export/_edf_bdf.py#L61-L63
#
# - `pybv.write_brainvision` converts V to µV by default (and we don't alter that)
# https://github.com/bids-standard/pybv/blob/2832c80ee00d12990a8c79f12c843c0d4ddc825b/pybv/io.py#L40
# https://github.com/mne-tools/mne-bids/blob/1e0a96e132fc904ba856d42beaa9ddddb985f1ed/mne_bids/write.py#L1279-L1280
if convert_fmt:
volt_like = "µV" if convert_fmt in ("BrainVision", "EDF", "EEGLAB") else "V"
Copy link
Contributor

@dominikwelke dominikwelke Nov 11, 2025

Choose a reason for hiding this comment

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

this logic implies that all file formats except bv, edf and eeglab store voltages in V.
is this correct?
or is it rather correct, that all other formats dont do any conversion? then we would still need to access orig_unit here

maybe practically irrelevant if mne_bids only supports these export formats (for eeg they are currently the only formats allowed in BIDS), but couldnt there be MEG formats that store both MEG and EEG in the same file (like curry does)?

Copy link
Member Author

@drammock drammock Nov 11, 2025

Choose a reason for hiding this comment

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

MNE-BIDS doesn't allow exporting eeg, ieeg, seeg, etc to anything other than these 3 formats when eeg, etc are the primary datatype. If there are EEG channels in a file that also contains MEG channels, they should remain as V (unprocessed MEG data must remain in the native format of the instrument; AFAIK no MEG instruments natively save as EDF, .set, or BV). So given that these 3 are the only permitted formats for data where EEG/iEEG/sEEG is the primary datatype, I think the logic is correct.

Copy link
Contributor

@dominikwelke dominikwelke Nov 12, 2025

Choose a reason for hiding this comment

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

ok, I wasnt sure about the BIDS rules for MEG data.
if format conversion is not allowed for combined MEG/EEG files, there shouldn't be a problem, as the code will always access orig_unit.
e.g. in the curry MEG files i saw, original units of recording are fT and µV. if they are not converted, this will be written to channels.tsv

likewise, if ppl kick out all MEG channels to store EEG as a separate modality, they have to pick either of the 3 specified formats.

but if i follow correctly, the else in your line 211 must never be reached then?
i.e. convert_fmt and convert_fmt not in ("BrainVision", "EDF", "EEGLAB") is not allowed.

wouldnt it then be better to write smth like

if convert_fmt:
    assert convert_fmt in ("BrainVision", "EDF", "EEGLAB")
    volt_like = "µV"

or just

if convert_fmt and convert_fmt in ("BrainVision", "EDF", "EEGLAB"):
    volt_like = "µV"

Copy link
Member Author

@drammock drammock Nov 12, 2025

Choose a reason for hiding this comment

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

If user reads MEG data in Curry format and is outputting FIFF then convert_fmt="FIFF" and the else clause is reached, no?

Copy link
Contributor

@dominikwelke dominikwelke Nov 12, 2025

Choose a reason for hiding this comment

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

unprocessed MEG data must remain in the native format of the instrument

i though you said that this is not allowed?

anyway, I'll stop questioning :)
if we are positive that all possible conversion output formats except bv, edf, and eeglab always store V (regardless of original unit), the logic is fine

Copy link
Member Author

@drammock drammock Nov 12, 2025

Choose a reason for hiding this comment

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

Ah yes, you're correct. but there are exceptions, e.g. if anonymization is needed then conversion may happen anyway

Copy link
Member Author

@drammock drammock Nov 12, 2025

Choose a reason for hiding this comment

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

Also the user can request/force conversion in spite of what the bids standard requires

units = [
volt_like
if ch_i["unit"] == FIFF.FIFF_UNIT_V
else _unit2human.get(ch_i["unit"], "n/a")
for ch_i in raw.info["chs"]
]
# if raw data is merely copied, check `raw._orig_units`
elif raw._orig_units:
units = [raw._orig_units.get(ch, "n/a") for ch in raw.ch_names]
# If `raw._orig_units` is missing, assume SI units
else:
units = [_unit2human.get(ch_i["unit"], "n/a") for ch_i in raw.info["chs"]]
units = [u if u not in ["NA"] else "n/a" for u in units]
# fixup "NA" (from `_unit2human`) → "n/a"
units = [u if u not in ["NA"] else "n/a" for u in units]

# Translate from MNE to BIDS unit naming
for idx, mne_unit in enumerate(units):
Expand Down Expand Up @@ -2229,7 +2254,6 @@ def write_raw_bids(
emptyroom_fname=associated_er_path,
overwrite=overwrite,
)
_channels_tsv(raw, channels_path.fpath, overwrite)

# create parent directories if needed
_mkdir_p(os.path.dirname(data_path))
Expand Down Expand Up @@ -2288,6 +2312,14 @@ def write_raw_bids(
f"for {datatype} datatype."
)

# this can't happen until after value of `convert` has been determined
_channels_tsv(
raw,
channels_path.fpath,
convert_fmt=format if convert else None,
overwrite=overwrite,
)

# raise error when trying to copy files (copyfile_*) into same location
# (src == dest, see https://github.com/mne-tools/mne-bids/issues/867)
if (
Expand Down
Loading