Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix mlab fallback for 32-bit systems#30273
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
base:main
Are you sure you want to change the base?
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Given that I had read the original check wrong, this can be further simplified to something likeif 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point that noverlap can be negative here. Probably the semantically meaningful check should be to replace, in _spectral_helper, erroring whennoverlap >= NFFT by erroring whennot 0 <= noverlap < NFFT.
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.
Did that then.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
b68335d toa5c273bCompareThere 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.
Still a minor point (#30273 (comment)) but it's not crucial and can be simplified later.
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`.)
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