-
Notifications
You must be signed in to change notification settings - Fork 45
Fix test_solve
generating arrays larger than our max array size
#242
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
With this PR, I get the following error https://gist.github.com/asmeurer/99eabdef967432ab2ab36ef36a34d6f4 from running
with hypothesis 6.99.2. |
The change here seems correct, though maybe we should get it fully working before merging. If the source of this error is passing too large of a shape to |
So far as I can tell, this strategy does not go through |
There's this line https://github.com/data-apis/array-api-tests/pull/242/files#diff-6056c0b3af9cd3ba66387432a17f5f36bbd54220419656441a8b01bcdc4df44bR619. Also, I'll need to check again tomorrow, but from what I recall from a week ago when I ran this through a debugger there was some line in hypothesis in the |
Looks like our
On my end I get a failed deadline health check for this, both before and after my changes 🤔 When ignoring the deadline ( |
Found the line in hypothesis I was thinking about. It's actually in the traceback in the gist I posted
If I am reading this correctly, |
I don't know. Maybe it requires a lot of examples. Indeed if By the way, in the test suite you can disable the deadline with |
So in any case, if you're not getting a failed health check now then you/I can merge this? And I'll mull over the other points 😅 |
I did get some deadline errors, but I ignored them. |
Resolves #238—the end side of the second
test_solve
array shape needed to be ceiled relative to the rest of the shape size, otherwise the array size could exceed our desired maximum!