Fix psd_array_welch for good data spans shorter than n_per_seg (#13039)#14003
Fix psd_array_welch for good data spans shorter than n_per_seg (#13039)#14003CedricConday wants to merge 7 commits into
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
|
Hi Cedric, thank you for your contribution. Could you disclose your AI usage according to our adopted policy. I ran into this same issue in my analysis recently and ended up deciding that dropping short spans was the safer approach, so I would be rather cautious about shrinking noverlap, particularly without warning the user about this behaviour. |
|
Hi Carina, thanks for the careful review. On the AI disclosure — I've added it to the PR description per the policy: I'm an AI engineer and use Claude Code in my workflow; I find, fix, and test, then review and verify everything before opening it under my name. Happy to walk through any part of the reasoning. On the approach — I agree, silently shrinking Happy to sync with @drammock before this is finalized. On your open question — I don't want to guess at why Thanks! |
|
Quick follow-up on the history question — I traced it. In 2015 ( |
|
So if I'm understanding correctly, the problem is:
I lean toward considering this a SciPy bug: the user passed input that ought to work, and then scipy-internal code changed one value, then chokes because that value is no longer OK relative the other passed value. Would love @larsoner's opinion as to whether upstreaming this report makes sense. As for what to do about it on our end (mostly repeating what others have said here), we could:
I lean toward (3) or (4 & 3) |
…e-tools#13039) Address @drammock's review on mne-tools#14003: rather than dropping good-data spans shorter than n_per_seg, analyze each such span with nperseg shrunk to the span length and noverlap clamped below it (a per-span functools.partial overriding the values baked into _func). n_fft is unchanged, so all spans share one frequency grid; short spans just get coarser spectral resolution, which is surfaced via a warning. No data is discarded. This is the option-3 workaround from the PR discussion. The underlying SciPy behavior (clamping nperseg but not noverlap) may still be worth reporting upstream (option 4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyuFNWN45FNffpGwsC4Su7
|
Thanks @drammock — your root-cause read is exactly right: I've gone with option 3. Instead of dropping short spans, each span now gets a per-span Two things worth your call:
|
Yeah I think it's worth asking them if they consider it bug-ish and want it fixed.
I don't have a clear sense of what users would prefer here (it might vary). drop-segment-plus-warning is a bit more friendly I guess, but would still be catastrophic if a particular dataset was mostly/all short spans... but I guess in either case the remedy is "pass in a shorter custom window array" so maybe that's fine. |
|
Pushed ae6fcbf handling the array- Array/ndarray window + short span — went with the clear-error route. A named/tuple window is regenerated by SciPy at the shrunk Upstream (option 4) — I'll open a SciPy issue for the clamp- |
|
Opened the upstream SciPy issue: scipy/scipy#25608 — clamp- |
…e-tools#13039) Address @drammock's review on mne-tools#14003: rather than dropping good-data spans shorter than n_per_seg, analyze each such span with nperseg shrunk to the span length and noverlap clamped below it (a per-span functools.partial overriding the values baked into _func). n_fft is unchanged, so all spans share one frequency grid; short spans just get coarser spectral resolution, which is surfaced via a warning. No data is discarded. This is the option-3 workaround from the PR discussion. The underlying SciPy behavior (clamping nperseg but not noverlap) may still be worth reporting upstream (option 4).
ae6fcbf to
a06d907
Compare
…13039) When good data spans (between bad annotations) are shorter than n_per_seg, SciPy reduces nperseg to the span length but leaves noverlap unchanged, so a span shorter than n_overlap raised 'noverlap must be less than nperseg'. Reduce noverlap per-span to stay < nperseg (nfft unchanged so frequency bins match across spans). Adds a regression test.
Per maintainer feedback (CarinaFo): rather than shrinking n_overlap to fit good-data spans shorter than n_per_seg, drop them from the estimate and warn, since a single Welch window does not fit them and shrinking the window per-span mixes incompatible estimates. Raise a clear ValueError if every good span is too short. Replaces the earlier noverlap-clamp approach.
for more information, see https://pre-commit.ci
…e-tools#13039) Address @drammock's review on mne-tools#14003: rather than dropping good-data spans shorter than n_per_seg, analyze each such span with nperseg shrunk to the span length and noverlap clamped below it (a per-span functools.partial overriding the values baked into _func). n_fft is unchanged, so all spans share one frequency grid; short spans just get coarser spectral resolution, which is surfaced via a warning. No data is discarded. This is the option-3 workaround from the PR discussion. The underlying SciPy behavior (clamping nperseg but not noverlap) may still be worth reporting upstream (option 4).
A named/tuple window is regenerated by SciPy at the shrunk nperseg, so short good-data spans are handled transparently. An explicit ndarray window has a fixed length and cannot be shortened to match, which previously surfaced as a cryptic SciPy length-mismatch error. Detect this case and raise an actionable ValueError pointing the user at passing a shorter window array or reducing n_per_seg/n_fft. Adds a regression test.
a06d907 to
689c843
Compare
Reference issue
Fixes #13039.
What does this implement/fix?
When
bad_*annotations split the data into good spans, a span shorter thann_per_segpreviously raised from SciPy:Following @CarinaFo's review, such spans are now dropped from the PSD estimate with a warning rather than analyzed with a shrunken window — a single Welch window does not fit them, and shrinking the window per-span would mix incompatible estimates. If every good span is shorter than
n_per_seg, a clearValueErroris raised pointing the user to lowern_per_seg/n_fft.Test
test_psd_welch_short_span_droppedcovers both the warning-and-drop path (one short span among long ones) and the all-spans-too-shortValueError. Fulltest_psd.pyis green under-W error.Disclosure: I'm an AI engineer and use Claude Code in my workflow. All code here is hand-reviewed and the reasoning is mine; I'm happy to walk through any of it.