Closed AmaiaBA closed 2 months ago
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 97.45%. Comparing base (
87eea28
) to head (b83b563
). Report is 25 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi there, thanks for the PR.
I am not sure whether we should include this here before OPM has been "properly" included in BIDS: https://github.com/bids-standard/bids-specification/issues/1676
any other opinions?
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
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:
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
@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: https://github.com/mne-tools/mne-bids/pull/1222#issuecomment-2134679150
do you have time to complete this soon?
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!
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
?
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:
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
Marking for merge, thanks @AmaiaBA
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪
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:
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():
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: