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

[Feature] pairwise phase consistency #392

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

TRuikes
Copy link

@TRuikes TRuikes commented Dec 16, 2020

Hi!
In our lab we are using 'pairwise phase consistency' as a metric to measure spike-LFP interactions. The conventional method 'Phase Locking Value' can give faulty output if there are few spikes, and PPC improves on this. (function as implemented by authors. In short you compute the phase of an lfp during spike times (spike_triggered_phase) and then compute the angular distance between each pair of phases.

The output for the function phase_analysis.spike_triggered_phase can be directly fed into this function. While PPC can also be used to compute synchony between two EEG/LFP signals, I think there are allready other (better) measures for this and we can leave it at this simple implementation for now.

It can be a nice addition, let me know if you want to add this and if so what you would like me to change/add. In addition I suggest a tutorial on 'spike-field' functionality of elephant, briefly describing basic LFP processing steps (filtering, hilbert transform) and possible metrics/functions (spike_triggered_phase, sta.spike_field_coherence, sta.spike_triggered_average (<-that's PLV)).

@pep8speaks
Copy link

pep8speaks commented Dec 16, 2020

Hello @TRuikes! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 194:1: W293 blank line contains whitespace

Comment last updated at 2023-09-25 09:45:53 UTC

@coveralls
Copy link
Collaborator

coveralls commented Dec 16, 2020

Coverage Status

coverage: 86.606% (-0.4%) from 86.983% when pulling a1ff672 on TRuikes:master into c3eb21f on NeuralEnsemble:master.

@dizcza
Copy link
Member

dizcza commented Dec 16, 2020

Hi Thijs,
we're currently advancing phase statistics (see #385), and we do have PPC on the to-do list, thanks for bringing this!
We're also porting the documentation to the new BibTex citation documentation format, so I've updated your docs & reference in TRuikes#1. You can merge my PR here. I still don't see you in the authors list of Elephant!

The tests are missing. If you're lazy to write them, I can come up with something, though I'm not an expert in this field.

bibtex citation, doc style
@TRuikes
Copy link
Author

TRuikes commented Dec 16, 2020

Allright, thanks for the quick response! I'll think of some tests (wasn't too lazy, just didn't want spend time before knowing if you were going to use it ;) ) and add myself to authors.

The authors made impoved versions of PPC (this one they call 'ppc0' and then there are two more, I think). Long story short, when computing PPC between pairs, they dont want to draw samples from the same trial (their full reasoning written here, due to spiking properties of a neuron). To compute this I'd have to give information on trial nr per phase. I think I'll add an optional keyword trial_ids whith same dimensions as phases with a number per trial. Then another keyword to select ppc0 or ppc1.

I'm going to work on this next week again (monday), so I'll push something around then.

@dizcza
Copy link
Member

dizcza commented Dec 16, 2020

I think we don't need to multiply by 2. The matrix dot_prod is completely filled, so each pair 'i','j' is computed twice.

Yes, you're right, I didn't think of that.

Regarding your previous comment about ppc1, if you think that it could take much time or you're unsure how to implement this additional feature that potentially (!) improves the estimate, it's better to come up with another pull request that addresses the improvement after this PR is merged.

elephant/phase_analysis.py Outdated Show resolved Hide resolved
-changed np.tile to np.broadcast_to
-added dtype=np.float32

added tests in test_phase_analysis.py
@dizcza
Copy link
Member

dizcza commented Jan 11, 2021

Let me know when it's ready for review.

@TRuikes
Copy link
Author

TRuikes commented Jan 12, 2021

It's ready for review! I'm allready testing it on a (large) dataset and will let you know if something comes up.

@TRuikes
Copy link
Author

TRuikes commented Jan 12, 2021

If I'm supposed to do something please let me know

Copy link
Member

@dizcza dizcza left a comment

Choose a reason for hiding this comment

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

I created a PR again TRuikes#2

There was a todo "handle nans". Nans can occur iff the sum is zero and n_trials=1. In such cases, nans are the valid output.

Once the PR is merged, I merge this.

@dizcza dizcza changed the title Draft version of 'pairwise phase consistency' Added 'pairwise phase consistency' Jan 12, 2021
@dizcza dizcza added the new functionality New modules, functions label Jan 12, 2021
@mdenker mdenker added this to the v0.12.0 milestone May 21, 2021
@mdenker mdenker requested a review from rjurkus May 21, 2021 14:01
@Moritz-Alexander-Kern Moritz-Alexander-Kern removed this from the v0.12.0 milestone Mar 31, 2022
@Moritz-Alexander-Kern
Copy link
Member

Moritz-Alexander-Kern commented Jun 27, 2022

Hi @TRuikes ,
thank you for your persistent and steady work on this project to contribute to elephant.

Since I am taking over this PR, I am unfortunately not familiar with all the details.
Looking at the previous comment, this PR already received an approving review.
From my perspective the next steps are the following: (please correct me if wrong)

  • merge PR Fixed many small things about PPC0 TRuikes/elephant#2 (you are the only one with permissions to merge this PR on your fork)
  • finish the review process (1 pending review, this might entail further changes)
  • resolve possible conflicts with master (ask me, I am happy to do so)
  • merge this PR

If there is anything blocking progress on this project, that I could help to solve, please let me know, I am happy to do my part.

Update

Moritz-Alexander-Kern and others added 2 commits June 27, 2022 11:57
# Conflicts:
#	doc/authors.rst
#	doc/bib/elephant.bib
#	doc/reference/phase_analysis.rst
#	elephant/phase_analysis.py
Fixed many small things about PPC0
@TRuikes
Copy link
Author

TRuikes commented Jun 27, 2022

Hi @Moritz-Alexander-Kern , happy to help.
I've merged the PR. Since I created the code I have extended it with 'ppc1' (same metric but excluded pairs from the same trials).
Should I add it to this PR/script? It is a minor extension from the existing code, but possibly you prefer to wrap this up first.

I talked to @rjurkus about this briefly, he mentioned there might be a need for more tests. I'm also happy to work on those, but that will be after september.

@Moritz-Alexander-Kern Moritz-Alexander-Kern removed the new functionality New modules, functions label Jun 27, 2022
@Moritz-Alexander-Kern Moritz-Alexander-Kern added the new functionality New modules, functions label Jun 27, 2022
@Moritz-Alexander-Kern
Copy link
Member

Hi @TRuikes ,
To keep the whole thing neatly organized, it seems to make sense to merge this as one PR.
So I suggest to continue on this after september, otherwise we would have to scatter the PPC1extension and additional tests across multiple PRs.
Nice work so far, looking forward to continue working on this with you.

@Moritz-Alexander-Kern Moritz-Alexander-Kern added new functionality New modules, functions and removed new functionality New modules, functions labels Jun 30, 2022
@Moritz-Alexander-Kern Moritz-Alexander-Kern changed the title Added 'pairwise phase consistency' [Feature] pairwise phase consistency May 2, 2023
@Moritz-Alexander-Kern Moritz-Alexander-Kern added this to the v0.14.0 milestone Sep 18, 2023
@Moritz-Alexander-Kern Moritz-Alexander-Kern removed this from the v0.14.0 milestone Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new functionality New modules, functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants