-
Notifications
You must be signed in to change notification settings - Fork 41
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
More improvements to test_linalg #101
Conversation
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.
Looks good.
Note that NumPy will fail this test without numpy/numpy#21084.
Ah, we should skip this in the workflow anywho because we're pinning against NumPy releases now.
Co-authored-by: Matthew Barber <quitesimplymatt@gmail.com>
The upstream has bugs that will be fixed in the next release.
@honno see what you think of the allclose helpers I've added here. They can likely use some improvement, but they should work for now. |
array_api_tests/array_helpers.py
Outdated
def allclose(x, y, rel_tol=0.25, abs_tol=1, return_indices=False): | ||
""" | ||
Return True all elements of x and y are within tolerance | ||
|
||
If return_indices=True, returns (False, (i, j)) when the arrays are not | ||
close, where i and j are the indices into x and y of corresponding | ||
non-close elements. | ||
""" | ||
for i, j in iter_indices(x.shape, y.shape): | ||
i, j = i.raw, j.raw | ||
a = x[i] | ||
b = y[j] | ||
if not (math.isfinite(a) and math.isfinite(b)): | ||
# TODO: If a and b are both infinite, require the same type of infinity | ||
continue | ||
close = math.isclose(a, b, rel_tol=rel_tol, abs_tol=abs_tol) | ||
if not close: | ||
if return_indices: | ||
return (False, (i, j)) | ||
return False | ||
return True |
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.
Yep looks good, this is really how we have to do it if we can't use masking.
Maybe move this and assert_allclose
to test_linalg.py
for now, as ideally what we'd be doing is standardising these utils across function groups then, and scoping these utils for each function group makes it easier to distinguish where they're used and their subtle differences. (standardising seems quite doable, I just need to get round to it)
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.
One problem here is represents a pretty significant performance hit. Before this commit (with the exact equality test), the linalg tests take 15 seconds on my computer. After, they take 44 seconds. Performance isn't our top priority, but maybe we should try array operations and only fallback to this when there are nonfinite values.
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.
Maybe move this and
assert_allclose
totest_linalg.py
for now, as ideally what we'd be doing is standardising these utils across function groups then, and scoping these utils for each function group makes it easier to distinguish where they're used and their subtle differences. (standardising seems quite doable, I just need to get round to it)
Think I'd still like this for now, esp as I'm generally rethinking the pytest helpers for #200/general look at vectorisation. Not blocking tho.
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.
Actually I'm just going to delete them. I'm not using them anymore, because testing float arrays from linalg functions like this turned out to be too difficult.
It looks like it isn't as straightforward as this. CuPy has several failures:
Note that the sort order of eigenvalues is unspecified in the spec. Also svd and svdvals fail with errors like
which shows that using an absolute tolerance is not going to work. There are also some test failures which I think are due to bugs in cupy. This one I'm not sure about >>> import cupy.array_api
>>> x1 = cupy.array_api.asarray([51307], dtype=cupy.array_api.uint16)
>>> x2 = cupy.array_api.asarray([[[327]]], dtype=cupy.array_api.uint16)
>>> cupy.array_api.linalg.matmul(x1, x2)
Array([[172]], dtype=uint16)
>>> cupy.array_api.linalg.matmul(x1, x2[0, ...])
Array([173], dtype=uint16)
>>> x1
Array([51307], dtype=uint16)
>>> import numpy as np
>>> np.asarray([51307*327], dtype=np.uint16)
array([173], dtype=uint16) CuPy is clearly doing something weird here, but should this overflow behavior be something that we should be avoiding in the test suite? If so then I guess we need to be more careful about how we generate integer array inputs to functions like matmul. |
Unfortunately I'm not familiar with linalg, so not super sure on expectations here. (I'll be out until 9th May, where I'll go through linalg and these tests more so I might be more useful in the future heh.)
Ah interesting catch. Personally I think it's fine to just relatively assert integer operations here (internal dtype-representation-shenanigans like numpy/numpy#20226 maybe?), as such edge cases probably won't affect array-consuming code. |
There are different equivalent ways of representing eigenspaces in general, and the same algorithm may choose different ways for stacked vs. non-stacked matrices (e.g., they differ for cupy).
This still is going to require some tweaking.
Floating-point dtypes now only test shape and dtype in the stacking test. Trying to make the allclose test work is too difficult for linear algebra functions.
array_api_tests/test_linalg.py
Outdated
# It's too difficult to do an approximately equal test here because | ||
# different routines can give completely different answers, and even | ||
# when it does work, the elementwise comparisons are too slow. So for | ||
# floating-point dtypes only test the shape and dtypes. | ||
|
||
# assert_allclose(x, y) | ||
|
||
assert x.shape == y.shape, f"The input arrays do not have the same shapes ({x.shape} != {y.shape})" | ||
assert x.dtype == y.dtype, f"The input arrays do not have the same dtype ({x.dtype} != {y.dtype})" |
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.
Yep this seems reasonable.
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.
Awesome LGTM, sans ndindex fixes and trivial linting issue. As far as I can tell you've utilised all the relevant helpers introduced since the PR was opened, which is grand to see!
array_api_tests/array_helpers.py
Outdated
def allclose(x, y, rel_tol=0.25, abs_tol=1, return_indices=False): | ||
""" | ||
Return True all elements of x and y are within tolerance | ||
|
||
If return_indices=True, returns (False, (i, j)) when the arrays are not | ||
close, where i and j are the indices into x and y of corresponding | ||
non-close elements. | ||
""" | ||
for i, j in iter_indices(x.shape, y.shape): | ||
i, j = i.raw, j.raw | ||
a = x[i] | ||
b = y[j] | ||
if not (math.isfinite(a) and math.isfinite(b)): | ||
# TODO: If a and b are both infinite, require the same type of infinity | ||
continue | ||
close = math.isclose(a, b, rel_tol=rel_tol, abs_tol=abs_tol) | ||
if not close: | ||
if return_indices: | ||
return (False, (i, j)) | ||
return False | ||
return True |
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.
Maybe move this and
assert_allclose
totest_linalg.py
for now, as ideally what we'd be doing is standardising these utils across function groups then, and scoping these utils for each function group makes it easier to distinguish where they're used and their subtle differences. (standardising seems quite doable, I just need to get round to it)
Think I'd still like this for now, esp as I'm generally rethinking the pytest helpers for #200/general look at vectorisation. Not blocking tho.
I've released ndindex 1.8 with the changes needed here, and updated the pin in requirements.txt. |
Can ignore flaky statistical tests (skipped in |
The test failure is because numpy.array_api is wrong for At any rate, I just remembered that I still wanted to check this with numpy, torch, and cupy. Although if you want to merge anyway, you can, and I'll just make any follow up fixes in another PR. |
Looks like the following causes NumPy main to hang >>> import numpy as np
>>> a = np.empty((3, 3, 0, 0), dtype=np.float32)
>>> np.linalg.cholesky(a, upper=True) The issue is only there with |
Submitted upstream numpy/numpy#25840. What's the easiest way to skip this test for now? (EDIT: looks like |
There are some other test failures with NumPy:
(need to investigate)
(also need to investigate)
|
Looks like it was fixed already 🎉
I wrote #235 to track this and other musings on our CI. Probably for this PR we're sticking with
Yep agreed can fix after, wrote #234 for this. Regarding |
Looks like the numpy solve failures are due to numpy/numpy#15349, which I guess was never addressed for 2.0. This is worked around in array-api-strict, but I never added a workaround to array-api-compat. |
The np solve issue in array-api-compat is fixed here data-apis/array-api-compat#93. Unfortunately, I don't think we can fix cupy as it doesn't seem to actually support stacking in its solve(). |
Here's the invertable_matrices strategy that is giving the health error for too much filtering: array-api-tests/array_api_tests/hypothesis_helpers.py Lines 306 to 318 in 4f83bb3
Is there a more direct way to generate arrays (of course, I should probably come up with some way to generate more interesting invertible matrices than just diagonal ones). |
Looks like the matrix_rank thing is a legitimate bug in the numpy implementation https://github.com/numpy/numpy/pull/25437/files#r1500043325. |
… linalg namespaces separately
@honno let me know what you think of the latest commit as a way to test functions like |
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.
let me know what you think of the latest commit as a way to test functions like matmul which should be in both the main and linalg namespace separately. Previously only the linalg one was tested, but I want to make sure that the compat library properly wraps things in both namespaces.
Yeah looks good, nice catch 👍
Is there a more direct way to generate arrays
d
s.t.all(abs(d) > 0.5)
?
Pushed a change which does this by passing a customer elements
strategy.
Lets merge as-is and resolve everything else in separate PRs 🎉
So far just vector_norm, but more are incoming.
Note that NumPy will fail this test without numpy/numpy#21084.
I also realized that the linalg tests should be making better use of the helper functions. I've done so for this test, but still need to update the other tests.