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

include OPM channels as megmag under .pick/get_coil_types() #1222

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

AmaiaBA
Copy link
Contributor

@AmaiaBA AmaiaBA commented Jan 31, 2024

PR Description

Description:
This pull request addresses an issue encountered when using the write_raw_bids() command for OPM (Optically Pumped Magnetometer) data. The problem arises in the _channels_tsv() function, which throws a KeyError when attempting to access a key in the map_chs dictionary.


Error Details:

File ~/miniconda3/envs/mne/lib/python3.11/site-packages/mne_bids/write.py:157, in _channels_tsv(raw, fname, overwrite)
    155     if _channel_type in get_specific:
    156         _channel_type = coil_type(raw.info, idx, _channel_type)
--> 157     ch_type.append(map_chs[_channel_type])
    158     description.append(map_desc[_channel_type])
    159 low_cutoff, high_cutoff = (raw.info["highpass"], raw.info["lowpass"])

KeyError: 'mag'

Issue Explanation:
The error occurs because the key _channel_type is not present in the map_chs dictionary. Specifically, when using any of the supported channel types for OPMs, _channel_type is set to 'mag' instead of 'megmag'. This discrepancy is due to the fact that get_coil_types() under mne_bids/pick.py does not include OPM channels.


Solution:
To resolve this issue, I expanded the list of supported OPM channels in get_coil_types():

megmag=(
            FIFF.FIFFV_COIL_POINT_MAGNETOMETER,
            FIFF.FIFFV_COIL_VV_MAG_W,
            FIFF.FIFFV_COIL_VV_MAG_T1,
            FIFF.FIFFV_COIL_VV_MAG_T2,
            FIFF.FIFFV_COIL_VV_MAG_T3,
            FIFF.FIFFV_COIL_NM_122,
            FIFF.FIFFV_COIL_MAGNES_MAG,
            FIFF.FIFFV_COIL_BABY_MAG,
            # support for OPM data
            FIFF.FIFFV_COIL_QUSPIN_ZFOPM_MAG, # QuSpin v1
            FIFF.FIFFV_COIL_QUSPIN_ZFOPM_MAG2, # QuSpin v2
            FIFF.FIFFV_COIL_FIELDLINE_OPM_MAG_GEN1, # FieldLine v1
            FIFF.FIFFV_COIL_KERNEL_OPM_MAG_GEN1, # Kernel
        )

This modification ensures that OPM channels are properly accounted for in _channel_type when using write_raw_bids().

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>"

Copy link

welcome bot commented Jan 31, 2024

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

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.45%. Comparing base (87eea28) to head (b83b563).
Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1222      +/-   ##
==========================================
- Coverage   97.61%   97.45%   -0.17%     
==========================================
  Files          40       40              
  Lines        8685     8685              
==========================================
- Hits         8478     8464      -14     
- Misses        207      221      +14     

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

@sappelhoff
Copy link
Member

Hi there, thanks for the PR.

I am not sure whether we should include this here before OPM has been "properly" included in BIDS: bids-standard/bids-specification#1676

any other opinions?

@jasmainak
Copy link
Member

I think we should enable the specific coil type for now so people can share their data ... of course, sharing helmet designs etc should also be included in the specification but we should not wait for the specification to be updated. You can imagine updating info['chs'] to include the sensor locations in a fif file as was done with conventional MEG

@sappelhoff
Copy link
Member

Hi @AmaiaBA I think I now agree with Mainak and think there is little harm in allowing mne-bids to produce experimental/BIDSish OPM datasets for now, so I'd be happy to merge this PR, pending these items:

  • please create a changelog entry
  • please add yourself to CITATION.cff

See the "instructions-for-first-time-contributors" section in https://github.com/mne-tools/mne-bids/blob/main/CONTRIBUTING.md#instructions-for-first-time-contributors

@sappelhoff sappelhoff added this to the 0.16 milestone May 28, 2024
@sappelhoff
Copy link
Member

@AmaiaBA I just realized we never finished this. All we really need are a changelog entry and your name in the authors list -- the steps are described here: #1222 (comment)

do you have time to complete this soon?

@AmaiaBA
Copy link
Contributor Author

AmaiaBA commented Jul 28, 2024

hey I thought I had added the info you had requested, but maybe I did not commit them at the time... sorry!! It should be done now!

@sappelhoff
Copy link
Member

hey @AmaiaBA

hey I thought I had added the info you had requested, but maybe I did not commit them at the time... sorry!! It should be done now!

I don't really see any changes -- did you forget to git push?

@sappelhoff
Copy link
Member

Please note that you would also have to git pull on your branch before adding new changes, because there have been commits to this branch since you last pushed.

For simplicity, you could also tell me the following in a comment:

  • given name
  • family name
  • affiliation
  • orcid

@AmaiaBA
Copy link
Contributor Author

AmaiaBA commented Jul 30, 2024

I did forget to git push... I am so sorry. I did git pull/git push now, but please let me know if you can not see it? just in case, here is the info:
given name : Amaia
family name: Benitez
affiliation: Magnetoencephalography Core National Institutes of Mental Health, Bethesda, Maryland, USA
orcid: https://orcid.org/0000-0001-6364-7272

@sappelhoff sappelhoff enabled auto-merge (squash) July 30, 2024 14:32
@sappelhoff
Copy link
Member

Marking for merge, thanks @AmaiaBA

@sappelhoff sappelhoff merged commit 3cbdbab into mne-tools:main Jul 30, 2024
21 checks passed
Copy link

welcome bot commented Jul 30, 2024

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

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