-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
MRG: Add SNIRF support for 2D locations and NIRx files #9347
Conversation
This will fail until mne-tools/mne-testing-data#84 is merged. |
@larsoner @agramfort @drammock Could you please review and merge if happy. I expect CIs to go green |
Famous last words, I forgot to update the testing hash and version |
Ok there is some weird CI behaviour going on. The new files are being found and read in the Mac and windows CI tests (I opened the logs and checked all snirf tests were run and passed). But the linux CIs cant find the files. Could it be a caching thing? Other than the ci not finding the files I think this is good to go |
Ok I'm truly stumped here. @larsoner could you please give me a clue when you have a chance? |
I just redid the experiment for our tutorial at https://mne.tools/stable/auto_tutorials/preprocessing/plot_70_fnirs_processing.html I measured 25 minutes worth of data on myself with the NIRSport2 device and it was saved in the SNIRF format. The tutorial above worked simple by changing |
tools/get_testing_version.sh
Outdated
@@ -1,6 +1,8 @@ | |||
#!/bin/bash -ef | |||
|
|||
TESTING_VERSION=`grep -o "testing='[0-9.]\+'" mne/datasets/utils.py | cut -d \' -f 2 | sed "s/\./-/g"` | |||
# This can be incremented to start fresh when the cache misbehaves | |||
TESTING_VERSION=${TESTING_VERSION}-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bash magic, thanks @larsoner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really StackOverflow magic :)
https://stackoverflow.com/questions/63521430/clear-cache-in-github-actions#answer-64819132
I can replicate the failure locally:
Is it perhaps a real path problem that only Linux tests because on other CIs there is a |
@rob-luke I take it you're either on macOS or Windows, which both play more loosely with case than Linux...? |
@larsoner Mac at home, windows at work. And currently very embarrassed. |
I think we should all blame the OS and not the user here :) |
Im actually quite surprised that they handle case differently, you learn something every day. |
Thanks @rob-luke ! |
FYI @rob-luke another nugget for the day, these need to be on separate lines -- only the first closed automagically on merge |
Thanks @larsoner. And I am pleased as squeezed this in before the release. Cheers |
Reference issue
Closes #9338 mne-tools/mne-nirs#257 #9308.
What does this implement/fix?
This PR enables reading of SNIRF files from the NIRx vendor and it also enables reading of SNIRF files with 2D locations from the SfNIRS team.
Support for 2D locations was enabled by setting the z coordinate to 0.
Support for NIRx was enabled by not relying on the
sourceLabels
field in SNIRF. Which was optional in the spec, so I shouldn't have expected it to be there.Additional information
People may wish to support no sensor location info, but they can do that in a future PR.
It may also be possible to do some fancy 2D to 3D conversion for data, but that would be significant work.
I wanted to use an exact copy of some code, so I turned it in to a function called
_validate_nirs_info
Specifically validating and testing Artinis files will have to wait till someone can provide test data. However, I think this will achieve the 2D support they require.