MRG: Honor optode_frame argument in read_raw_snirf #9461
Conversation
rob-luke
changed the title
rob-luke
changed the title
|
@larsoner @agramfort could you please review. |
mne/io/snirf/tests/test_snirf.py
Outdated
| assert read_raw_snirf(nirx_nirsport2_103, optode_frame="unknown").\ | ||
| info['chs'][10]['coord_frame'] == 0 | ||
| assert read_raw_snirf(nirx_nirsport2_103, optode_frame="head").\ | ||
| info['chs'][10]['coord_frame'] == 4 |
There was a problem hiding this comment.
can you replace the 4 by FIFF.FIFFV_COORD_HEAD ?
mne/io/snirf/tests/test_snirf.py
Outdated
| assert read_raw_snirf(nirx_nirsport2_103, optode_frame="asdf").\ | ||
| info['chs'][10]['coord_frame'] == 0 | ||
| assert read_raw_snirf(nirx_nirsport2_103, optode_frame="mri").\ | ||
| info['chs'][10]['coord_frame'] == 4 |
There was a problem hiding this comment.
why is it not
FIFF.FIFFV_COORD_MRI
? if there is a good reason can you put FIFFV_COORD_HEAD and explain why?
There was a problem hiding this comment.
why is it not FIFF.FIFFV_COORD_MRI
Because we currently convert everything to head coordinates.
As of a year ago there wasn't even a way to store the coordinate frame of individual channels. Now there is (via the extended channel info blocks). In some/many places in MNE, we also still assume that non-MEG channels are in the HEAD coordinate frame. This is gradually being changed but we're not there yet.
|
Thanks for the review @agramfort. I have changed to testing against FIFF. Is @larsoner's description above sufficient? Or do you think we should change to returning something other than head? I am happy to implement whatever is appropriate. Edit: I think the failing CI is due to a timeout, not this PR. |
|
Unfortunately the CI is failing as its hitting a 60 min timeout, so I will not merge myself. @larsoner could you merge if this is acceptable. |
|
Thanks @rob-luke ! |
3 participants
What does this implement/fix?
PR #9401 introduced the argument
optode_frametoread_raw_snirf. This was being used to transform the coordinates of sensors, but was not being stored ininfo['chs']. Also added a test.