Skip to content
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.

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

MRG: Add interpolation for NIRS signals #7428

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

rob-luke
Copy link
Member

Reference issue

Ongoing fNIRS improvements #7057

What does this implement/fix?

Adds the most basic type of "fixing" of bad channels, simply replaces them by the closest good channel.

Additional information

Maybe interpolation isn't the right word for what I wrote, but it seemed inline with previous functions. And I would like to expand to something smarter like actual interpolation

@rob-luke rob-luke force-pushed the fnirs_interpolate branch 2 times, most recently from d77e7f5 to 2403340 Compare March 12, 2020 08:44
@larsoner
Copy link
Member

Is there an established correct way to do this?

If not, I wonder if the spherical spline interpolation used in EEG would be good enough. Maybe the thing to do is to get a clean dataset that's good for testing, e.g., with five sensors arranged in an X pattern (maybe you already have this?). Then you could try interpolating the middle sensor just by taking the mean of the other four, or by using the edges of a Delaunay triangulation, or by spherical spline interpolation (what we use for EEG) and see which one explains the most variance of the original sensor data. If they are all close then we can just use the EEG code path.

@rob-luke
Copy link
Member Author

rob-luke commented Mar 12, 2020

Is there an established correct way to do this?

Absolutely not. You are massively overestimating the state of the field. In most of what I read people discard channels with bad scalp coupling index then do not discuss the impact of this. From talking to people, most studies create a grand average with different number of averages per channel (depending on how many channels were discarded).

What prompted me to write this code is that in the group analysis MNE will take only the channels that have no individual recording marked as bad. It is more common to have poor channels in NIRS than EEG, as the optodes don't have that nice pin like ending to get through peoples hair that EEG has. So when analysing a group of 10 recordings, you end up with no channels left.

If not, I wonder if the spherical spline interpolation used in EEG would be good enough.

Wonderful idea. I was actually hoping you would come up with a better solution than mine, and that's why I included the method parameter in the function definition. I can take a recording like this in the next few weeks.

This code to find the closest channel will also be useful when we write the short channel correction code, which in the basic approaches uses the closest short channel to each long channel to remove systemic responses.

@larsoner so do we keep this closest approach and then add a spline interpolation method later? Or hold off on this until I have time to collect the data and look at the spline method?

@larsoner
Copy link
Member

larsoner commented Mar 13, 2020

so do we keep this closest approach and then add a spline interpolation method later? Or hold off on this until I have time to collect the data and look at the spline method?

Let's wait until 0.20 is merged released to merge any PR that implements a method, then we have ~6 mo to decide what the default should be without needing a deprecation cycle. I'm then okay with merging nearest to start and keep PRs manageable.

@larsoner larsoner added this to the 0.21 milestone Mar 13, 2020
@rob-luke
Copy link
Member Author

Great. Sounds like a plan. I will keep working on this and ping you when it needs another look. Thanks

@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #7428 into master will increase coverage by 0.00%.
The diff coverage is 93.87%.

@@           Coverage Diff           @@
##           master    #7428   +/-   ##
=======================================
  Coverage   90.12%   90.12%           
=======================================
  Files         452      452           
  Lines       82800    82846   +46     
  Branches    13083    13093   +10     
=======================================
+ Hits        74626    74669   +43     
- Misses       5349     5350    +1     
- Partials     2825     2827    +2     

@rob-luke rob-luke force-pushed the fnirs_interpolate branch from 428c907 to a238a56 Compare April 3, 2020 07:04
@rob-luke
Copy link
Member Author

rob-luke commented Apr 3, 2020

@larsoner can you take another look at merging this now that 0.2 is released. Thanks

@rob-luke rob-luke changed the title WIP: Add basic interpolation for NIRS signals MRG: Add basic interpolation for NIRS signals Apr 3, 2020
@rob-luke rob-luke force-pushed the fnirs_interpolate branch from c9e703b to ad33da5 Compare April 4, 2020 06:09
@rob-luke rob-luke changed the title MRG: Add basic interpolation for NIRS signals WIP: Add interpolation for NIRS signals Apr 4, 2020
@rob-luke
Copy link
Member Author

rob-luke commented Apr 4, 2020

@larsoner could you please review again. I believe I have made the changes you requested. And the CI failure seems unrelated

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.

Also needs latest.inc update but you might want to wait a few minutes until other PRs are merged so you can rebase first

Also feel free to change the variable name as well as the string if you want (I only pushed a commit to change the string), and feel free to force push over my change if you want, too

@rob-luke rob-luke force-pushed the fnirs_interpolate branch from f203c1c to 59e431e Compare April 4, 2020 23:14
@rob-luke
Copy link
Member Author

rob-luke commented Apr 4, 2020

@larsoner my attempt at git magic has removed your commit!!! 😢

The code change you made is still there, but you credit via commit history has disappeared.

@rob-luke rob-luke changed the title WIP: Add interpolation for NIRS signals MRG: Add interpolation for NIRS signals Apr 4, 2020
@rob-luke rob-luke changed the title MRG: Add interpolation for NIRS signals WIP: Add interpolation for NIRS signals Apr 4, 2020
@rob-luke rob-luke changed the title WIP: Add interpolation for NIRS signals MRG: Add interpolation for NIRS signals 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.

No problem about the lost commit.

Just a couple more small things I noticed, don't need to be done here if you want to move on

@rob-luke
Copy link
Member Author

rob-luke commented Apr 5, 2020

Thanks I will fix these here

@rob-luke rob-luke force-pushed the fnirs_interpolate branch from 5b3881b to fb1b3e5 Compare April 5, 2020 23:32
@rob-luke
Copy link
Member Author

rob-luke commented Apr 6, 2020

@larsoner and @agramfort could you please review. I believe the failing test is unrelated.

@rob-luke
Copy link
Member Author

rob-luke commented Apr 6, 2020

Thanks for the feedback @agramfort

@larsoner larsoner merged commit 86d7988 into mne-tools:master Apr 6, 2020
@larsoner
Copy link
Member

larsoner commented Apr 6, 2020

Thanks @rob-luke

@rob-luke rob-luke mentioned this pull request Apr 6, 2020
19 tasks
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.

3 participants