mne-tools / mne-nirs

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

Validate SNIRF file writer #421

Closed rob-luke closed 2 years ago

rob-luke commented 2 years ago

Validate that the SNIRF files we write are valid using the official validator: https://github.com/BUNPC/pysnirf2 thanks to @sstucker

Our files did pass the old validator https://github.com/andyzjc/snirf_validator

Currently our files are not passing the new validator, so we may need to modify the MNE SNIRF writer.

sstucker commented 2 years ago

I would wait to let a few kinks get worked out of the validation code.

Some good goalposts exist in the code as it stands today, however:

proper string formatting via h5py which is required by the validator (or else a warning will be raised)

_varlen_str_type = h5py.string_dtype(encoding='ascii', length=None)  # Length=None creates HDF5 variable length string

def _create_dataset_string(file: h5py.File, name: str, data: str) -> h5py.Dataset:
    return file.create_dataset(name, dtype=_varlen_str_type, data=str(data))

This consistently exports a variable length string/ascii null-terminated string. If a mixture of variable length and fixed length strings are used the parsing logic becomes more complicated so we are trying to move towards just one: https://github.com/fNIRS/snirf/issues/72

Another thing that happens a lot because is values being saved as arrays of 1 element and 1 dimension. This will cause the validator to mark your Dataset with INVALID_DATASET_SHAPE.

codecov[bot] commented 2 years ago

Codecov Report

Merging #421 (ad6f918) into main (d4eb96d) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #421   +/-   ##
=======================================
  Coverage   95.38%   95.38%           
=======================================
  Files          62       62           
  Lines        2252     2255    +3     
  Branches      290      290           
=======================================
+ Hits         2148     2151    +3     
  Misses         48       48           
  Partials       56       56           
Impacted Files Coverage Δ
mne_nirs/io/snirf/tests/test_snirf.py 97.43% <100.00%> (+0.10%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d4eb96d...ad6f918. Read the comment docs.

rob-luke commented 2 years ago

Thanks @sstucker It seems our files now pass the validator, so I will merge this additional test to ensure we dont break SNIRF comparability in the future.