Skip to content

Conversation

@drammock
Copy link
Member

PR Description

All valid EEG export formats (BrainVision, EDF, and EEGLAB .set) convert V -> µV when writing, but µV wasn't getting written to channels.tsv units column in many cases.

see #1475 (comment)

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

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

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.91%. Comparing base (1e0a96e) to head (b3b19e9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1477      +/-   ##
==========================================
- Coverage   96.94%   96.91%   -0.03%     
==========================================
  Files          43       43              
  Lines       10077    10080       +3     
==========================================
  Hits         9769     9769              
- Misses        308      311       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

drammock added a commit to drammock/mne-bids that referenced this pull request Nov 10, 2025
# 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

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

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

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

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

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

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

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

drammock added a commit to drammock/mne-bids that referenced this pull request Nov 11, 2025
@drammock drammock mentioned this pull request Nov 11, 2025
7 tasks
@drammock drammock deleted the fix-bv-units branch November 13, 2025 20:39
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.

3 participants