-
Notifications
You must be signed in to change notification settings - Fork 45
Make test_eye more efficient #200
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
9decccd
to
e04bdb7
Compare
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.
Thanks for contributing, makes sense but one major blocker I've put in review comments.
Also test failures look irrelevant to this PR 👀
Thanks – I wonder how you'd feel about using |
e04bdb7
to
243dfea
Compare
OK, I removed references to |
Hmm, it looks like the approach in the recent commit fails to correctly error on mismatched signed zeros. It looks like the array API standard doesn't have anything akin to |
xref data-apis/array-api#670 - a stronger use case would help move that along, and also it looked like it wasn't available in JAX (maybe that was wrong?). |
|
Thanks for the link @rgommers! That would help to express this efficiently. That said, I still think the best solution for the test suite would be to convert things to numpy via dlpack and use existing utilities in |
Converting to numpy via DLPack only works well for CPU implementations though, there's no generic way to do it without introducing library-specific code for CuPy, PyTorch CUDA, etc. Of course there would be if That said: performance matters for this test suite. I also noticed last week that it was way slower than I had expected, and it makes it harder to use and contribute to. The |
Understood, but |
No, JAX uses |
I think the core issue here is
...and so
We could fix that by either (1) ensuring the Array API includes all elementwise operations required to validate its own specification, or (2) choose to validate those requirements in a more flexible reference implementation like (1) is blocked by the lack of |
I don't think the lack of
Good to know. In SciPy we can easily add JAX support that way (and I assume we'll get to adding JAX support in SciPy at some point in the first half of next year).
I think using it unconditionally is tricky for the non-CPU cases, but if there's no better option than we should. We need vectorized checks. I think we can make it non numpy specific without too much trouble. Also, as a data point, the SciPy I had a closer look at We probably need only a couple of functions. In SciPy we found so far that we needed |
Sounds good! Thanks for tackling this, @rgommers! |
We just need to be careful that we minimize the number of array functions that we actually use in a test helper. Because
If we are adding something like |
Thanks @asmeurer, that makes sense. If you're worried about circular reasoning and/or complexity in the testing utilities, I think the natural solution would be to find a strategy to export the data to numpy and use I'd point out that performance aside, the current approach has that issue as well: for example, if my array library has a bug where indexing always returns zero, I think that would be enough to make a lot of tests pass, even if the functions being tested are returning garbage! |
We need to take a pragmatic approach. Weird corner cases that wouldn't actually happen for a real array library where the tests would technically pass aren't that interesting. But helper functions using an array function that isn't implemented, causing every single test to fail is a very real concern. This sort of thing has already happened and it's why the test suite has some affordances for things like not every dtype being implemented (because pytorch doesn't implement uint64). We want to maintain the ability to use the test suite to produce reasonable results for the main NumPy or Pytorch namespaces (i.e., not just run against the compat library). Some functions, like Similarly, a test function that just uses itself to test itself would pass with any possible implementation, not just weird corner cases. For example, numpy/numpy#10322, is a wrong result from But if there's only a handful of these base functions, we can test everything else indirectly using them. |
I think we’re advocating for the same thing: an “export to numpy” function seems like it would be sufficient for all existing tests, and probably represents the minimum possible requirement in order to validate the API in a way that doesn’t lead to poor performance. |
I have half an implementation and will complete it, but it does use a fair amount of API surface, so it doesn't quite meet the requirements @asmeurer and @honno have. @jakevdp the current diff in this PR looks quite good. If you add |
Thanks! With the addition of |
I think we should be able to sort the decision on that out by Thursday evening, and then we can immediately add it. |
243dfea
to
89b2112
Compare
Closing this PR as its superseded by #236. Thanks for all the work and discussion here—utilising vectorised methods for assertions throughout the test suite is my number 1 priority! |
Part of #197