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

Median-averaging of complex CSDs #12669

Closed
hadrev opened this issue Aug 6, 2020 · 3 comments · Fixed by #12676
Closed

Median-averaging of complex CSDs #12669

hadrev opened this issue Aug 6, 2020 · 3 comments · Fixed by #12676
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.signal
Milestone

Comments

@hadrev
Copy link
Contributor

hadrev commented Aug 6, 2020

signal.csd does not median-average complex CSDs correctly because of unexpected behavior of np.median for complex-valued arrays (see the numpy issue). Line 589 of signal/spectral.py could be modified so that if the CSD is complex, its real and imaginary parts are separately median-averaged.

Reproducing code example:

import numpy as np
from scipy import signal

s = 0.3 * np.random.uniform(size=100000) # white noise
n1 = np.random.uniform(size=100000)
n2 = np.random.uniform(size=100000)
d1 = s + n1
d2 = s + n2
freq, Pss = signal.csd(s, s, nperseg=100) # mean average PSD of s
_, Pssmed = signal.csd(s, s, nperseg=100, average='median') # median average PSD of s
_, P12 = signal.csd(d1, d2, nperseg=100) # mean average CSD of d1 and d2
_, P12med = signal.csd(d1, d2, nperseg=100, average='median') # median average CSD of d1 and d2

The real and imaginary parts of P12 should have similar variances, but in the median-averaged case the variance of the imaginary portion is much too high.

sig_csd_mean
sig_csd_median

Scipy/Numpy/Python version information:

import sys, scipy, numpy; print(scipy.__version__, numpy.__version__, sys.version_info)
1.4.1 1.16.2 sys.version_info(major=3, minor=7, micro=3, releaselevel='final', serial=0)
@peterbell10 peterbell10 added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.signal labels Aug 6, 2020
@larsoner
Copy link
Member

larsoner commented Aug 6, 2020

@hadrev agreed they should be processed separately (not 100% sure it's correct but seems reasonable to me), would you be up for making a PR for this?

hadrev pushed a commit to hadrev/scipy that referenced this issue Aug 6, 2020
Separately median-average real and imaginary components of complex
CSDs. This avoids unexpected behavior when np.median is passed a
complex array.

This addresses scipy#12669
@tylerjereddy tylerjereddy added this to the 1.6.0 milestone Aug 7, 2020
@e-q
Copy link
Contributor

e-q commented Aug 7, 2020

Good catch @hadrev! It definitely looks like that the current behavior of np.median on complex arrays isn't what we want. There isn't a unique notion of a multivariate median, though, which is effectively what we're looking for in this case (See this wikipedia page).

I haven't looked at this carefully yet, but I think that for cross-spectra, we may in fact want something more like a geometric median rather than the medians of the real and imaginary parts of the array, as this may throw away correlations between the two.

@larsoner larsoner removed this from the 1.6.0 milestone Nov 9, 2020
@larsoner
Copy link
Member

larsoner commented Nov 9, 2020

No consensus on this so I'm removing the 1.6.0 milestone

hadrev pushed a commit to hadrev/scipy that referenced this issue Mar 7, 2021
Separately median-average real and imaginary components of complex
CSDs. This avoids unexpected behavior when np.median is passed a
complex array.

This addresses scipy#12669
larsoner pushed a commit that referenced this issue May 3, 2021
* BUG: update median averaging in signal.csd

Separately median-average real and imaginary components of complex
CSDs. This avoids unexpected behavior when np.median is passed a
complex array.

This addresses #12669

* Update CSD docstring to mention complex averaging

Average the real and imaginary parts separately.

Co-authored-by: hadrev <>
@tylerjereddy tylerjereddy added this to the 1.7.0 milestone May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.signal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants