[MRG, ENH] Get annotation description from snirf stim name #9575
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
dsleiter
changed the title
|
I believe this is ready for review! |
| dat.get('/nirs/' + key + '/data'))) | ||
| if data.size > 0: | ||
| annot.append(data[:, 0], 1.0, key[4:]) | ||
| desc = dat.get('/nirs/' + key + '/name')[0] |
There was a problem hiding this comment.
For anyone else reviewing, this corresponds to https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#nirsistimjname
|
Thanks @dsleiter, this looks good to me. Lets see if we can get a review from @larsoner too. As there is a matching PR at mne-tools/mne-nirs#326 I suggest we wait till this PR has all green CI, then I will pull both PR branches and test locally (I will test a few real files have no loss of info when converted back and forth). Then once once all CIs are green, my local tests are passing, and we have a few review approvals we merge. Ok? |
rob-luke
left a comment
rob-luke
left a comment
There was a problem hiding this comment.
I tested this locally with the associated MNE-NIRS branch and all results seemed correct to me. I also tested locally with some real measurements.
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
|
Thanks @dsleiter ! |
* upstream/main: [MRG, ENH] Find aseg labels near montage (mne-tools#9545) Add label to colorbar in GAT plot [skip actions] (mne-tools#9582) [ENH, MRG] Encapsulate warp elec image in function (mne-tools#9544) [DOC, MRG] Add "info" to `docdict` (mne-tools#9574) [MRG] Add `units` parameter to get_data for Evoked (mne-tools#9578) [MRG, ENH] Get annotation description from snirf stim name (mne-tools#9575) [MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg (mne-tools#9556)
3 participants
Thanks for contributing a pull request! Please make sure you have read the
contribution guidelines
before submitting.
Please be aware that we are a loose team of volunteers so patience is
necessary. Assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
Again, thanks for contributing!
Reference issue
Fixes #9571
What does this implement/fix?
Uses the 'name' field from SNIRF file stim groups as the description for Annotations rather than the index of the stim group.
Additional information
This is in response to mne-tools/mne-nirs#326
This could be a breaking change for anyone who depends on annotation descriptions when reading from SNIRF files that have not been created from the update to the MNE-NIRS SNIRF writer. I'd like some guidance on whether or not I should implement a deprecation or not. I listed this as an ENH because it isn't obvious that this is a bug fix, but it is in response to a fix of what is a bug in the SNIRF writer in order to maintain consistency.