seqasim / LFPAnalysis

6 stars 4 forks source link

Bug in oscillation_utils compute_connectivity & compute_surr_connectivity_time for 'coh' metric only #63

Closed aliefink closed 2 weeks ago

aliefink commented 2 weeks ago

Error when running oscillation_utils.compute_connectivity for coherence (metric = 'coh').

There's a small bug in the backend code - on line 641 the mne.spectral_connectivity_time function call throws an error because rank and gc_n_lags passed as inputs, but shouldn't be defined when computing coherence. (Rank is set to None, but still throws an error.) Args rank and gc_n_lags need to be removed from spectral_connectivity_time calls in compute_connectivity and compute_surr_connectivity_time functions. I tested it manually and it worked after commenting out rank + gc_n_lags.

upstream analysis code that produced error:

pwise = oscillation_utils.compute_connectivity(subj_epochs.copy(), 
                                   band = freq_dict[band], 
                                   metric = metric, 
                                   indices = seed_to_target, 
                                   freqs = freqs, 
                                   n_cycles = n_cycles,
                                   buf_ms = buf_ms, 
                                   n_surr=n_surr,
                                   avg_over_dim='time',
                                   band1 = freq_dict[band],
                                   parallelize=True)

error message : spectral_connectivity_time() got an unexpected keyword argument 'rank'

code snippets producing error: --> 641 pairwise_connectivity = np.squeeze(spectral_connectivity_time(data=mne_data, 642 freqs=freqs[(freqs>=band[0]) & (freqs<=band[1])], 643 average=False, 644 indices=indices, 645 method=metric, 646 sfreq=mne_data.info['sfreq'], 647 mode='cwt_morlet', 648 fmin=band[0], fmax=band[1], faverage=True, 649 padding=(buf_ms / 1000), 650 n_cycles=n_cycles[(freqs>=band[0]) & (freqs<=band[1])], 651 rank=None, 652 gc_n_lags=7, 653 verbose='warning').get_data())

Also breaks in compute_surr_connectivity_time: [line 396]

        surr_conn = np.squeeze(spectral_connectivity_time(data=surr_mne, 
                                    freqs=freqs[(freqs>=band[0]) & (freqs<=band[1])], 
                                    average=False, 
                                    indices=indices, 
                                    method=metric, 
                                    sfreq=surr_mne.info['sfreq'], 
                                    mode='cwt_morlet', 
                                    fmin=band[0], fmax=band[1], faverage=True, 
                                    padding=(buf_ms / 1000), 
                                    n_cycles=n_cycles[(freqs>=band[0]) & (freqs<=band[1])],
                                     rank=None, 
                                     gc_n_lags=7,
                                    verbose='warning').get_data())
seqasim commented 2 weeks ago

Nice catch! Fixed with commit 637a890478b956869bc15450d2860a7152332f2f

seqasim commented 2 weeks ago

Actually, to clarify, this commit fixed an error with metric ='granger', where lags were not specified for the surrogate analyses. metric='coh' should be unaffected

aliefink commented 2 weeks ago

Still getting the same error for connectivity metric 'coh' spectral_connectivity_time() got an unexpected keyword argument 'rank' with pkg versions: mne==1.2.3 mne_connectivity==0.5.0

seqasim commented 2 weeks ago

you'll likely solve this by upgrading mne_connectivity > 0.5

aliefink commented 2 weeks ago

upgrading mne connectivity fixed this issue!