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

Fix SNIRF duration writing bug #497

Closed rob-luke closed 1 year ago

rob-luke commented 1 year ago

Reference issue

SNIRF file should write stims/annotations with the following format: [starttime duration value]. As described here: https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#nirsistimjdata

What does this implement/fix?

Modifes the write_raw_snirf function to write stims according to the format above. Added tests to ensure round trip read-write is lossless for stimulus parameters.

Additional information

This also requires a change to MNE here: https://github.com/mne-tools/mne-python/pull/11397

codecov[bot] commented 1 year ago

Codecov Report

Merging #497 (c1e4390) into main (3c81e9d) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
+ Coverage   95.26%   95.31%   +0.05%     
==========================================
  Files          69       69              
  Lines        2724     2754      +30     
  Branches      394      394              
==========================================
+ Hits         2595     2625      +30     
  Misses         65       65              
  Partials       64       64              
Impacted Files Coverage Δ
mne_nirs/io/snirf/_snirf.py 95.48% <100.00%> (ø)
mne_nirs/io/snirf/tests/test_snirf.py 97.50% <100.00%> (+0.83%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rob-luke commented 1 year ago

@larsoner could you please review. Specifically, I am unsure about my skipping of tests for pre 1.4 and if this is a bad idea?

larsoner commented 1 year ago

Up to you if you want to skip. Another slightly more complete option would be to triage based on < 1.4 or >= 1.4 and check that the duration matches expectation, e.g. 5 on < 1.4 and the correct value(s) on >= 1.4. But this is probably overkill. Feel free to merge if you're happy with this version!

rob-luke commented 1 year ago

Thanks mate, in it goes!