mne-tools / fiff-constants

Bookkeeping and documentation of FIFF file format constants
4 stars 12 forks source link

Modify NIRS coil type names to match SNIRF and add frequency domain types #28

Closed rob-luke closed 4 years ago

rob-luke commented 4 years ago

This PR modifies the existing 302 coil type name to better match the SNIRF specification (a common format agreed to by the society for fNIRS) https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#appendix

It also adds two new types (also defined in SNIRF) for frequency domain NIRS.

In https://github.com/mne-tools/mne-python/issues/7912 we have discussed if these types are actually required, but please feel free to continue the discussion here if there are additional questions.

rob-luke commented 4 years ago

FYI @larsoner @agramfort

mkajola commented 4 years ago

For Megin this is ok. fNIRS community can decide. There is only one new value; for the raw->amplitude only the name changes. This is fine if the meaning is the same. This would be true if the "raw signal" in (about all) systems indeed is the cw amplitude, and there is no need to have possibility to tell the difference. For example, that the raw signals of some systems might be something that need to be further processed to produce amplitude and phase. I don't know about the conventions used in fNIRS, but for narrow band signal the amplitude can either mean the momentary amplitude (raw signal probably is that), or the envelope of the signal (that is, the ampitude of the analytic signal). Maybe this should be stated in the description which one is meant.

larsoner commented 4 years ago

There is only one new value; for the raw->amplitude only the name changes. This is fine if the meaning is the same. This would be true if the "raw signal" in (about all) systems indeed is the cw amplitude, and there is no need to have possibility to tell the difference. For example, that the raw signals of some systems might be something that need to be further processed to produce amplitude and phase.

The two new ones are:

fnirs_fd_ac_amplitude   304  "fNIRS frequency domain AC amplitude"
fnirs_fd_phase          305  "fNIRS frequency domain phase"

And I actually initially had the same thought as you, that there could somehow be a translation between the frequency domain amplitude and phase and the time domain / continuous wave data. However, from what I understand, they are actually distinct in terms of the data they provide:

This is explained better in https://github.com/mne-tools/mne-python/issues/7912#issuecomment-646926461 and the comment that follows it, but basically, the meaning of "raw" is now more specific to mean the continuous wave / DC value that you get by just A/D converting the data. Frequency domain time-varying amplitude and phase of the MHz frequencies could also be considered different "raw" forms/aspects of the measured data, but they seem sufficiently different to warrant a new coil type (like mag vs grad). They are complementary and both can and will be used to estimate the existing other fNIRS types we have (optical density, hbo, and hbr).

Does this make sense?

For narrow band signal the amplitude can either mean the momentary amplitude (raw signal probably is that), or the envelope of the signal (that is, the ampitude of the analytic signal). Maybe this should be stated in the description which one is meant.

The signal formerly called "raw" that we now call "continuous wave amplitude" is indeed just the momentary amplitude, so I don't think we need to add anything (if it were the Hilbert envelope, indeed something like "envelope amplitude" would make more sense).

mkajola commented 4 years ago

I entered through the diff and the second change slipped my eye. Should have read the comment thread bit better. Yes, your explanation makes sense. And the cw / fd distinction is quite clear now when I understand bit better what this is all about. So fine for me.

larsoner commented 4 years ago

Okay great, thanks @mkajola @rob-luke @agramfort for the quick work!

rob-luke commented 4 years ago

Thanks @mkajola, sorry I wasn’t clearer in the description. Thanks @larsoner for clarifying above.