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.

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

Jump to bottom

ch_type_mapping has no entry for chpi #1323

Closed
skjerns opened this issue Oct 21, 2024 · 6 comments · Fixed by #1325
Closed

ch_type_mapping has no entry for chpi #1323

skjerns opened this issue Oct 21, 2024 · 6 comments · Fixed by #1325
Labels
bug

Comments

@skjerns
Copy link
Contributor

skjerns commented Oct 21, 2024 •
edited
Loading

Description of the problem

MEGIN systems store chpi information in a channel called 'CHPI00X', which is read by mne as ch_type chpi. However, this entry is missing in the mne to bids conversion dictionary.

This leads to

  File ~/anaconda3/lib/python3.11/site-packages/mne_bids/write.py:150 in _channels_tsv
    ch_type.append(map_chs[_channel_type])

KeyError: 'chpi'

Steps to reproduce

Choose a raw file that contains cHPI channels

In [18]: mne.channel_type(raw.info, 330)
Out[18]: 'chpi'

then try to call write_raw_bids using that file or use the command line interface

write_raw_bids(raw=raw,  # raw containing CHPI MEGIN channels
		bids_path=bids_subj,
		overwrite=True,
		verbose=True)

Solution

chpi should be added to the channel mapping dictionary, probably as MEGOTHER or MISC?

Somewhere after

mne-bids/mne_bids/utils.py

Line 62 in a99fe4d

if fro == "mne" and to == "bids":

Additional information

I'm using mne==1.8.0 and mne_bids=='0.16.0.dev49+ga99fe4d'
@skjerns skjerns added the bug label Oct 21, 2024
@welcome Welcome
Copy link

welcome bot commented Oct 21, 2024

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

@sappelhoff
Copy link
Member

thanks for the report, would you mind elaborating what this means?

MEGIN systems store chpi information

We'd be happy to receive a PR as a fix.

@skjerns
Copy link
Contributor Author

skjerns commented Oct 21, 2024 •
edited
Loading

thanks for the report, would you mind elaborating what this means?

Not sure what you mean, but cHPI is "continuous head position indicators", it's stored in additional MEG channels (usually 5 of them for redundancy), and they get the ch_type=chpi assigned by MNE.

I can do a PR, but I would need to know which ch_type they would get according to MNE-BIDS: MEGOTHER or MISC? And also cannot promise when I can work on it, as writing a test is also required (the fix itself is miniscule)

@sappelhoff
Copy link
Member

Ah thanks!

I would need to know which ch_type they would get according to MNE-BIDS: MEGOTHER or MISC?

If these channels contain (or are primarily used for) brain data, then MEGOTHER, else I would say MISC.

And also cannot promise when I can work on it, as writing a test is also required (the fix itself is miniscule)

No problem, perhaps you could open a PR and push the fix, and then wait with adding the test until you have time.

@skjerns
Copy link
Contributor Author

skjerns commented Oct 21, 2024 •
edited
Loading

Ah, I see that there is also

HLU Measured position of head and head coils, mentioned in the BIDS specs would that be an option?

@sappelhoff
Copy link
Member

Yes, that seems even more appropriate. We just need to get the mapping between user, mne, and bids right.

@skjerns skjerns mentioned this issue Oct 21, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add chpi to known channel list skjerns/mne-bids
2 participants
@sappelhoff @skjerns