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?)
There was a problem hiding this comment.
Well, _stride_windows is only called by _spectral_helper in the 32-bit case, and no other functions do. So I guess it doesn't matter much, and it can be dropped here.
QuLogic
force-pushed
the
mlab32
branch
2 times, most recently
from
b68335d to
a5c273b
Compare
anntzer
left a comment
anntzer
left a comment
There was a problem hiding this comment.
Still a minor point (#30273 (comment)) but it's not crucial and can be simplified later.
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`.)
3 participants
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