Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use old stride_windows implementation on 32-bit builds#29115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
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 atnumpy/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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 toaf49409Compare| x = np.asarray(x) | ||
| if n == 1 and noverlap == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nowadays I think it is more usual to error out if noverlap or n arenot ints (or rather number.Integer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, both error out in the 64-bit case for floats.
lib/matplotlib/mlab.py Outdated
| @@ -210,6 +211,30 @@ def detrend_linear(y): | |||
| return y - (b*x + a) | |||
| def _stride_windows(x, n, noverlap=0): | |||
| if noverlap >= n: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Given that this is just a backcompat function I would suggest grouping together all the checks for conciseness, something like
ifnot (isinstance(n,Integer)andisinstance(noverlap,Integer)and1<=n<=x.sizeandn<noverlap:raiseValueError(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, I merged and didn't notice the typo:#30273
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
4c345b4 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Unfortunately, I applied the change frommatplotlib#29115 (comment)directly without noticing the typo, or running full tests.So fix the swapped condition, and add a test (for `csd` only, whichshould be enough since everything goes though `_spectral_helper`.)
Unfortunately, I applied the change frommatplotlib#29115 (comment)directly without noticing the typo, or running full tests.So fix the swapped condition, and add a test (for `csd` only, whichshould be enough since everything goes though `_spectral_helper`.)
Uh oh!
There was an error while loading.Please reload this page.
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