MRG: Add SNIRF reader

[rob-luke]

Reference issue

#7057

What does this implement/fix?

Adds ability to read NIRS data in the SNIRF format.

Additional information

This is becoming the default file format and will become the BIDS approved format.

[rob-luke]

This work is on hold until I better understand the expected behaviour of SNIRF files as discussed here fNIRS/snirf_homer3#5

[rob-luke]

Test data is at mne-tools/mne-testing-data#68

[rob-luke]

Ping @larsoner @agramfort

SNIRF is a very large standard that include many features that we will not support in MNE (at least for now) including time domain NIRS, a variety of data types we won't support(diffuse correlation spectroscopy), multiple simultaneous measurements on different people (hyperscanning), non uniform sampling, etc.

I have included a few checks for data types we don't support. But this is causing a decrease in coverage as I don't have test data for all these different types (and there is no way I can obtain most of these exceptions). Do you prefer a) I don't test all these obscure features and silently fail if a user passes in a gated time domain fluorescence measurement, or b) put these tests in and have a decrease in test coverage as I cant hit them?

Here is example of a check that I am referring to

https://github.com/mne-tools/mne-python/pull/7972/files#diff-6ddea29a0563d9d09a63ec6af1be0bf8R73

https://github.com/mne-tools/mne-python/pull/7972/files#diff-6ddea29a0563d9d09a63ec6af1be0bf8R92

[larsoner]

I have included a few checks for data types we don't support. But this is causing a decrease in coverage as I don't have test data for all these different types (and there is no way I can obtain most of these exceptions). Do you prefer a) I don't test all these obscure features and silently fail if a user passes in a gated time domain fluorescence measurement, or b) put these tests in and have a decrease in test coverage as I cant hit them?

One way to hit these lines with monkeypatch. If you want to try what I'm thinking of, here is an example where I hack an MNE function to give the output that we would get if a file were exhibiting the problematic behavior:

https://github.com/mne-tools/mne-python/pull/7965/files#diff-89350124c3b525af7b8aabdf157ae7ecR396-R411

It's less robust than actually having the problematic files because you have to make sure that what you mock actually properly captures the problematic behavior, but if you can accurately capture the behavior it's nice. But we also often just live with these lines not being covered, so it might not be worth the effort. Depends on how intrinsically motivated you are to work on this sort of problem...

Feel free to give it a shot if you want to play with pytest, otherwise I can try a little bit tomorrow (going to bed here no) and push a commit if successful.

[rob-luke]

Thanks for the info.

Depends on how intrinsically motivated you are to work on this sort of problem...

In this situation I am not very motivated. Lots of the checks will be for situations I never expect we will see in practice. Take for example the non uniform sampling, no commercial devices do this, I doubt even research devices do.

I think we should focus on making sure we support the same features in snirf as are available with the other readers in MNE. So currently we should support continuous wave data with uniform sample rates. Then as MNE is expanded to other nirs types we add this to our snirf support (eg once we have a frequency domain file type merged we can add snirf support for this).

This way we can provide support for what I imagine is the most common workflow. Measure data and save in native format -> read data with MNE -> save in bids format as snirf -> read in snirf bids data for processing.

So how about I put in a few checks, but I don't go overboard and we allow the coverage to slightly decrease?

[larsoner]

Sounds good

[rob-luke]

@larsoner @agramfort could you please review this file reader. I have been using it for a week with no issues, so I think its behaving correctly.

The SNIRF file format is quite extensive, so as NIRS support expands in MNE this reader will need to be revisited (e.g. kylemath is adding frequency domain nirs support). Next I will write a SNIRF writer function.

[larsoner]

Looks good, just a bunch of tiny comments

[larsoner]

This is really a testing decorator not meant to be used by user-facing functions. Usually we do this:

if not check_version('h5py'):
    raise ImportError('The h5py package is required to read raw SNIRF data')

If you want, you could add a helper to mne/utils/check.py to make this process more general, like:

def _require_version(lib, what, version='0.0'):
    if not check_version(lib, version):
        extra = f' (version >= {version})' if version != '0.0' else ''
        raise ImportError(f'The {lib} package{extra} is required to {what}')

WDYT?

[rob-luke]

Sounds good. Ive added this as you suggest. I have not gone through the code base to edit other functions. But will do that once this is merged as a seperate PR.

[rob-luke]

I figure I can do a search for all uses of check_version and see which match this pattern. Any other suggestions?

[larsoner]

That's all I would do. Don't need to catch all of them, we can always migrate as we're working on bits of code anyway.

[larsoner]

Ahh, if you're already using this (which probably does what I propose adding internally to MNE above) then you shouldn't have requires_h5py above

[rob-luke]

Ok I think I have handled this as you suggest now. But its probably worth double checking I interpreted your suggestions correctly.

[rob-luke]

Thanks for the review @larsoner. As always I have learnt a few things from your comments. Could you please check I have included your suggestions accurately. The Mac CI failed due to 50 min limit, I assume this is unrelated to these code changes.

Next I will code up a SNIRF writer (then we should have close to full support for NIRS BIDS). Where should this writer be placed? I could write a write_raw_snirf and put it in mne/io/snirf/_snirf.py, or should I extend save in mne/io/base.py to add an fname=*.snirf option?

[larsoner]

Next I will code up a SNIRF writer (then we should have close to full support for NIRS BIDS). Where should this writer be placed? I could write a write_raw_snirf and put it in mne/io/snirf/_snirf.py, or should I extend save in mne/io/base.py to add an fname=*.snirf option?

Let's open a separate issue about this as it will require some discussion

[larsoner]

#8060

[larsoner]

Thanks @rob-luke !