Skip to content

ENH: TDDR for Hb type fNIRS data #10125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
larsoner merged 12 commits into from
May 17, 2022
Merged

ENH: TDDR for Hb type fNIRS data #10125

larsoner merged 12 commits into from
May 17, 2022

Conversation

@HanBnrd
Copy link
Contributor

@HanBnrd HanBnrd commented Dec 13, 2021 •
edited
Loading

Reference issue

Closes #10124. Related to the #7556 PR.

What does this implement/fix?

Enables to apply the temporal derivative distribution repair (TDDR) algorithm on hemoglobin concentrations.

(FYI @rob-luke @frankfishburn)

@rob-luke
Copy link
Member

rob-luke commented Dec 13, 2021

Thanks @HanBnrd I am largely supportive of this, but just have a few practical questions.

  • Does the original article or code base state why they only ran TDDR on optical density? It is a very well considered algorithm, so I guess they must have had some reasoning. (we can wait a few days here incase Frank replies to your ping too)
  • I think we should we update the documentation to state that the original authors ran it the algorithm on optical density data, but MNE allows you to run in on hb data too.
  • And should we restrict the algorithm to run only on OD and Hb data as you suggest. Your current implementation will also run the algorithm on raw data. I understand that you may wish to run it on Hb data, as not everyone has OD signals. But anyone who has raw signals will have OD signals. So what is the benefit of allowing it to run on raw (I think we should try and stay as faithful to original author and try and encourage best practices in users)?

@HanBnrd
Copy link
Contributor Author

HanBnrd commented Dec 15, 2021

Hi @rob-luke,

Thanks for your comment.

I couldn't find if there is a specific reason for applying TDDR on OD data rather than other types. It is just mentioned in the article (Fishburn et al. 2019):

"These hemoglobin concentration signals were then converted back to raw intensity. Spike and baseline shift motion artifacts were then generated with a frequency of 2 spikes and 2 baseline shifts per minute."

related to how simulated data has been generated; and

"The resampled optical density signals were then either 1) submitted to direct application of TDDR, or 2) had only the low-frequency portion of the signal corrected using TDDR, as described in section 2.3. The corrected optical density signals were then converted to hemoglobin concentration using the modified Beer-Lambert relationship (Cope and Delpy, 1988)."

when describing the analysis methods. By taking into account how the algorithm works, this is why I believe this should be applicable to Hb type or even intensities. I believe that this is applied on OD initially since this technique is usually applied before bandpass filtering and researchers usually filter OD data.

Maybe it can also be useful to apply TDDR on intensity data. Since the average intensity is usually used to get the OD type data: deltaOD(t) = -log10(I(t)/I(average)), do you think it could be useful eventually to correct artefacts before the conversion so that the average intensity is not skewed by the motion artefacts? I don't know if that would make a big difference since relative changes are usually studied but maybe it can be useful to someone?

I agree though that it's probably better to stay as close as possible to the original work if @frankfishburn designed it for OD data specifically.

@rob-luke rob-luke added the fNIRS label Dec 20, 2021
rob-luke
rob-luke reviewed Dec 20, 2021
View reviewed changes
Copy link
Member

@rob-luke rob-luke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the suggested changes and restricting the application to od and hb data. Just a few wording comments. Otherwise, I think this is ready for broader review once the tests go green. Thanks @HanBnrd

@HanBnrd HanBnrd changed the title WIP: TDDR for Hb type fNIRS data ENH: TDDR for Hb type fNIRS data May 5, 2022
@HanBnrd HanBnrd marked this pull request as ready for review May 5, 2022 15:20
@HanBnrd
Copy link
Contributor Author

HanBnrd commented May 5, 2022

Hi @rob-luke,

Sorry it took so long but it should now be ready for review. I've seen you just modified the TDDR tests in a PR today, I had an issue having them pass with Hb data with the baseline shift initially implemented which is why I modified them too (this is why there's a conflict at the moment), let me know if you'd like me to use your new version (with 1.1 * shift_amp) instead of my fix.

Thanks :)

@rob-luke
Copy link
Member

rob-luke commented May 5, 2022

Great to hear from you @HanBnrd, I hope everything is going well for you.

Could you rebase off the main branch so we can easier see your proposed changes? It will be great to get this merged. Thanks for the persistence

@HanBnrd
Copy link
Contributor Author

HanBnrd commented May 5, 2022

I'm good, I hope you are well too @rob-luke! Thanks for checking this PR. This should be good with the rebase now.

larsoner
larsoner approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1 for merge

@rob-luke feel free to merge if you're happy

@larsoner
Merge remote-tracking branch 'upstream/main' into tddr-hb
bdfb2bb 
* upstream/main: (24 commits)
  Use less memory when loading EDF file (mne-tools#10638)
  BUG: fix ipython console accessibility after MNEQtBrowser in Spyder (mne-tools#10637)
  WIP: Fix mne.time_frequency.multitaper Nyquist adjustment slightly incorrect (mne-tools#10541)
  BUG: Fix bug with fNIRS reordering (mne-tools#10630)
  MNT: PyQt6 for pip pre 3.10 (mne-tools#10636)
  CI: Fix conda (mne-tools#10628)
  MRG: Update installer links to point to 1.0.3_0 (mne-tools#10622)
  [BUG, MRG] fix info write access (mne-tools#10626)
  [MRG] Fixed group_by titles in evoked_plot. (mne-tools#10618)
  [BUG, MRG] Replace image_interp with interpolation in topomap plot arguments (mne-tools#10617)
  MAINT: Fix timeout (mne-tools#10624)
  Simplify mne-tools#10619 (mne-tools#10620)
  Drop EEG rejection thresholds when replacing EEG with CSD channels (mne-tools#10619)
  Add get_montage support for fNIRS (mne-tools#10611)
  ENH: Improve make_head_surface options (mne-tools#10592)
  [ENH, MRG] Add citation for intracranial JOSS paper (mne-tools#10616)
  [ENH] Add sys info for mne-icalabel (mne-tools#10615)
  MRG: Add "highlight" parameter to Evoked.plot() to conveniently highlight time periods (mne-tools#10614)
  MRG: Allow to pass array of "average" values to plot_evoked_topomap() (mne-tools#10610)
  fix: snirf length units (mne-tools#10613)
  ...
@HanBnrd
Copy link
Contributor Author

HanBnrd commented May 17, 2022

Hi @larsoner and @rob-luke,

I've seen that some changes in the fNIRS tests have been reverted with #10630, should I replace the (1.1 * shift_amp) from the tests I added by just shift_amp?

@larsoner
Copy link
Member

larsoner commented May 17, 2022

In principle yes, in practice no because I already did that when I pushed a merge with upstream/main :)

@larsoner larsoner merged commit 575193c into mne-tools:main May 17, 2022
@larsoner
Copy link
Member

larsoner commented May 17, 2022

Thanks @HanBnrd !

@rob-luke
Copy link
Member

rob-luke commented May 18, 2022

This is great. Thanks @HanBnrd and @larsoner

@HanBnrd HanBnrd deleted the tddr-hb branch May 18, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rob-luke rob-luke rob-luke left review comments

@larsoner larsoner larsoner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable to apply TDDR on Hb type fNIRS data

3 participants

@HanBnrd @rob-luke @larsoner