ENH: Support TD data

[larsoner]

Closes #9661

@rob-luke how about this plan:

• [x] Switch to f-strings for compactness
• [x] Switch to .item() instead of using _correct_shape
• [x] Get CIs green on existing tests
• [x] Merge PR to fiff-constants for new constants: https://github.com/mne-tools/fiff-constants/pull/40
• [x] Add tests for files from @JohnGriffiths in https://github.com/mne-tools/mne-python/pull/9661#issuecomment-964493871
• [x] @Zahra-M-Aghajan, @julien-dubois-k, @JohnGriffiths, and @rob-luke to latest.inc for this nice contribution (I'm just cleaning up really, you all did the hard work!)
• [x] Add files from https://github.com/mne-tools/mne-python/pull/11064#discussion_r1838886429
• [x] Fix constants from https://github.com/mne-tools/mne-python/pull/11064#discussion_r1846660191
• [x] Check that constants etc. make sense
• [ ] Merge fixes into fiff-constants
• [ ] Figure out scale factor issue and proper scale factors for TD data in plotting using https://github.com/mne-tools/mne-nirs/pull/586#issuecomment-2475498853
• [ ] Merge adding the above collaborators as co-authored-by

For now I'm inclined to make it so if you get a SNIRF_PROCESSED data type that we don't know how to handle, we should maybe raise an error in this case...? For now I've removed it as a coil_type because this _PROCESSED really seems to mean something else specific, and we should perhaps add those one by one as we find use cases for them.

I have some collaborators who have seen Kernel help files telling them to use this very out of date branch, so it would be nice to get this in!

[rob-luke]

Thanks for jumping in to finish this off @larsoner

For now I'm inclined to make it so if you get a SNIRF_PROCESSED data type that we don't know how to handle, we should maybe raise an error in this case...?

Agreed

Ping me when you want a review, it doesn't need to be a final review, I am happy to take a look early if that helps.

[larsoner]

Feel free to look now @rob-luke to see if you're happy with the overall design, the rest of the work is "just" some remaining details (adding unit tests, playing with the data a little bit to set scale factors, etc.) that I think we can rely on CIs to check, or that we can iterate on in follow-ups (e.g., tweaking plot scale factors)

[larsoner]

@Zahra-M-Aghajan, @julien-dubois-k, @JohnGriffiths this seems to have at least some basic tests and testing files now. When you get a chance, can you try it with some your TD-moments data, and one of you push some commits to fix any plotting scale factors (ideally at least raw.plot to start)? It doesn't have to be perfect but anything that gets close is a good start.

Any chance @julien-dubois-k you can also export the TD-gated-amplitude data for the same files as before in https://github.com/mne-tools/mne-python/pull/9661#issuecomment-964493871 (if this even makes sense)? It looks like we have just TD-moments-amplitude in the testing files so far.

I think this is close to merge!

[julien-dubois-k]

@larsoner I'll check this out by week's end -- thanks a lot for pushing this forward!

[julien-dubois-k]

@larsoner I tried the branch with some moments data and it loads and plots 👍 but yes, with bad scalings.

Re: scalings

• moment0 (# photons, from 0 to ~1e7),
• moment1 (mean time of flight, in picoseconds is on the order of 1000);
• moment2 (variance of time of flight, in picoseconds^2 is on the order of 100000) but they're all considered the same data type (fnirs_td_moments_amplitude) -- how do you suggest we deal with this?

I just generated three short test files again, which I can share soon once we agree on the units and path forward for scalings:

• one for TD-GATED-AMPLITUDE
• one for TD-MOMENTS-AMPLITUDE
• one for HBO/HBR (looks like we should export in M for it to play well with plotting, we currently export microM)

[larsoner]

moment0 (# photons, from 0 to ~1e7), moment1 (mean time of flight, in picoseconds is on the order of 1000); moment2 (variance of time of flight, in picoseconds^2 is on the order of 100000)

Well we have this for the channel information (basically):

                fnirs_td_moments_amplitude=dict(
                    kind=FIFF.FIFFV_FNIRS_CH,
                    unit=FIFF.FIFF_UNIT_V,
                    coil_type=FIFF.FIFFV_COIL_FNIRS_TD_MOMENTS_AMPLITUDE),

So right now we say the units are Volts, which is wrong.

So far in MNE there is a 1:1 mapping between the coil_type and its units. The easiest way forward from a code perspective would be to continue with this practice and actually have three different coil_types. I think that probably makes the most sense here, even though it'll require another PR to fiff-constants. WDYT @julien-dubois-k ?

Notably, in https://github.com/mne-tools/mne-python/pull/11152#issuecomment-1466506923 we discuss how this 1:1 mapping won't work well for eye-tracking data (units need to be converted sometimes) so we'll need to change that. In this PR we could similarly set the unit properly and eventually get it to look okay with plotting functions, but it will take some time I think. EDIT: Also I don't think the issue here is conceptually similar enough to wait for this as an option.

one for HBO/HBR (looks like we should export in M for it to play well with plotting, we currently export microM)

Ideally whatever reader (SNIRF?) you're using to read in MNE would detect that the units on disk are not SI and convert by scaling by 1e-6 to go from uM to M when reading the data. But as a workaround in the meantime if you can export in SI units that's what we should start with

[larsoner]

... and one more @julien-dubois-k , are these moment0, moment1, moment2 the same units for TD-GATED-AMPLITUDE and for TD-MOMENTS-AMPLITUDE? Or does TD-GATED-AMPLITUDE have other units/data types?

[julien-dubois-k]

Yes, different coil types makes sense to work within the MNE framework.

The coil types that were defined in the PR to fiff-constants borrowed from the dataType attribute of measurements in the SNIRF file. We can make the coil types more specific by also specifying the more precise dataTypeLabel, also defined in the SNIRF spec. So for instance -- fnirs_td_moments_amplitude_dOD, fnirs_td_moments_amplitude_dMean, fnirs_td_moments_amplitude_dVar. Then they can have proper units (A.U. for dOD, s for dMean, s^2 for dVar).

I actually see that Kernel needs to update the SNIRF files to use one of the accepted dataTypeLabel (oops). That spec wasn't there when we first wrote our SNIRF export code. So we'll need to make some updates and will share new test files. Also, I see that the quantities we currently export are NOT supported by the SNIRF spec. We can easily convert them -- moment0 -> dOD, moment1 -> dMean, and moment2 -> dVar. I've raised an issue in the SNIRF spec to see if only those relative quantities are to be supported.

Re: SI -- we use mne.io.snirf.read_raw_snirf to read into MNE, I'll look into where to detect SI non-compliance and convert.

Thank you for your support!

[julien-dubois-k]

been a few months :-) I just re-raised the issue with the SNIRF spec in the SNIRF github repo...

[larsoner]

@julien-dubois-k should we wait for the SNIRF people to make a decision before proceeding here? Or would now be a good time for me to come back to this PR?

[julien-dubois-k]

let's wait -- I'll try to reach out directly to some of the main developers on the SNIRF team if needed. Thanks!

[larsoner]

Closing because 1) we're still waiting for info (I think?) and 2) there is a rebase/merge mess probably not worth fixing for +308 −127 -- better to open a new dedicated PR when someone is motivated I think!

[julien-dubois-k]

ah! as a matter of fact, there was some movement on the SNIRF side 5 days ago (here) which may have unblocked this. I'll re-open an issue once I get around to taking stock of what is needed.