Fix mlab fallback for 32-bit systems #30273
Conversation
lib/matplotlib/mlab.py
Outdated
|
|
||
| if n == 1 and noverlap == 0: | ||
| return x[np.newaxis] | ||
| f'with noverlap < n and n <= x.size ({x.size})') |
There was a problem hiding this comment.
Given that I had read the original check wrong, this can be further simplified to something like if not 0 <= noverlap < n <= x.size: raise ValueError(f"n ({n}), noverlap ({noverlap}), and x.size ({x.size}) must satisfy 0 <= noverlap < n <= x.size")
Sorry for the very piecemeal review.
There was a problem hiding this comment.
Hmm, technically, noverlap doesn't need to be non-negative in the non-fallback/64-bit case. Maybe we should move this check up there earlier?
There was a problem hiding this comment.
Good point that noverlap can be negative here. Probably the semantically meaningful check should be to replace, in _spectral_helper, erroring when noverlap >= NFFT by erroring when not 0 <= noverlap < NFFT.
There was a problem hiding this comment.
Wasn't your point that _stride_windows doesn't require anything regarding noverlap and only requires 0 < n <= x.size (so the test in _stride_windows still needs to be relaxed), whereas _spectral_helper requires 0 <= noverlap < NFFT (which you have correctly changed?)
Unfortunately, I applied the change from matplotlib#29115 (comment) directly without noticing the typo, or running full tests. So fix the swapped condition, and add a test (for `csd` only, which should be enough since everything goes though `_spectral_helper`.)
There was a problem hiding this comment.
Still a minor point (#30273 (comment)) but it's not crucial and can be simplified later.
PR summary
Unfortunately, I applied the change from
#29115 (comment) directly without noticing the typo, or running full tests.
So fix the swapped condition, and add a test (for
csdonly, which should be enough since everything goes though_spectral_helper.) This test should work on 64-bit systems as well, but I also double-checked on WASM.PR checklist