mne-tools / mne-nirs

Process Near-Infrared Spectroscopy Data in MNE
https://mne.tools/mne-nirs/
BSD 3-Clause "New" or "Revised" License
79 stars 35 forks source link

write subject ID to ["his_id"], not ["first_name"] #546

Closed aaronjnewman closed 4 months ago

aaronjnewman commented 5 months ago

Reference issue

Fixes #544

What does this implement/fix?

write subject ID to ["subject_info"]["his_id"], not ["subject_info"]["first_name"]. This makes the code more consistent with BIDS specifications, and fixes error generated by write_raw_snirf() described in #544

rob-luke commented 5 months ago

Thanks @aaronjnewman. My first gut feeling is that this makes sense. I just enabled the CI to run the tests. Don't worry if the tests fail, we will work with you to fix any unrelated errors and understand why anything is failing due to the change.

larsoner commented 5 months ago

I think you need to at least adjust this test:

mne_nirs/io/snirf/tests/test_snirf.py:79: in test_snirf_write_raw
    assert diffs == ""
E   assert "\n['subject_info']['first_name'] value mismatch (NIRX_Test, NIRX)" == ''
E     
E     + 
E     + ['subject_info']['first_name'] value mismatch (NIRX_Test, NIRX)

to check his_id now instead of first_name

aaronjnewman commented 4 months ago

I may have to defer to @rob-luke or others on this one - I'm not familiar with CI and not sure where this originates. I don't see any instance of first_name anywhere in the repository since my change was made.

larsoner commented 4 months ago

Pushed a commit that should make sense alongside https://github.com/mne-tools/mne-python/pull/12526. We should merge that then restart CIs here then things should be green :crossed_fingers:

@aaronjnewman feel free to look to see if the two PRs make sense to you

aaronjnewman commented 4 months ago

@larsoner thanks - like many things, it's clear when I see it, but generating it myself would be a whole 'nother thing :) Appreciate the quick fix!

larsoner commented 4 months ago

Thanks @aaronjnewman