Skip to content

Conversation

@rob-luke
Copy link
Member

@rob-luke rob-luke commented Apr 3, 2020

Reference issue

As discussed in #7057

What does this implement/fix?

Adds temporal derivative distribution repair (TDDR) technique.
Adds new example showing how to apply nirs artefact correction (with plan to expand as new methods are implemented).

Additional information

I have taken the code directly from the author with permission.

I have kept the code as close to the original from the author as possible, and then used a wrapper function. I did this as the code was very nice. Also if the upstream version changes it is then easier to move the changes here.

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #7556 into master will decrease coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #7556      +/-   ##
==========================================
- Coverage   90.13%   90.12%   -0.02%     
==========================================
  Files         450      452       +2     
  Lines       82728    82800      +72     
  Branches    13076    13083       +7     
==========================================
+ Hits        74565    74621      +56     
- Misses       5342     5351       +9     
- Partials     2821     2828       +7     

@rob-luke
Copy link
Member Author

rob-luke commented Apr 3, 2020

Thanks for the early feedback @agramfort.

@rob-luke rob-luke force-pushed the fnirs_tddr branch 3 times, most recently from 6fb24ee to 04337a1 Compare April 4, 2020 03:32
@rob-luke
Copy link
Member Author

rob-luke commented Apr 4, 2020

@larsoner could you please review again. I think I made all your suggested changes and the CI segmentation error looks unrelated.

@rob-luke rob-luke changed the title WIP: Add temporal derivative distribution repair algorithm MRG: Add temporal derivative distribution repair algorithm Apr 4, 2020
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.

Just a few more minor comments

Example looks good. Would it be worth adding to tutorials/plot_70_fnirs_processing.py in addition to the example? If we're thinking of that as the gold standard for people to follow it seems like it, even if you just add a cross-link to the other example and the :func:.

if not len(pick_types(raw.info, fnirs='fnirs_od')):
raise RuntimeError('TDDR should be run on optical density data.')

raw._data = _TDDR(raw._data.T, raw.info['sfreq']).T
Copy link
Member

Choose a reason for hiding this comment

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

This will operate on all of the data even if there are non-fNIRS channels. So probably needs picks incorporated

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Good catch.

if len(signal.shape) != 1:
for ch in range(signal.shape[1]):
signal[:, ch] = _TDDR(signal[:, ch], sample_rate)
return signal
Copy link
Member

Choose a reason for hiding this comment

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

If this is how it operates, it would be simpler not to make this modification, and do the looping over picks directly in temporal_derivative_distribution_repair. It will hopefully keep this function just a copy-paste drop-in (?) and also takes care of the picks problem at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

This still required the name change to keep flake happy unfortunately.

@larsoner
Copy link
Member

larsoner commented Apr 4, 2020

... also needs a rebase

@rob-luke rob-luke changed the title MRG: Add temporal derivative distribution repair algorithm WIP: Add temporal derivative distribution repair algorithm Apr 4, 2020
@rob-luke
Copy link
Member Author

rob-luke commented Apr 5, 2020

@frankfishburn this code is ready to be merged in to MNE. You mentioned you probably wouldn't have time to take a look, so this is just an FYI.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

@rob-luke
Copy link
Member Author

rob-luke commented Apr 5, 2020

Thanks @agramfort

@larsoner larsoner merged commit 50f9f7b into mne-tools:master Apr 5, 2020
@larsoner
Copy link
Member

larsoner commented Apr 5, 2020

Thanks @rob-luke

larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 10, 2020
* upstream/master: (1522 commits)
  FIX: Show bug
  MRG, FIX: Datetime call in gdf 2.x age calculation (mne-tools#7581)
  DOC: Simplify Darwin installation (mne-tools#7584)
  MRG, ENH: Allow picking without preload (mne-tools#7507)
  DOC: Document anonymization better (mne-tools#7587)
  Rework _Brain show (mne-tools#7580)
  DOC: Fixes in tutorial (mne-tools#7579)
  ENH: muscle artifact detection (mne-tools#7407)
  MRG: Remove toolbars in PyVista plotter (mne-tools#7572)
  WIP: Deregister plotter from the figure list in close() (mne-tools#7573)
  MRG: Fix mouse wheel event in _TimeViewer (mne-tools#7563)
  FIX: Fix toggle all (mne-tools#7567)
  MRG, FIX: parallel n_jobs check (mne-tools#7566)
  Rename artifact detection to movement detection (mne-tools#7569)
  ENH: Update spelling check [ci skip] (mne-tools#7565)
  MRG, ENH: Dont require preload for raw resample (mne-tools#7508)
  MRG: Add interpolation for NIRS signals (mne-tools#7428)
  WIP: Add temporal derivative distribution repair algorithm (mne-tools#7556)
  DOC: fix link in docstr [skip ci] (mne-tools#7562)
  ENH: Custom figure title when plotting Dipole locations (mne-tools#7558)
  ...
larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 25, 2023
* upstream/master: (1522 commits)
  FIX: Show bug
  MRG, FIX: Datetime call in gdf 2.x age calculation (mne-tools#7581)
  DOC: Simplify Darwin installation (mne-tools#7584)
  MRG, ENH: Allow picking without preload (mne-tools#7507)
  DOC: Document anonymization better (mne-tools#7587)
  Rework _Brain show (mne-tools#7580)
  DOC: Fixes in tutorial (mne-tools#7579)
  ENH: muscle artifact detection (mne-tools#7407)
  MRG: Remove toolbars in PyVista plotter (mne-tools#7572)
  WIP: Deregister plotter from the figure list in close() (mne-tools#7573)
  MRG: Fix mouse wheel event in _TimeViewer (mne-tools#7563)
  FIX: Fix toggle all (mne-tools#7567)
  MRG, FIX: parallel n_jobs check (mne-tools#7566)
  Rename artifact detection to movement detection (mne-tools#7569)
  ENH: Update spelling check [ci skip] (mne-tools#7565)
  MRG, ENH: Dont require preload for raw resample (mne-tools#7508)
  MRG: Add interpolation for NIRS signals (mne-tools#7428)
  WIP: Add temporal derivative distribution repair algorithm (mne-tools#7556)
  DOC: fix link in docstr [skip ci] (mne-tools#7562)
  ENH: Custom figure title when plotting Dipole locations (mne-tools#7558)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants