Repository navigation
FIX: Reject averaged complex Welch coefficients - #14498
Conversation
Averaging Fourier coefficients across Welch segments mixes their phases, so the power recovered from them no longer matches output="power". Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
tsbinns
left a comment
There was a problem hiding this comment.
Thanks for the PR @iis-MarkKuang!
A couple comments and suggestions.
Keep the docstring to the new restriction, and check phase on each segment. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Shortened the |
tsbinns
left a comment
There was a problem hiding this comment.
Thanks @iis-MarkKuang! Just some tiny suggestions.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
tsbinns
left a comment
There was a problem hiding this comment.
@iis-MarkKuang Don't worry if you can't find a solution for the failing autofix check. I can look into this later today if it's still failing.
|
Thanks. The test jobs on this PR are green. The remaining failure is Autofix and style (prek), not the new tests. Happy for you to look at that one. |
|
@iis-MarkKuang it would actually be really helpful to us if you could try to solve the autofix error yourself. The message is: if you can't tell what needs to be done from the message (and looking at the docstring and the docdict), that is helpful for us to know so that we can make the error message more clear/explicit. |
|
Sorry for the back-and-forth @iis-MarkKuang. The fix ended up being a bit different to usual, so we have just pushed it. |
tsbinns
left a comment
There was a problem hiding this comment.
Thanks @iis-MarkKuang! Marking for merge when green.
Reference issue
Fixes #14495.
What does this implement/fix?
Welch
output="complex"withaverage="mean"or"median"averages the Fourier coefficients across segments. Their phases do not line up, so the power formed from the average is not the average of the power.normalizationis not involved: the one-sided factor of 2 already makes unaveraged coefficients matchoutput="power"after squaring and then averaging.average=Noneandaverage=Falsestill return one coefficient per segment.output="power"is unchanged.Additional information
AI assistance: Grok 4.7 in Cursor drafted the guard, the regression test, and this description, after I asked it to fix this issue the same way as #14493.