Closed rob-luke closed 4 years ago
Pinging @larsoner @agramfort @cbrnr @mshader @NathalieGou
@kylemath Would these two data types be sufficient for you? Maybe you only need fnirs_phase
, but I think its easiest to just add both at once.
Naively a frequency-domain amplitude + phase representation signal is related to the time-domain (continuous wave) representation just by an inverse Fourier transform. How are the amplitude+phase signals actually measured? Are they just the real FFT of an actually measured continuous wave? That would mean that the "time points" in the data wouldn't even really be time points at all, and in order to do any meaningful processing the first thing you would do is have to convert to the time domain anyway (otherwise nothing will work correctly).
So what are the dimensions of the amplitude+phase data on disk? Is it something equivalent to (n_channels, 2, n_freqs)
where (2) is amplitude, phase?
This matters because, if the last dimensions is not time, then it breaks a lot of assumptions about what raw data are in MNE, and I wonder if it would be possible to immediately just convert it to time-domain continuous wave during read (would require preload=True
but might be okay).
(i.e., I'd like to first decide if we need to support the frequency domain format before deciding the best way to do so, or if we can instead get away with just ensuring all raw NIRS data is either continuous wave, or converted to continuous wave on read)
... and sorry if you already discussed this in the other issue / PR, if so let me know and I'll read through it
I'd like to first decide if we need to support the frequency domain format before deciding the best way to do so
Absolutely, that’s why I thought I’d raise an issue rather than start working on a PR.
and sorry if you already discussed this in the other issue / PR
Some discussion had happened but it got lost to me amongst the other discussions. That’s why I thought to raise this explicitly. If there is relevant discussion from the other PR we should post it here so its easily accessible to everyone making the decision.
Naively a frequency-domain amplitude + phase representation signal...
I am going to leave these questions to @kylemath, our resident frequency domain NIRS (FD-NIRS) expert. I know about continuous wave NIRS (CW-NIRS), Kyle has explained details about FD to me before, so best to hear it from the professional. But its worth nothing that these FD machines operate in a fundamentally different way to continuous wave machines, I find the following figure handy to remind myself about the differences in the system, but again, Kyle can explain better than me.
My only strong preference is that if we do add additional types, that we keep them as close to SNIRF as possible.
@kylemath Would these two data types be sufficient for you? Maybe you only need fnirs_phase, but I think its easiest to just add both at once.
Yes we have three types, DC, AC, and Phase, so this would be perfect
The data files are no different from EEG, MEG, or fNIRS @larsoner, channel by time by three data types.
Long answer:
The machine modulates a light source at say 40 MhZ, so it pulses in a light that is increasing and decreasing in intensity at 40 MhZ. The light goes through the head, and arrives at detectors nearby, along with other room and ambient light. The detectors bias is also modulated at the same phase locked 40 MhZ speed as the light itself, PLUS a heterodyne frequency (beat frequency) of say 23 KhZ. The source and detector MhZ frequencies cancel out and the MhZ frequency is left in the output of the detector to become digitized by A to D.
A to D digitizes this Signal with is an oscillating signal in the Mhz range, plus other room noise, then RECORDING SOFTWARE runs the FFT, pulls out the power and the phase of the MhZ of the source (our new systems actually have four different modulation frequencies that can be co-localized on the head and be separated by this FFT).
This power (AC) and phase measurement, plus the power of the overall spectra (DC), is recorded into data files, for each time step (as fast as 4 ms sampling periods).
no strong feeling on my side
No strong opinion either, do whatever the NIRS experts think that's best. If you feel that changing an existing type makes it more consistent please go for it. I don't think that many people use MNE with NIRS already.
A to D digitizes this Signal with is an oscillating signal in the Mhz range, plus other room noise, then RECORDING SOFTWARE runs the FFT, pulls out the power and the phase of the MhZ of the source (our new systems actually have four different modulation frequencies that can be co-localized on the head and be separated by this FFT).
This power (AC) and phase measurement, plus the power of the overall spectra (DC), is recorded into data files, for each time step (as fast as 4 ms sampling periods).
Ahh okay, so when you say it runs the FFT, it does this every ~4 ms after having collected data at a very high sampling rate (MHz) range, then from that FFT of ~4 ms of data extracts the amplitude and phase, so you end up with a time-varying (at the same sample rate as the rest of the data in the file) estimate of the amplitude and phase of this signal.
Now things make more sense, and I'll just work from @rob-luke's original proposal:
Is it going to break backwards compatibility to rename fnirs_raw, or are we best leaving as is and noting in comments that it corresponds to SNIRF fnirs_amplitude
If we are to change fnirs raw, do we add a new coil type number? Or just rename the existing 302 value?
In MNE we can call this a bug fix and just rename it. I think we can also change FIFF-constants to have this new name. But it makes things less painful if we keep the existing number unchanged.
I have not proposed adding the SNIRF fluorescence types or diffuse correlation spectroscopy types as I dont know any commercial devices for these types or anyone that uses them. They can be added later. Or does someone want this?
Or the time-domain gated ones. We can always add it later, as long as we get the naming scheme correct now.
I propose that we update our types and modify the FIFF types here mne-tools/fiff-constants#21 to best match the SNIRF specification.
Then instead of
fnirs_amplitude
fnirs_ac_amplitude
fnirs_phase
I prefer
fnirs_cw_amplitude
fnirs_fd_ac_amplitude
fnirs_fd_phase
The two-letter prefix is helpful because if we ever need the time-domain gated one we can add fnirs_tdgated_amplitude
and there is no name collision or ambiguity about what fnirs_amplitude
is.
@rob-luke @kylemath you happy with this naming? If so, @rob-luke indeed feel free to make a PR to fiff-constants.
sounds good to me
@jkuziek note these new names planned
Im happy with @larsoner suggestions and will open a PR to make this change. Thanks all.
Once that PR is merged I will check nothing has broken in MNE locally and by opening a test PR to run the CI.
It won't break until we bump the commit hash in test_constants.py
cross referencing #7923
Originally MNE supported two types of NIRS data, oxygenated and deoxygenated (
fnirs_hbo
/fnirs_hbr
) haemoglobin. In #6674 we added support for continuous wave fNIRS and added thefnirs_raw
andfnirs_od
to represent the raw amplitude measurements and the derived optical density measurements.@kylemath is now adding support for a frequency domain fnirs device in #7717 which requires a phase measurement type.
Also since 6674 the fNIRS community has released a common data format called SNIRF. In this format they have consulted the community and taken in to consideration many different NIRS measurements methods including continuous wave, frequency domain, gated time domain, diffuse correlation spectrsocopy etc. They have settled on a standard set of measurement types listed here https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#appendix
I propose that we update our types and modify the FIFF types here https://github.com/mne-tools/fiff-constants/pull/21 to best match the SNIRF specification. This will also make my upcoming implementation of a SNIRF reader easier. For our derived types of optical density and hbo/hbr these already match the SNIRF spec, so need to change those.
Could we
fnirs_raw
tofnirs_amplitude
fnirs_ac_amplitude
andfnirs_phase
Questions
fnirs_raw
, or are we best leaving as is and noting in comments that it corresponds to SNIRF fnirs_amplitudeIf we get some agreement here I will open a PR for this change in https://github.com/mne-tools/fiff-constants