Fix get_psd_welch Nyquist bin averaging and infinite loop when overlap equals nfft - #851
Conversation
…== nfft Two independent defects in the same function. The accumulation loop sums nfft / 2 + 1 bins but the averaging loop only divides nfft / 2 of them, so the Nyquist bin at index nfft / 2 is summed across every segment and never divided by the segment count. Its value is inflated by exactly the number of segments, which means it grows with the length of the input for a stationary signal. get_band_power inherits the error for any band that reaches the Nyquist frequency. Argument validation accepts overlap == nfft. The segment loop then advances by nfft - overlap, which is zero, so pos never moves and the function spins forever. Rejecting overlap >= nfft turns a hang into INVALID_ARGUMENTS_ERROR. Also updates the Python, C# and Java docstrings, which described the valid range as 0 to nfft inclusive.
|
Both native changes look correct, and the reference comparison convincingly isolates the unaveraged Nyquist bin. Please add automated coverage before merge:
These are compact regressions for a process hang and silent numerical error, so the example-script coverage alone is not sufficient. It would also be worth checking that all public-language documentation consistently describes the valid range as |
Asserts overlap equal to nfft returns INVALID_ARGUMENTS_ERROR, and runs that call on a worker thread so a regression of the guard fails rather than hanging CI. Also asserts every Welch bin including Nyquist equals the mean of the per-segment PSD bins.
|
Added One thing worth calling out. The The Nyquist check uses no overlap so the segments are disjoint, which lets every Welch bin be compared against the mean of the per-segment On the docs: python, C#, java and swift all state |
|
Follow-up: I had only wired the new test into Both workflow files still parse. |
get_psd_welchhas two independent defects. Both are in the same function, so they are here together.The Nyquist bin is never averaged. The accumulation loop sums
nfft / 2 + 1bins across every segment, but the averaging loop divides onlynfft / 2of them. The Nyquist bin at indexnfft / 2is summed and never divided by the segment count, so it comes back inflated by exactly the number of segments. For a stationary signal that means the value grows without bound as you feed in more data, which is the opposite of what a Welch average is for.get_band_powerintegrates up todata_len - 1, so any band whose upper edge reaches the Nyquist frequency inherits the error.overlap == nffthangs the process. Argument validation accepts it, and the segment loop then advances bypos += (nfft - overlap), which is zero.posnever moves, the loop condition never goes false, and the call spins forever inside the library with no way for the caller to recover.The fix averages
nfft / 2 + 1bins instead ofnfft / 2, and rejectsoverlap >= nfftinstead ofoverlap > nfft. Two characters each. The Python, C# and Java docstrings said the valid range was 0 to nfft inclusive, so they are corrected too.Verified by building
libDataHandler.dylibon macOS arm64 and calling the C API directly, comparing against a reference Welch average computed by callingget_psdon each segment and averaging every bin including Nyquist.Nyquist bin,
nfft=64,overlap=0,sampling_rate=256, signal containing a tone exactly at the 128 Hz Nyquist frequency:The before to reference ratio is 1.0000, 2.0000, 3.0000 up to 8.0000, exactly the segment count. Every other bin already matched the reference to 0.000e+00 absolute difference before the fix, which isolates the defect to the single Nyquist bin.
Propagation into
get_band_power, 6 segments:Bands that stop short of Nyquist are untouched. The band that reaches it was 3.44x too large.
For the hang,
get_psd_welch(data_len=256, nfft=64, overlap=64, ...)undertimeout 15was killed at 15016 ms with exit 124 before, and returns 13 (INVALID_ARGUMENTS_ERROR) in 9 ms after. Overlaps 0, 8, 16, 32 and 63 all still returnSTATUS_OKwith unchanged values.All 9 signal processing examples that CI runs pass on this branch.
clang-formatreports no changes on the touched files, anddotnet buildis clean at 0 warnings and 0 errors. The Java change is javadoc only and was not compiled since there is nomvnhere, but I checked it withjavac -Xdoclint:htmlafter rewording a bare<that would otherwise trip doclint.Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.