Skip to content
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.

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

amended _write_electrodes_tsv to exclude writing stim channels to electrodes.tsv #1023

Merged
merged 14 commits into from
Jul 30, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion mne_bids/dig.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,11 @@ def _write_electrodes_tsv(raw, fname, datatype, overwrite=False):
# create list of channel coordinates and names
x, y, z, names = list(), list(), list(), list()
for ch in raw.info['chs']:
if (
if ch['kind'] == FIFF.FIFFV_STIM_CH:
logger.debug(f"Not writing stim chan {ch['ch_name']} "
f"to electrodes.tsv")
continue
elif (
np.isnan(ch['loc'][:3]).any() or
np.allclose(ch['loc'][:3], 0)
):
Expand Down
35 changes: 35 additions & 0 deletions mne_bids/tests/test_dig.py
Original file line number Diff line number Diff line change
Expand Up @@ -325,3 +325,38 @@ def test_convert_montage():
assert pos['coord_frame'] == 'mri'
assert_almost_equal(pos['ch_pos']['EEG 001'],
[-0.0313669, 0.0540269, 0.0949191])


def test_electrodes_io(tmp_path):
'''test that only channels with associated electrodes were written to
electrodes sidecar.'''
scott-huberty marked this conversation as resolved.
Show resolved Hide resolved
raw = _load_raw()
raw.pick_types(eeg=True, stim=True) # we don't need meg channels
bids_root = tmp_path / 'bids1'
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg')
os.makedirs(op.join(bids_root, 'sub-01', 'ses-01', 'eeg'), exist_ok=True)
scott-huberty marked this conversation as resolved.
Show resolved Hide resolved
write_raw_bids(raw, bids_path)

# test 1
with open(bids_path.directory /
'sub-01_ses-01_acq-01_space-CapTrak_electrodes.tsv') as sidecar:
Copy link
Member

Choose a reason for hiding this comment

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

Should specify encoding='utf-8'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks for the tip!

n_entries = len([line for line in sidecar
if 'name' not in line]) # don't need the header
# only eeg chs w/ electrode pos should be written to electrodes.tsv
assert n_entries == len(raw.copy().pick_types(eeg=True)
.info['ch_names'])
scott-huberty marked this conversation as resolved.
Show resolved Hide resolved

# test 2
with pytest.warns(RuntimeWarning) as record:
read_raw_bids(bids_path)
# list of warnings emitted by read_raw_bids
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate your writing this test (looks like a lot of work!), but I don't think we need this one … we just need to ensure we're writing the correct stuff (which the previous test ensures); testing the reading operation again is, IMHO, overkill. I'd suggest to remove test 2.
@sappelhoff WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was wondering if it was too much ha. I can remove it.

warnings = [record[i].message for i in range(len(record))]

# if the following test fails, then channels without
# associated electrodes were written to electrodes.tsv
# and mne.set_montage will issue a warning about these channels
# during read_raw_bids
if any('There are channels without locations'
in str(message) for message in warnings):
pytest.fail('1 or more channels without associated electrodes were'
' written to electrodes.tsv')