MRG: Additional checking of fNIRS data before processing #9184
Conversation
|
I think this is good to go (assuming CIs come back green, they do locally). @larsoner @agramfort @drammock could you please review. |
|
|
||
| For raw amplitude measurements and for optical density data | ||
| the wavelength information must be stored in | ||
| ``info["chs"][ii]["loc"][9]`` |
There was a problem hiding this comment.
Everything before this (channel names and ordering) has a public API that allows it to be changed easily. But this part -- about info['chs'][ii]['loc'] -- does not, and exists in an info field that we say shouldn't be modified by people. So this I'd rather add somewhere else as it doesn't seem right for a user-facing tutorial.
There was a problem hiding this comment.
... so really I think I would recommend:
- Move this text in its entirety to somewhere else
meas_info.pymaybe? Ormne/preprocessing/*nirs*? - Reword this tutorial text to be much shorter, warning people that the channel names and ordering are set properly by MNE reading and preprocessing functions and shouldn't be changed by the user by
raw.rename_channels,raw.reorder_channels, etc.
There was a problem hiding this comment.
as it doesn't seem right for a user-facing tutorial
Thanks @larsoner, I agree that users shouldn't be editing or creating fields like this and have expressed that in #9141.
I will remove the tutorial section from this PR and leave these details for discussion in #9141. And keep this PR as just backend improvements.
|
Done. This PR is now only additional back end checks for fNIRS data. @larsoner could you please review again (I didnt edit code so I assume CIs will go green as they did above). |
rob-luke
changed the title
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
LGTM and I think a latest.inc change is not necessary since this seems mostly like internal refactoring / tests that shouldn't affect users (just devs implementing new things), feel free to merge once CIs come back happy @rob-luke
|
Thanks @larsoner Ill merge if all green. |
2 participants
Reference issue
Further preparation for #9141
What does this implement/fix?
Now that we are going to have people creating their own data structures it will become more important to thoroughly check the ordering and encoding of fNIRS metadata.
Additional information
I will add checking for hbo/hbr type next, but wanted to thoroughly test the existing function before I expand it.