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

BUG:signal: Fix median bias in csd(..., average="median") #15611

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

yotarok
Copy link
Contributor

@yotarok yotarok commented Feb 17, 2022

This commit fixes the bug that wrongly computes _median_bias from the
number of frequency instead of the number of frames.
A test is added to check (at least) the same bias is applied for both
cases where x is y, and x is not y.

Reference issue

Closes #15601

What does this implement/fix?

The original CSD with average='median' returns results with incorrect scale when the return value is complex.
This PR fix this behavior and adds a test that calls csd 2 times with x is y setting and x is not y, but x==y setting.
The former expected to return real-valued array, where the latter expected to return complex-valued array (with zeroed imag-part).
This test fails if the scalefactor is wrongly applied depending on whether result is complex or not.

Additional information

This commit fixes the bug that wrongly computes _median_bias from the
number of frequency instead of the number of frames.
A test is added to check (at least) the same bias is applied for both
cases where `x is y`, and `x is not y`.
@ilayn ilayn changed the title BUG: Fix median bias in csd(..., average="median") (scipy#15601) BUG:signal: Fix median bias in csd(..., average="median") Feb 17, 2022
Copy link
Member

@ilayn ilayn left a comment

Choose a reason for hiding this comment

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

Excuse my blindness but I can't see the detail that makes the difference with the previous behavior; it seems like before code was

if condition:
    Pxy = (median with Some real imag stuff) 
    Pxy /= bias
else:
    Pxy = (median stuff)
    Pxy /= bias

and now division operation is taken out of the branches and applied at the end. Could you please clarify it for me?

@ilayn
Copy link
Member

ilayn commented Feb 18, 2022

The failures seem unrelated so in it goes. Thanks @yotarok @tupui

@ilayn ilayn merged commit 3355c5f into scipy:main Feb 18, 2022
@ilayn ilayn added this to the 1.9.0 milestone Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Scalefactors for signal.csd with average=='median' are different depending on whether x==y or not
3 participants