add curryreader as full dep

[drammock]

PR Description

mne-tools/mne-python#13176 switched MNE-Python to use https://github.com/mne-tools/curry-python-reader/ for reading Neuroscan Curry files. CIs that use MNE main have started failing for not having curryreader installed.

closes #1477 (supersedes)
closes #1478 (supersedes)

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]

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1475   +/-   ##
=======================================
  Coverage   96.94%   96.94%           
=======================================
  Files          43       43           
  Lines       10077    10087   +10     
=======================================
+ Hits         9769     9779   +10     
  Misses        308      308           

☔ 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]

hmm, openneuro is having a bad day, so fetching https://openneuro.org/datasets/ds002778 is failing (causing the build_docs job and CircleCI to both fail). But there's one legitimate failure also: the units column in channels_tsv is ending up as "µV" instead of "V". @dominikwelke if you know the cause off the top of your head, please chime in, otherwise I'll try to dig up the answer tomorrow.

[sappelhoff]

Thanks Dan, please also add new dependencies here: https://github.com/mne-tools/mne-bids/blob/main/doc/install.rst

Other than that, +1 to merge when green

[larsoner]

Opened OpenNeuroOrg/openneuro#3626, TBD whether they have some bug they can fix or openneuro-py needs to be adjusted

[larsoner]

@drammock pushed a commit to document curryreader in doc/install.rst but the uV issue remains

[drammock]

yeah I'm working on the uV issue right now

[drammock]

progress so far: old curry reader code and new code do slightly different things. With https://github.com/mne-tools/mne-testing-data/blob/master/curry/test_bdf_stim_channel%20Curry%208.cdt:

• old reader: each channel in raw.info['chs'] has cal=1e-6, range=1.0, and raw._orig_units = {}. Sets cal directly.
• new reader: each channel in raw.info['chs'] has cal=1.0, range=1.0, and raw._orig_units = {'C3': 'µV', 'C4': 'µV', 'Cz': 'µV'}. Sets raw._cals directly after BaseRaw.__init__ is finished.

the difference in _orig_units is what causes MNE-BIDS to write µV into the "units" column of channels.tsv now (which it didn't before), and that is the direct cause of the test failure. That said, the data arrays end up the same (to within assert_allclose tolerance), and after a round-trip through write_raw_bids (as BrainVision format) and read_raw_bids, raws read with both old and new curry readers end up as cal=0.1, range=1e-6, and _orig_units={'C3': 'µV', 'C4': 'µV', 'Cz': 'µV'}

I think the behavior of the new reader is correct w/r/t _orig_units, because:

1. the curry header does say "uV" in it
2. the curry data (before it's converted to SI units) is on the order of 7000-16000 (plausible as uV for EEG data)

Therefore, I think (?) it is MNE-BIDS that is incorrect here, in that it is writing the channels.tsv "unit" column based on original unit, rather than the unit that is actually being written to disk; using original unit might make sense for cases where the file is not preloaded / only copied / no format conversion occurs, but in this case it's reading Curry and writing BrainVision so IIUC the BrainVision data will be in V (right?)

[dominikwelke]

hi @drammock -
sorry for late reply!

just to confim -
you are correct, my new curry reader sets orig_units to what is written in the file header.
in all the files I saw so far this was either µV or fT. should there be any other unit, this will be copied verbatim with cal=1.0

cals are defined as:

    cals = [
        1.0 / 1e15 if (u == "fT") else 1.0 / 1e6 if (u == "uV") else 1.0 for u in units
    ]

and then set directly via:

    self._cals = np.array(cals)

(before loading any data)

[dominikwelke]

Therefore, I think (?) it is MNE-BIDS that is incorrect here, in that it is writing the channels.tsv "unit" column based on original unit, rather than the unit that is actually being written to disk;

i also looked into the mne-bids code, and yes - this seems to be the case.
unit written to channels.tsv is taken from raw._orig_units, and pybv.write_brainvision is handed units (µV) to allow appropriate scaling but there seems to be no feedback to reflect this unit conversion in the tsv

[drammock]

Therefore, I think (?) it is MNE-BIDS that is incorrect here, in that it is writing the channels.tsv "unit" column based on original unit, rather than the unit that is actually being written to disk;

i also looked into the mne-bids code, and yes - this seems to be the case. unit written to channels.tsv is taken from raw._orig_units, and pybv.write_brainvision is handed units (µV) to allow appropriate scaling but there seems to be no feedback to reflect this unit conversion in the tsv

Thanks for checking @dominikwelke. Looking at _export_mne_raw I see this:

https://github.com/mne-tools/mne-python/blob/1b921f4af5154bad40202d87428a2583ef896a00/mne/export/_brainvision.py#L52-L60

which seems to say that when writing to BV format, units are always converted from V to uV. Therefore, when MNE-BIDS is converting data to BV, the units given in channels.tsv "unit" column should always be uV (which previously they were not). So behavior is correct here, but for the wrong reason (it shouldn't be based on raw._orig_units; that just happens to also be uV in this particular case). I'll open a PR to correct that.

[drammock]

edit for correctness/posterity: mne-bids isn't using mne's export functionality, it is calling pybv directly. But it does the same thing that MNE-Python does (always write V data as uV):

mne-bids/mne_bids/write.py

Lines 1275 to 1283 in 1e0a96e

	# pybv needs to know the units of the data for appropriate scaling
	# get voltage units as micro-volts and all other units "as is"
	unit = []
	for chs in raw.info["chs"]:
	if chs["unit"] == FIFF.FIFF_UNIT_V:
	unit.append("µV")
	else:
	unit.append(_unit2human.get(chs["unit"], "n/a"))
	unit = [u if u not in ["NA"] else "n/a" for u in unit]

[dominikwelke]

also for completeness -
good thing for mne users is that mne_bids does not use the potentially incorrect units in channels.tsv when reading data. it only takes channel name, type and status

so while the BIDS dataset is flawed, any results generated down the line should be fine.

[larsoner]

Just two little things I noticed / ideas

[larsoner]

Couldn't/shouldn't all of these be in a conditional like if fmt == "EEGLAB": pytest.importorskip("eeglabio") and similar? Worth making here if it's a trivial change (and it works) but okay to leave if you aren't motivated to investigate or it doesn't trivially work @drammock

[drammock]

ok @larsoner I've rebased to isolate the semantically unrelated changes into different commits; should be fairly easy to review now by viewing one commit at a time and reading the corresponding commit msgs.

[larsoner]

Nice, thanks @drammock !