Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs #210

seqasim · 2024-06-28T21:16:58Z

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:

…arallel_con

for more information, see https://pre-commit.ci

tsbinns · 2024-08-01T21:28:42Z

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 · 2024-08-01T21:37:39Z

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

mne-connectivity/mne_connectivity/spectral/time.py

Lines 72 to 75 in ef0a484

    
               method : str | list of str 
        
                   Connectivity measure(s) to compute. These can be ``['coh', 'cacoh', 
        
                   'mic', 'mim', 'plv', 'ciplv', 'pli', 'wpli', 'gc', 'gc_tr']``. These 
        
                   are:

And also an equation entry like for coh:

mne-connectivity/mne_connectivity/spectral/time.py

Lines 245 to 256 in ef0a484

    
               **Supported Connectivity Measures** 
        
               The connectivity method(s) is specified using the ``method`` parameter. The 
        
               following methods are supported (note: ``E[]`` denotes average over 
        
               epochs). Multiple measures can be computed at once by using a list/tuple, 
        
               e.g., ``['coh', 'pli']`` to compute coherence and PLI. 
        
                   'coh' : Coherence given by:: 
        
                                | E[Sxy] | 
        
                       C = --------------------- 
        
                           sqrt(E[Sxx] * E[Syy])

tsbinns · 2024-08-01T21:58:07Z

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?

tsbinns

So just pretty minor comments. There are a couple formatting things (mainly line length) that I would have thought pre-commit would sort (if you've set that up). Would recommend doing so otherwise getting that CI check to pass will be very tedious...

mne_connectivity/spectral/time.py

mne_connectivity/effective.py

tsbinns · 2024-08-01T22:10:22Z

mne_connectivity/effective.py

+
+    Parameters
+    ----------
+    data : array-like, shape=(n_epochs, n_signals, n_times)


This should also include Epochs (again, I realise you took this from the existing function which doesn't have this, so should change there too).

mne_connectivity/tests/test_effective.py

seqasim · 2024-08-02T04:04:47Z

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 · 2024-08-02T11:46:40Z

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!

for more information, see https://pre-commit.ci

seqasim · 2024-10-31T18:20:57Z

I think I addressed everything here (including adding verbose arg) but tests still failing

tsbinns · 2024-11-11T23:03:40Z

Thanks for the changes @seqasim! I am quite busy at the moment, but I will try to do another round of reviews soon.
Looking at the CI logs, seems like the failures come from the verbose and fill_doc decorators not being completely redundant here, but from a quick look I'm not sure why.

for more information, see https://pre-commit.ci

seqasim · 2024-11-12T16:39:39Z

Ok, I think I resolved all failures. There were some dimension issues with the way I was extracting the cohy results, which are now fixed.

tsbinns · 2024-11-16T17:48:52Z

mne_connectivity/tests/test_effective.py

+    # conn = phase_slope_index_time(data, mode="fourier", sfreq=sfreq, freqs=np.arange(4))
+
+    # assert conn.get_data(output="dense")[1, 0, 0] < 0
+    # assert conn.get_data(output="dense")[2, 0, 0] > 0
+
+    # # only compute for a subset of the indices
+    indices = (np.array([0]), np.array([1]))
+    # conn_2 = phase_slope_index_time(data, mode="fourier", sfreq=sfreq, freqs=np.arange(4), indices=indices)
+
+    # # the measure is symmetric (sign flip)
+    # assert_array_almost_equal(
+    #     conn_2.get_data()[0, 0], -conn.get_data(output="dense")[1, 0, 0]
+    # )
+
+    freqs = np.arange(5.0, 20, 0.5)
+    conn_cwt = phase_slope_index_time(
+        data, mode="cwt_morlet", sfreq=sfreq, freqs=freqs, indices=indices
+    )


Including some of the things commented here would be nice to better reflect the checks happening for the regular PSI function.

So as well as checking that seed -> target connectivity is > 0, should also check that target -> seed connectivity is < 0 and they are identical but just sign-flipped.

tsbinns

Have pushed some changes to formatting and also documentation updates to reflect the spec_conn_time more. Also added the new function to the API file and main init file.

One thing which I think is still outstanding is an average parameter that controls averaging over epochs, since this behaviour can be controlled with spec_conn_time.
It should be a pretty simple case of just updating the documentation and adding the extra behaviour of packaging the PSI data in a SpectralConnectivity container (just like for spec_conn_time).
The current implementation of computing PSI from coherency should already be flexible w.r.t. an epoch dimension or not, but this could also be added as a check in the unit test.

I also still think it makes sense to expose the spec_conn_time smoothing options (sm_times, sm_freqs, and sm_kernel).
This would just be a simple case of just updating the docstring.

Once that's added and the other point about the unit test being extended, I think this would be ready!

seqasim and others added 8 commits June 28, 2024 14:53

added _cohy to spectral.time

2631a83

list cohy method in spectral.time

388ab8c

Mimic conditional statements for coh for cohy in _pairwise_con and _p…

c38374a

…arallel_con

initial add of phase_slope_index_time to effective.py

b44da50

remove unused code from phase_slope_index_time

75a72b5

fixing documentation for phase_slope_index_time

6cef3cb

added test_psi_time to test_effective

dffda01

[pre-commit.ci] auto fixes from pre-commit.com hooks

ef0a484

for more information, see https://pre-commit.ci

seqasim mentioned this pull request Jun 28, 2024

Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs? #178

Open

tsbinns reviewed Aug 1, 2024

View reviewed changes

seqasim and others added 4 commits October 31, 2024 14:13

address pull request comments

be0c3cd

address pull review comments

ce68935

fixed typo in effective.py

401a6a8

[pre-commit.ci] auto fixes from pre-commit.com hooks

573211d

for more information, see https://pre-commit.ci

seqasim and others added 6 commits November 12, 2024 09:50

removing 'names' from phase_slope_index_time args

a96db00

explicitly provide freqs to test_psi_time

ac393fe

fix freq dim in phase_slope_index_time

b5ab63b

fix shape req in test_psi_time

3c93427

[pre-commit.ci] auto fixes from pre-commit.com hooks

5dad254

for more information, see https://pre-commit.ci

Merge branch 'main' into main

8820b6f

Update for review

fd5738e

tsbinns reviewed Nov 16, 2024

View reviewed changes

Fix too long line

48e81a8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs #210

Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs #210

seqasim commented Jun 28, 2024

tsbinns commented Aug 1, 2024

tsbinns commented Aug 1, 2024

tsbinns commented Aug 1, 2024

tsbinns left a comment

tsbinns Aug 1, 2024

seqasim commented Aug 2, 2024

tsbinns commented Aug 2, 2024

seqasim commented Oct 31, 2024 •

edited

Loading

tsbinns commented Nov 11, 2024

seqasim commented Nov 12, 2024

tsbinns Nov 16, 2024

tsbinns left a comment

Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs #210

Are you sure you want to change the base?

Phase-slope index using spectral_connectivity_time instead of spectral_connectivity_epochs #210

Conversation

seqasim commented Jun 28, 2024

PR Description

Merge checklist

tsbinns commented Aug 1, 2024

tsbinns commented Aug 1, 2024

tsbinns commented Aug 1, 2024

tsbinns left a comment

Choose a reason for hiding this comment

tsbinns Aug 1, 2024

Choose a reason for hiding this comment

seqasim commented Aug 2, 2024

tsbinns commented Aug 2, 2024

seqasim commented Oct 31, 2024 • edited Loading

tsbinns commented Nov 11, 2024

seqasim commented Nov 12, 2024

tsbinns Nov 16, 2024

Choose a reason for hiding this comment

tsbinns left a comment

Choose a reason for hiding this comment

seqasim commented Oct 31, 2024 •

edited

Loading