FIX: Correctly set the calibration factor in Nihon Kohden reader #13468

myd7349 · 2025-10-28T11:09:27Z

Reference issue (if any)

What does this implement/fix?

Additional information

Considering that M1 and M2 only replace A1 and A2 in certain cases, they will not appear simultaneously. Therefore, when A1 and A2 are already in _default_chan_labels, adding M1 and M2 is not appropriate (they should be placed at the same position in the channel list).

_map_ch_to_specs should not use _default_chan_labels. Instead, it should use the electrode definitions read from the .21E file. If the .21E file does not exist, it can fall back to _default_chan_labels.

myd7349 · 2025-10-28T11:45:39Z

With this patch, the output & plot:

[{'cal': 0.00015625239261476192,
  'ch_name': 'Fp1',
  'coil_type': 1 (FIFFV_COIL_EEG),
  'coord_frame': 4 (FIFFV_COORD_HEAD),
  'kind': 2 (FIFFV_EEG_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 1,
  'range': 6399.902,
  'scanno': 1,
  'unit': 107 (FIFF_UNIT_V),
  'unit_mul': 0 (FIFF_UNITM_NONE)},
 {'cal': 0.00015625239261476192,
  'ch_name': 'Fp2',
  'coil_type': 1 (FIFFV_COIL_EEG),
  'coord_frame': 4 (FIFFV_COORD_HEAD),
  'kind': 2 (FIFFV_EEG_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 2,
  'range': 6399.902,
  'scanno': 2,
  'unit': 107 (FIFF_UNIT_V),
  'unit_mul': 0 (FIFF_UNITM_NONE)},
 {'cal': 0.00015625239261476192,
  'ch_name': 'M1',
  'coil_type': 1 (FIFFV_COIL_EEG),
  'coord_frame': 4 (FIFFV_COORD_HEAD),
  'kind': 2 (FIFFV_EEG_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 21,
  'range': 6399.902,
  'scanno': 21,
  'unit': 107 (FIFF_UNIT_V),
  'unit_mul': 0 (FIFF_UNITM_NONE)},
 {'cal': 0.00015625239261476192,
  'ch_name': 'M2',
  'coil_type': 1 (FIFFV_COIL_EEG),
  'coord_frame': 4 (FIFFV_COORD_HEAD),
  'kind': 2 (FIFFV_EEG_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 22,
  'range': 6399.902,
  'scanno': 22,
  'unit': 107 (FIFF_UNIT_V),
  'unit_mul': 0 (FIFF_UNITM_NONE)}]

larsoner

Any chance this affects some test data file so you could update a test? Ideally the test change would fail if you ran it on main but pass on this PR

doc/changes/dev/13468.bugfix.rst

larsoner · 2025-10-28T13:48:58Z

... and by "some test data file" I mean one in mne-testing-data already. If not then we could add https://github.com/user-attachments/files/23185821/DA00100E.zip to mne-testing-data and then a test could be updated

larsoner · 2025-10-28T13:49:40Z

... there appears to be one failing test already

https://github.com/mne-tools/mne-python/actions/runs/18872986923/job/53855835645?pr=13468#step:19:6018

Can you look?

myd7349 · 2025-10-28T13:51:54Z

... there appears to be one failing test already

https://github.com/mne-tools/mne-python/actions/runs/18872986923/job/53855835645?pr=13468#step:19:6018

Can you look?

Hi! @larsoner Thanks for your review. I am trying to figure out why it failed right now.

myd7349 · 2025-10-28T14:14:42Z

Turns out the test failure is caused by channel $A2 and $A1:

Before:

{'cal': 4.1657189656019924e-05,
  'ch_name': '$A2',
  'coil_type': 0 (FIFFV_COIL_NONE),
  'coord_frame': 0 (FIFFV_COORD_UNKNOWN),
  'kind': 502 (FIFFV_MISC_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 24,
  'range': 24005.46,
  'scanno': 24,
  'unit': -1 (FIFF_UNIT_NONE),
  'unit_mul': 0 (FIFF_UNITM_NONE)},
 {'cal': 4.1657189656019924e-05,
  'ch_name': '$A1',
  'coil_type': 0 (FIFFV_COIL_NONE),
  'coord_frame': 0 (FIFFV_COORD_UNKNOWN),
  'kind': 502 (FIFFV_MISC_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 25,
  'range': 24005.46,
  'scanno': 25,
  'unit': -1 (FIFF_UNIT_NONE),
  'unit_mul': 0 (FIFF_UNITM_NONE)}

After:

 {'cal': 0.00015625239261476192,
  'ch_name': '$A2',
  'coil_type': 0 (FIFFV_COIL_NONE),
  'coord_frame': 0 (FIFFV_COORD_UNKNOWN),
  'kind': 502 (FIFFV_MISC_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 24,
  'range': 6399.902,
  'scanno': 24,
  'unit': -1 (FIFF_UNIT_NONE),
  'unit_mul': 0 (FIFF_UNITM_NONE)},
 {'cal': 0.00015625239261476192,
  'ch_name': '$A1',
  'coil_type': 0 (FIFFV_COIL_NONE),
  'coord_frame': 0 (FIFFV_COORD_UNKNOWN),
  'kind': 502 (FIFFV_MISC_CH),
  'loc': array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan]),
  'logno': 25,
  'range': 6399.902,
  'scanno': 25,
  'unit': -1 (FIFF_UNIT_NONE),
  'unit_mul': 0 (FIFF_UNITM_NONE)}

I will do some debugging to see if I can resolve it.

myd7349 · 2025-10-28T15:37:47Z

OK. I think I’ve fixed the unit test.

In the .21E file, each electrode corresponds to a unique code, which is also how NK identifies electrode types.
The information read from the .21E file would be more appropriately called elec_codes rather than chan_labels, but for consistency with the rest of the codebase, the name chan_labels is used here as well.
In _map_ch_to_specs, this is exactly the information we need.

From the following link, we can see that the electrode codes for $A2 and $A1 are 76 and 77:
https://github.com/mne-tools/mne-testing-data/blob/d30b82782ff45039b106759cc212d0caac657e26/NihonKohden/MB0400FU.21E#L242-L243

And _map_ch_to_specs also includes special handling for 76 and 77:

mne-python/mne/io/nihon/nihon.py

Line 390 in 610ebe5

if (idx < 42 or idx > 73) and idx not in [76, 77]:

A similar logic can also be found in nk2edf — treating 76 and 77 as DC channels.

Before this patch, _default_chan_labels did not contain $A1 and $A2, so they were treated as DC channels, which happened to match the logic in nk2edf.

In this patch, the initial argument passed to _map_ch_to_specs was info["ch_names"], where the indexes of $A1 and $A2 were no longer 76 and 77. As a result, these two channels were treated as EEG channels, causing the unit test to fail.

In the latest commit, by passing the electrode code definitions returned by _read_21e_file to _map_ch_to_specs, $A1 and $A2 are once again correctly handled as DC channels, and thus the unit test now passes.

Some possible improvements:

Instead of:

def _read_nihon_header(fname):
    # Read the Nihon Kohden EEG file header
    fname = _ensure_path(fname)
    _chan_labels = _read_21e_file(fname)
    header = {}
    ...
    return header, _chan_labels

perhaps it would be better to do this:

def _read_nihon_header(fname, _chan_labels):
    # Read the Nihon Kohden EEG file header
    fname = _ensure_path(fname)
    header = {}
    ...
    return header

That is, call _read_21e_file externally and pass its return value to _read_nihon_header and _map_ch_to_specs.

The variable name chan_labels is not very accurate — elec_codes would be more appropriate.
I’m not entirely sure whether treating $A1 and $A2 as DC channels is the correct approach, but both nk2edf and mne currently do so.
In fact, in a MATLAB implementation I have saw, 76 and 77 are not treated specially. I will perform further verification in the future.
If needed, I can also add another unit test.

larsoner · 2025-10-28T15:54:25Z

Feel free to implement whichever of the enhancements above you think will make the code more understandable next time we need to change/fix it

For the question about which channels to handle as DC, do you have any test files where you could load them in some NK software and look at them, like you did for M1 and M2 in the top comment?

myd7349 · 2025-10-28T16:06:50Z

Hi! @larsoner
I’ll write a script later to go through the EEG files I have and check which ones contain DC channels. I’ll let you know if there’s any new progress.

myd7349 · 2025-10-28T16:57:28Z

Hi @larsoner I have just created a PR: mne-tools/mne-testing-data#126 .

myd7349 · 2025-10-29T15:21:23Z

Hi! @larsoner I have just added a new test to reproduce #13467 .

I will push the fix after all CI jobs finished.

If the .21E file does not exist, issue a warning. Co-authored-by: Eric Larson <[email protected]>

for more information, see https://pre-commit.ci

myd7349 · 2025-10-29T16:09:32Z

OK. I’ve made another commit to indicate that it can fix this issue. In addition, since the .21E file contains the correct electrode definitions, I think a warning should be issued when this file is missing.

larsoner

Just a couple of small things, otherwise LGTM!

mne/io/nihon/tests/test_nihon.py

mne/io/nihon/nihon.py

Co-authored-by: Eric Larson <[email protected]>

larsoner

Looks good, marking for merge-when-green. Thanks in advance @myd7349 !

* upstream/main: (230 commits) FIX: Fix ICA.apply when fitted including marked bad channels (mne-tools#13478) FIX: Correctly set the calibration factor in Nihon Kohden reader (mne-tools#13468) [pre-commit.ci] pre-commit autoupdate (mne-tools#13479) MAINT: Update code credit (mne-tools#13477) Fix `versionadded` directive formatting (mne-tools#13471) typo in mailmap (mne-tools#13475) FIX: Fix _plot_topomap channel names plotting when using a mask (mne-tools#13470) FIX: Handle an Eyelink File with blank lines injected throughout file (mne-tools#13469) ENH: adds annotation filtering to raw and ica source figures (mne-tools#13460) MAINT: Restore VTK nightly wheel on Linux and bump changelog checker (mne-tools#13436) [pre-commit.ci] pre-commit autoupdate (mne-tools#13465) FIX: Fix add_reference_channels for passing two channels names (mne-tools#13466) ENH: Add on_missing for combine_channels (mne-tools#13463) Bump the actions group with 2 updates (mne-tools#13464) Move development dependencies into a dependency group (no more extra) (mne-tools#13452) ENH: add on_missing for rename_channels (mne-tools#13456) add advisory board to website (mne-tools#13462) ENH: Support Nihon Kohden EEG-1200A V01.00 (mne-tools#13448) MAINT: Update dependency specifiers (mne-tools#13459) ENH: Add encoding parameter to Nihon Kohden reader (mne-tools#13458) ...

* upstream/main: (85 commits) FIX: Fix ICA.apply when fitted including marked bad channels (mne-tools#13478) FIX: Correctly set the calibration factor in Nihon Kohden reader (mne-tools#13468) [pre-commit.ci] pre-commit autoupdate (mne-tools#13479) MAINT: Update code credit (mne-tools#13477) Fix `versionadded` directive formatting (mne-tools#13471) typo in mailmap (mne-tools#13475) FIX: Fix _plot_topomap channel names plotting when using a mask (mne-tools#13470) FIX: Handle an Eyelink File with blank lines injected throughout file (mne-tools#13469) ENH: adds annotation filtering to raw and ica source figures (mne-tools#13460) MAINT: Restore VTK nightly wheel on Linux and bump changelog checker (mne-tools#13436) [pre-commit.ci] pre-commit autoupdate (mne-tools#13465) FIX: Fix add_reference_channels for passing two channels names (mne-tools#13466) ENH: Add on_missing for combine_channels (mne-tools#13463) Bump the actions group with 2 updates (mne-tools#13464) Move development dependencies into a dependency group (no more extra) (mne-tools#13452) ENH: add on_missing for rename_channels (mne-tools#13456) add advisory board to website (mne-tools#13462) ENH: Support Nihon Kohden EEG-1200A V01.00 (mne-tools#13448) MAINT: Update dependency specifiers (mne-tools#13459) ENH: Add encoding parameter to Nihon Kohden reader (mne-tools#13458) ...

myd7349 force-pushed the fix-nk-cal-factor branch from 402e662 to f7cc86f Compare October 28, 2025 11:17

myd7349 requested review from agramfort, dengemann, drammock and larsoner as code owners October 28, 2025 11:17

larsoner reviewed Oct 28, 2025

View reviewed changes

doc/changes/dev/13468.bugfix.rst Outdated Show resolved Hide resolved

TST: Add test to reproduce mne-tools#13467

586eb7f

myd7349 force-pushed the fix-nk-cal-factor branch from 4645a70 to 586eb7f Compare October 29, 2025 14:42

myd7349 and others added 2 commits October 29, 2025 23:59

FIX: Correctly set the calibration factor in Nihon Kohden reader

b0d51c7

If the .21E file does not exist, issue a warning. Co-authored-by: Eric Larson <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

b5acb91

for more information, see https://pre-commit.ci

larsoner reviewed Oct 29, 2025

View reviewed changes

mne/io/nihon/tests/test_nihon.py Outdated Show resolved Hide resolved

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

myd7349 and others added 2 commits October 30, 2025 00:29

Update mne/io/nihon/tests/test_nihon.py

4045778

Co-authored-by: Eric Larson <[email protected]>

Update nihon.py

8c87abe

larsoner approved these changes Nov 4, 2025

View reviewed changes

larsoner merged commit f443d1c into mne-tools:main Nov 4, 2025
32 checks passed

myd7349 deleted the fix-nk-cal-factor branch November 4, 2025 18:11

Uh oh!

FIX: Correctly set the calibration factor in Nihon Kohden reader #13468

FIX: Correctly set the calibration factor in Nihon Kohden reader #13468

Uh oh!

Conversation

myd7349 commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference issue (if any)

What does this implement/fix?

Additional information

Uh oh!

myd7349 commented Oct 28, 2025

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larsoner commented Oct 28, 2025

Uh oh!

larsoner commented Oct 28, 2025

Uh oh!

myd7349 commented Oct 28, 2025

Uh oh!

myd7349 commented Oct 28, 2025

Uh oh!

myd7349 commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Oct 28, 2025

Uh oh!

myd7349 commented Oct 28, 2025

Uh oh!

myd7349 commented Oct 28, 2025

Uh oh!

myd7349 commented Oct 29, 2025

Uh oh!

myd7349 commented Oct 29, 2025

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

myd7349 commented Oct 28, 2025 •

edited

Loading

myd7349 commented Oct 28, 2025 •

edited

Loading