search

mne-tools / mne-connectivity

Connectivity algorithms that leverage the MNE-Python API.
https://mne.tools/mne-connectivity/dev/index.html
BSD 3-Clause "New" or "Revised" License
68 stars 34 forks source link

Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs #210

Open seqasim opened 3 months ago

seqasim commented 3 months ago

Thanks for contributing. If this is your first time, make sure to read contributing.md

PR Description

Currently, the function for computing phase-slope index is hard-coded to be computed over epochs to produce time-resolved PSI, and there's no option for computing the phase-slope index over time to produce PSI per epoch. This would be useful if you think the average directionality between two signals over the entire timecourse is informative. You could compute this by binning your trials into categories (i.e. good trials v. bad trials) and then computing PSI, but there's no way to compute PSI per epoch and relate it to continuous trial-level predictors.

Here, I add a new function phase_slope_index_time to mne_connectivity.effective that compute PSI over time. I should note that I wasn't able to test this function directly as I'm having trouble getting the forked repo installed, in editable mode, with all necessary dependencies, but have written a test function in tests.test_effective to facilitate this.

Merge checklist

Maintainer, please confirm the following before merging:

tsbinns commented 2 months ago

Hi @seqasim, thanks for the PR and sorry for not getting back to you sooner on this. I have a few comments/suggestions which I'll post below, but it's a really good start!

If this is something you're still interested in working on, your contributions are of course very welcome, however I realise it's been a while. If you're not able to invest time into this anymore and you don't want your hard work to go to waste, I can pick things up and we can make sure the changes are added.

Just let us know. Cheers!

tsbinns commented 2 months ago

Just need to make sure that the new cohy method is described in the docstring for spectral_connectivity_time():

https://github.com/mne-tools/mne-connectivity/blob/ef0a4842d70ff784cbb3b913fc46033f25710ea2/mne_connectivity/spectral/time.py#L72-L75


And also an equation entry like for coh:

https://github.com/mne-tools/mne-connectivity/blob/ef0a4842d70ff784cbb3b913fc46033f25710ea2/mne_connectivity/spectral/time.py#L245-L256

tsbinns commented 2 months ago

A more general comment for the new phase_slope_index_time: Even if we are not averaging over epochs in the call to spectral_connectivity_time(), do you think it makes sense to add an average option which would allow averaging over epochs after PSI has been computed?

seqasim commented 2 months ago

Hi @seqasim, thanks for the PR and sorry for not getting back to you sooner on this. I have a few comments/suggestions which I'll post below, but it's a really good start!

If this is something you're still interested in working on, your contributions are of course very welcome, however I realise it's been a while. If you're not able to invest time into this anymore and you don't want your hard work to go to waste, I can pick things up and we can make sure the changes are added.

Just let us know. Cheers!

Hey,

Thanks for reviewing things! It's super illuminating for me to see the details I missed. I sadly won't have time to follow through on this for a few months, so if you feel you can wrap it up quickly, please don't wait for me! But if not, I'm happy to pick it up with all your helpful comments later in the year.

tsbinns commented 2 months ago

But if not, I'm happy to pick it up with all your helpful comments later in the year.

Yeah sounds good, there's no rush!