ENH: Support TD data

[larsoner]

Closes #9661

@rob-luke how about this plan:

• Switch to f-strings for compactness
• Switch to .item() instead of using _correct_shape
• Get CIs green on existing tests
• Merge PR to fiff-constants for new constants: ENH: Add two more fNIRS coil types fiff-constants#40
• Add tests for files from @JohnGriffiths in Feature/td nirs snirf #9661 (comment)
• @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!)
• Add some example
• Figure out scale factor issue and proper scale factors for TD data in plotting
• 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)

[rob-luke]

Looking good, it will be great to have support for TD in MNE! I will review again once there are tests.

[larsoner]

@JohnGriffiths @julien-dubois-k @Zahra-M-Aghajan let me know if you want a different URL to link from your name in the changelog

[JohnGriffiths]

grifflab.com is perfect, thanks

[larsoner]

Okay code has been updated. I suspect the scaling could be wrong as I mention in a comment above ☝️ below 👇 but in any case people can test it with some data!

It's possible/likely that in addition to the possible scaling bug, we'll want to adjust the default plot scaling limits.

It would also be great to have an example added (maybe using mne-misc-data) showing that this data can at least be read!

[larsoner]

I modified this to index into fnirs_time_delay_widths when it didn't before. It didn't error in either case, and it seems like one of them should have! So we still need to add a suitable SNIRF test file to mne-testing-data @JohnGriffiths (or others) if you have a tiny one you can share

[larsoner]

@julien-dubois-k this one is also for you ☝️

[julien-dubois-k]

test file shared here, hopefully it helps with this case

[larsoner]

This should be in units of M (mol). Looking at the values in the test file:

>>> import h5py
>>> import numpy as np
>>> x = h5py.File("td_moments.snirf", "r")
>>> list(x["nirs/data1/measurementList1"])  # no dataUnit, so code assumes M not uM!
['dataType', 'dataTypeIndex', 'dataTypeLabel', 'detectorIndex', 'sourceIndex', 'wavelengthIndex']
>>> np.array(x["nirs/data1/measurementList1/dataTypeLabel"]).item()
b'Time Domain - Moments - Amplitude'
>> np.nanmedian(np.linalg.norm(np.array(x["nirs/data1/dataTimeSeries"]), axis=0))
np.float64(429256.98709975014)

should the code assume that if no dataUnit is present, the units are actually uM?

[larsoner]

@julien-dubois-k this one is for you ☝️

[julien-dubois-k]

b'Time Domain - Moments - Amplitude' so this is moments data.
The units will be:

• unitless for the "sum" moment (it's a photon count)
• some multiple of s for the "mean" moment (it's a time of flight)
• some multiple of s**2 for the "var" moment (it's a variance of the time of flight)

The SNIRF spec doesn't require dataUnit to be populated (here). In cedalion it looks like they just treat unspecified dataUnit as unitless or a.u. (here). I think vendors who generate SNIRF files should populate the dataUnit field always if there is a unit. Kernel didn't originally (hence the missing dataUnit in the current test files), but does now. I'll provide new test files shortly.

[julien-dubois-k]

test files shared here

[JohnGriffiths]

confirming that the example runs here, and also that it seems to load other data files properly. the latter point I will can make and share a separate NB demonstrating that for 'peace of mind', in lieu of an analysis example discussed on the issue thread. Also noting that given @julien-dubois-k 's comment re some .snirf attributes not being read, so those steps may also need to be added either to this PR or (probably better) a subsequent one. I will be looking at this next.

Afraid I don't know the answer to the uM units Q off hand.

[JohnGriffiths]

@larsoner thanks for this. Have gone through, left review above.

Not quite sure if I missed something specific other than the units Qs and the time_delay_widths Qs that you needed an immediate answer to; LMK

I don't not know to these off the top of my head, but I imagine @julien-dubois-k does. I can take a look at relevant documentation if that is useful.

Re: datasets

LMK if I have got this right re next steps

• You have two small .snirf files in the example data that is loaded here so new ones of those aren't needed from me now for this test function
• But you are suggesting to add some other, larger files to mne-misc-data for an analysis example (?)
• So, would you like me to do a PR to that repo with a .snirf data file for a more fulsome analysis example?
• If so yes I can get on that. What I would do for the example code is use the colab notebook example @julien-dubois-k linked to in his above comment, and the example file the one used in that nb.
• ( I can add other finger tapping .snirf files to this if warranted )
• As I work on that I will take a look at the comments from @julien-dubois-k noted in my review comment above about some .snirf attributes not being read from the .hdf properly by the mne reader. Which may lead to another .io.snirf PR before the analysis example PR.
• But I think it's prob best not to muddy the current PR with that Q (unless you disagree?)

[julien-dubois-k]

thank you @larsoner and @JohnGriffiths for your work pushing this PR forward. I’ll review tomorrow and answer any remaining questions that I can answer!

…

On Wed, Nov 6, 2024 at 17:56 John Griffiths ***@***.***> wrote: @larsoner <https://github.com/larsoner> thanks for this. Have gone through, left review above. Not quite sure if I missed something specific other than the units Qs and the time_delay_widths Qs that you needed an immediate answer to; LMK I don't not know to these off the top of my head, but I imagine @julien-dubois-k <https://github.com/julien-dubois-k> does. I can take a look at relevant documentation if that is useful. Re: datasets LMK if I have got this right re next steps - You have two small .snirf files in the example data that is loaded here so those aren't needed now for this test function - But you are suggesting to add some other, larger files to mne-misc-data for an analysis example (?) - So, would you like me to do a PR to that repo with a .snirf data file a a more fulsome analysis example? - If so yes I can get on that. What I would do for the example code is use the colab notebook example @julien-dubois-k <https://github.com/julien-dubois-k> linked to in his above comment <#11064 (comment)>, and the example file the one used in that nb. - ( I can add other finger tapping .snirf files to this if warranted ) - As I work on that I will take a look at the comments from @julien-dubois-k <https://github.com/julien-dubois-k> noted in my review comment above about some .snirf attributes not being read from the .hdf properly by the mne reader. Which may lead to another .io.snirf PR before the analysis example PR. - But I think it's prob best not to muddy the current PR with that Q (unless you disagree?) — Reply to this email directly, view it on GitHub <#11064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWIKIDKMADYHYTI6KHYECT3Z7LCGNAVCNFSM57FOV3OKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TENBWGEYTKNZRG44A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[larsoner]

You have two small .snirf files in the example data that is loaded here so new ones of those aren't needed from me now for this test function

Well related to the time_delay_widths issue, it would be good to have some small SNIRF test files that actually have this field. I'm not sure if we're using it properly, and if we had test files with this field then we could probably figure it out. @julien-dubois-k if you have such a file we could add it to mne-testing-data!

But you are suggesting to add some other, larger files to mne-misc-data for an analysis example (?) ... What I would do for the example code is use the colab notebook example @julien-dubois-k linked to in his above comment, and the example file the one used in that nb.

Sure, then we modify an example here or more likely in mne-nirs to use it. Doesn't have to be a full-blown example with regression and stats. But if that notebook runs in less than, say, a minute, we could convert it, add some narrative docs if needed, and be good to go. (Examples are meant to run fairly quickly and not require a ton of data to be downloaded!)

( I can add other finger tapping .snirf files to this if warranted )

Sure if it adds something complementary from a documentation perspective. But if it's just additional subjects or a different paradigm from what we currently have, might not be worth it.

(Just an aside, not sure if you've looked at mne-bids-pipeline but there is mne-tools/mne-bids-pipeline#365 that we could resurrect if you're interested in a BIDS pipeline for your data.)

But I think it's prob best not to muddy the current PR with that Q (unless you disagree?)

I don't mind adding more stuff here, a +305/-85 PR is fairly small actually. So based on discussion I think the next steps would be:

1. @JohnGriffiths open PR to add example data in mne-misc-data that we need for the notebook
2. I modify mne.datasets code in this PR to reflect the new data
3. @JohnGriffiths open a PR to mne-nirs to use the new example data based on the notebook
4. I modify the mne-nirs PR to use this branch temporarily (or you can if you already know how @JohnGriffiths)
5. @julien-dubois-k supply some tiny files we can add to mne-testing-data that would shed light on the time_delay_widths issue
6. @julien-dubois-k comment on the unit issue above
7. I push commits to update tests with new data and/or unit expectations
8. I make sure all plotting scales etc. are correct based on the MNE-NIRS example
9. We merge this PR
10. I remove "use larsoner:feature/TD-nirs_snirf branch" shim from MNE-NIRS PR
11. We merge the MNE-NIRS PR

[JohnGriffiths]

@JohnGriffiths open a PR to mne-nirs to use the new example data based on the notebook

Ok I'm on it.

@JohnGriffiths open PR to add example data in mne-misc-data that we need for the notebook

Will do. What is the heuristic on acceptable size for a file in mne-misc-data to use in an example?

I modify the mne-nirs PR to use this branch temporarily (or you can if you already know how @JohnGriffiths)

So just temporarily change this requirements line to the commit url for the current PR, right?

(Just an aside, not sure if you've looked at mne-bids-pipeline but there is mne-tools/mne-bids-pipeline#365 that we could resurrect if you're interested in a BIDS pipeline for your data.)

That's great to know. thanks. We are actually currently in the process of getting an mne-bids-pipeline auto QC in XNAT for our EEG data. Would like to do the same for fNIRS. That would slot in nicely.

[larsoner]

Will do. What is the heuristic on acceptable size for a file in mne-misc-data to use in an example?

We try to keep our datasets to < 1GB if possible. Looks like mne-misc-data is around 700 MB so it would be good not to add too much more. Maybe up to 100MB is okay for the purposes here? If you need more than that then we can add it as a new dataset on OSF or somewhere else, and add a mne.datasets.<something>.data_path() downloader. That's a little more work but can be worth it.

So just temporarily change this requirements line to the commit url for the current PR, right?

CircleCI is the CI that builds the doc so I would do it here actually

https://github.com/mne-tools/mne-nirs/blob/5c63e855b60d6f7aab907477e582acb323a69d81/tools/circleci_dependencies.sh#L13

[julien-dubois-k]

these are photon counts, should they be "AU"?

[julien-dubois-k]

I imagine this should be 1.0; same for fnirs_td_moments_amplitude

[julien-dubois-k]

this is a bit weird, the dataType Time domain – Moments (TD Moments) - Amplitude can be any of the moments, which all have different units:

• sum or intensity is photon counts
• mean is s
• variance is s^2

[julien-dubois-k]

update

[julien-dubois-k]

Here are some files to add. The path could be updated if it matters, these are no longer Flow50/Flow1 but Flow2, and the version is latest (aka, Portal_2024-10-23).

• Type 201 (Time Domain Gated): c345d04_2.snirf (there is no current equivalent)
• Type 301 (Time Domain Moments): c345d04_3.snirf (replace td_moments.snirf)
• Type 99999 (Time Domain Hb Moments): c345d04_5.snirf (replace hb.snirf)