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 to PSP metric to account for unbiased cross-correlation #33

Closed
wants to merge 2 commits into from

Conversation

lauracarlton
Copy link
Contributor

Bug fixes in PSP

  • updated to match the qtnirs matlab implementation
  • unbiased scaling of the cross-correlation timeseries
  • calculate the power spectrum density instead of just power spectrum

Copy link
Contributor

@emiddell emiddell left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -106,6 +106,7 @@ def psp(
# Vectorized signal extraction and correlation
sig = windows.transpose("channel", "time", "wavelength", "window").values
psp = np.zeros((sig.shape[0], sig.shape[1]))
lags = np.arange(-nsamples + 1, nsamples)

for w in range(sig.shape[1]): # loop over windows
Copy link
Contributor

Choose a reason for hiding this comment

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

use sig.sizes['time'] for clarity


corr /= np.tile(norm_factor, (corr.shape[1],1)).T
# corr /= np.tile(norm_factor, (corr.shape[1],1)).T

for ch in range(sig.shape[0]):
Copy link
Contributor

Choose a reason for hiding this comment

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

same: sig.sizes['channel']


corr /= np.tile(norm_factor, (corr.shape[1],1)).T
# corr /= np.tile(norm_factor, (corr.shape[1],1)).T

for ch in range(sig.shape[0]):
window = signal.windows.hamming(len(corr[ch,:]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think corr[ch,:] has the same length for all channels and windows. So this does not need to be recomputed and could be moved out of the two loops.

@emiddell
Copy link
Contributor

emiddell commented Sep 3, 2024

Hi @lauracarlton, one more idea how we could avoid the two nested loops over windows and channel. Welch's method improves the estimate of the spectral density by using a sliding window approach on the passed time series. This we don't use. nperseg and nfft are both set to the length of the whole time series (here the window length len(corr[ch,:]). So from signal.welch we only use the application of the Hamming window and the scaling of the density estimate. If we apply these two things ourselves, we could use numpy's implementation of the fft for real valued signals numpy.fft.rfft. This function operates on n-dimensional arrays and one can specify via the axis parameter along which array the fft should be calculated. Should speed things up.

@lauracarlton
Copy link
Contributor Author

Thanks for that idea @emiddell ! I can try implementing it and see how it goes:) I realized yesterday I am still getting different answers than the qt-nirs toolbox is returning. Meryem found this forum on the mne-nirs page
mne-tools/mne-nirs#503
Our implementation is a little different than how mne does it but I was using the same method for calculating PSD as they are so I think I am running into a similar issue documented there. Maybe your implementation suggestion above could help address this.

@lauracarlton
Copy link
Contributor Author

I have attempted an implementation using the method you described above. I am still getting different results compared to the qt-nirs matlab implementation ... Meryem suggested we reach out to Luca Pollonini to see if he has any ideas. Let me know if something jumps out to you though.

@emiddell
Copy link
Contributor

emiddell commented Sep 3, 2024

In the linked thread it was pointed out that the difference between qt-nirs and mne-nirs could stem from the former calculating the power spectrum and the latter calculating the power spectral density

Scipy's different scaling constants to get the PSD or the power spectrum are defined here.

Your current implementation seems to calculate the PSD, too. Hence, I would expect that you get other result than qt-nirs.

@emiddell
Copy link
Contributor

emiddell commented Sep 7, 2024

Changes were merged in e802f72.

@emiddell emiddell closed this Sep 7, 2024
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