data-apis / array-api-tests

Test suite for the PyData Array APIs standard
https://data-apis.org/array-api-tests/
MIT License
64 stars 41 forks source link

Fix `test_solve` generating arrays larger than our max array size #242

Closed honno closed 7 months ago

honno commented 7 months ago

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!

asmeurer commented 7 months ago

With this PR, I get the following error https://gist.github.com/asmeurer/99eabdef967432ab2ab36ef36a34d6f4 from running

ARRAY_API_TESTS_MODULE=numpy.array_api pytest array_api_tests/test_linalg.py -k solve --max-examples=1000

with hypothesis 6.99.2.

asmeurer commented 7 months ago

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 arrays perhaps hypothesis should have a separate check for this in arrays, or else intercept the error there and translate it to a more meaningful one.

Zac-HD commented 7 months ago

If the source of this error is passing too large of a shape to arrays perhaps hypothesis should have a separate check for this in arrays, or else intercept the error there and translate it to a more meaningful one.

So far as I can tell, this strategy does not go through arrays().

asmeurer commented 7 months ago

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 arrays code that looked like it was what was generating the long list (but I also found it very difficult to tell what was going on inside of the hypothesis stack in the debugger).

honno commented 7 months ago

If the source of this error is passing too large of a shape to arrays perhaps hypothesis should have a separate check for this in arrays, or else intercept the error there and translate it to a more meaningful one.

So far as I can tell, this strategy does not go through arrays().

Looks like our hh.invertible_matrices() strategy does use arrays() and like before there seemed to be a case where the final array shape could exceed our max size, so pushed another fix if you could take a gander @asmeurer.

ARRAY_API_TESTS_MODULE=numpy.array_api pytest array_api_tests/test_linalg.py -k solve --max-examples=1000

On my end I get a failed deadline health check for this, both before and after my changes :thinking: When ignoring the deadline (from hypothesis import settings; @settings(deadline=None)) the test passes locally both before and after my changes too.

asmeurer commented 7 months ago

Found the line in hypothesis I was thinking about. It's actually in the traceback in the gist I posted

    |   File "/Users/aaronmeurer/miniconda3/envs/array-apis/lib/python3.11/site-packages/hypothesis/extra/array_api.py", line 369, in do_draw
    |     elems = data.draw(
    |             ^^^^^^^^^^

If I am reading this correctly, arrays does indeed use lists https://github.com/HypothesisWorks/hypothesis/blob/5eeb51ad38a44fefdc4b82dc0062487968ee089a/hypothesis-python/src/hypothesis/extra/array_api.py#L370

asmeurer commented 7 months ago

On my end I get a failed deadline health check for this, both before and after my changes 🤔 When ignoring the deadline (from hypothesis import settings; @settings(deadline=None)) the test passes locally both before and after my changes too.

I don't know. Maybe it requires a lot of examples. Indeed if invertible_matrices alone can cause this I've never seen it in other tests that use it. I just ran a whole bunch of examples for test_solve and I don't see this any more in this branch.

By the way, in the test suite you can disable the deadline with --disable-deadline.

honno commented 7 months ago

I just ran a whole bunch of examples for test_solve and I don't see this any more in this branch.

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 :sweat_smile:

asmeurer commented 7 months ago

I did get some deadline errors, but I ignored them.