Fix inconsistencies between written SNIRF files and the SNIRF spec

dsleiter commented 3 years ago

Describe the bug

hdf5 files written via mne_nirs.io.snirf.write_raw_snirf have a few inconsistencies with the (SNIRF File Format spec)[https://github.com/fNIRS/snirf/blob/master/snirf_specification.md].

In particular, the ones I've noticed so far are:

required /formatVersion is missing
required /nirs/metaDataTags/TimeUnit is missing
required /nirs/metadataTags/FrequencyUnit is missing
required /nirs/data(i)/measurementList(i)/dataType is only present for measurementList1
required /nirs/data(i)/measurementList(i)/dataTypeIndex is missing
/nirs/stim(i) should be using integer i values rather than floats (eg. the current files have stim1.0 instead of stim1)
required /nirs/stim(i)/name is missing

Steps to reproduce

Explore the snirf file using h5py or an HDF explorer like HDFView and compare against the (SNIRF File Format spec)[https://github.com/fNIRS/snirf/blob/master/snirf_specification.md].

Expected results

That the output snirf files are consistent with the SNIRF format spec.

Actual results

The inconsistencies listed above are present.

Additional information

Are any of these inconsistencies intentional?

dsleiter commented 3 years ago

I'll submit a PR for this issue. Please let me know if there are any of these that shouldn't be modified.

rob-luke commented 3 years ago

Thanks Darin! Thats great observations. We may need to follow this up with improvements to the reader afterwards too. But if you could open a PR to fix the writer that would be wonderful.

I also owe you an email. My apologies, I'll get on to it ASAP.

rob-luke commented 3 years ago

Are any of these inconsistencies intentional?

Absolutely not. Originally I copied the format from some of the example SNIRF files provided by the SFNIRS group, but later found out that the official files didn't even match the spec. So I think any errors you find we should fix. Thanks!

rob-luke commented 3 years ago

See https://github.com/fNIRS/snirf/issues/51 regarding the formatVersion string

dsleiter commented 3 years ago

Great, thanks for the info and context, @rob-luke. Happy to take a look at the reader as well afterward! I agree there will probably need to be some changes there too (for example, with the stim group numbering).

We've got some research results that we'll be sending you shortly!

mne-tools / mne-nirs