Repository navigation
Fix mlab fallback for 32-bit systems - #30273
QuLogic merged 1 commit into
Conversation
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Did that then.
Sorry, something went wrong.
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?)
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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`.)
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 csd only, 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