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

Fix icoh normalisation term and add unit test #366

Merged
merged 7 commits into from
Oct 4, 2024

Conversation

tsbinns
Copy link
Member

@tsbinns tsbinns commented Oct 2, 2024

Two things in this PR:

  • Adds a unit test for the coherence feature, including a sanity check that the connectivity values lie in an expected range using simulated connectivity data.
  • Fixes a bug for icoh where it was not being normalised properly, causing connectivity values to not lie in the expect [-1, 1] range.

Comment on lines 88 to 89
if self.icoh:
self.icoh_val = np.array(self.Pxy / (self.Pxx * self.Pyy)).imag
Copy link
Member Author

Choose a reason for hiding this comment

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

Denominator term should have the square root taken. Unfortunately the strategy for coh where you square the numerator and take the absolute value doesn't work for icoh since we extract only the imaginary part and this info isn't properly preserved.

@tsbinns
Copy link
Member Author

tsbinns commented Oct 2, 2024

Another note, I would argue it is beneficial to expose the nperseg parameter. With a high sampling frequency like 500 Hz, nperseg=128 is completely fine, but as sampling frequency decreases, this can come very close to the max possible. The result is coherence values which converge towards 1 (i.e. perfect correlation) regardless of the data passed in, and icoh values which are completely erratic.

At least if the end-user has control over nperseg, they can reduce this to reflect their sampling frequency. Doing so decreases the spectral resolution, but at least the connectivity values are still valid.

self.f, self.Pxx = welch(x, self.sfreq, self.window, nperseg=128)
self.Pyy = welch(y, self.sfreq, self.window, nperseg=128)[1]
self.Pxy = csd(x, y, self.sfreq, self.window, nperseg=128)[1]

What are people's thoughts on exposing this param as a setting?

tests/test_coherence.py Outdated Show resolved Hide resolved
@timonmerk
Copy link
Contributor

Also @tsbinns, it definitely makes sense to expose nperseg. Tbh I did not even think about that, but especially for lower frequencies that will be necessary!

@timonmerk timonmerk merged commit ea669e2 into neuromodulation:main Oct 4, 2024
6 checks passed
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.

2 participants