Use old stride_windows implementation on 32-bit builds#29115
Use old stride_windows implementation on 32-bit builds#29115QuLogic merged 1 commit intomatplotlib:mainfrom
Conversation
|
It's not really clear to me why sliding_window_view (in the way we use it) would lead to an OOM while the manual approach wouldn't? |
|
Perhaps there is a NumPy calculation bug? It ends up as: |
|
Oh, I see, this is because the intermediate array is too large even though we slice it immediately (to compute the overlapping FTs); also it seems like numpy wants array.size * array.itemsize to be representable even though that may be much bigger than the physical array size. That seems overall related to the request for step_size at numpy/numpy#18244. I guess the easy way out is indeed to go back to as_strided. |
|
I thought mlab was being deprecated at some point. How useful is this to add this code piece, versus adding a |
|
Fair enough; I don't know what the status of the deprecations are at this point. I will say that this is reverting to the pre-#21190 code, so it's not new, and I've been using the patch on Fedora without issue since that PR, so it's been stable AFAICT. |
oscargus
left a comment
There was a problem hiding this comment.
To me, it seems more sense to put the comment about OOM in the 32-bit clause, but apart from that it makes sense to add this to simplify life for those running 32-bit systems.
bc3a3ba to
af49409
Compare
|
|
||
| x = np.asarray(x) | ||
|
|
||
| if n == 1 and noverlap == 0: |
There was a problem hiding this comment.
I think the special-case here can go away, it should be handled by the general case below.
lib/matplotlib/mlab.py
Outdated
| # np.lib.stride_tricks.as_strided easily leads to memory corruption for | ||
| # non integer shape and strides, i.e. noverlap or n. See #3845. | ||
| noverlap = int(noverlap) | ||
| n = int(n) |
There was a problem hiding this comment.
Nowadays I think it is more usual to error out if noverlap or n are not ints (or rather number.Integer).
There was a problem hiding this comment.
Indeed, both error out in the 64-bit case for floats.
lib/matplotlib/mlab.py
Outdated
|
|
||
|
|
||
| def _stride_windows(x, n, noverlap=0): | ||
| if noverlap >= n: |
There was a problem hiding this comment.
Given that this is just a backcompat function I would suggest grouping together all the checks for conciseness, something like
if not (isinstance(n, Integer) and isinstance(noverlap, Integer) and 1 <= n <= x.size and n < noverlap:
raise ValueError(f"n ({n}) and noverlap ({noverlap}) must be positive integers with n < noverlap and n <= x.size ({x.size})")There was a problem hiding this comment.
Oops, I merged and didn't notice the typo: #30273
anntzer
left a comment
There was a problem hiding this comment.
Minor nits, but nothing critical. Feel free to merge with or without them.
This was originally for i686 on Fedora, but is now applicable to WASM, which is 32-bit. The older implementation doesn't OOM.
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`.)
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`.)
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`.)
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`.)
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
I've long had the patch on Fedora (since #21190 (comment)), but it's now applicable to WASM as well (#29093), which is 32-bit. The older implementation doesn't OOM.
cc @anntzer as original author of that PR in case you have an alternate implementation idea.
PR checklist