Add multi-dimensional arrays support for existing elementwise tests #23

honno · 2021-09-29T15:15:44Z

This PR will implement the stub elementwise tests. Uses stuff from #18 so I just copied it's commit history for now—can rebase this PR to whatever merge/squash/rebase you do with that PR once it's in master.

I'll try starting from the bottom as you suggested. Right now there's just test_trunc, if you'd like to take a look.

array_api_tests/test_elementwise_functions.py

asmeurer · 2021-09-30T20:47:16Z

I actually meant work your way from the bottom up, fixing the existing tests (so starting with round). Writing new tests is a harder prospect, and will probably be easier once you see how the existing tests are written (although if you have any suggestions for improvements to the existing tests I'm all ears).

For trunc, we the test should be similar to the tests for floor, ceil, and round (in effect, those four functions implement the four types of rounding modes, round to infinity, negative infinity, even, and zero). Actually, I think the spec text is wrong for trunc. It says "Rounds each element x_i of the input array x to the integer-valued number that is closest to but no greater than x_i." but that actually describes floor (e.g., floor(-0.5) is -1.0 but trunc(-0.5) is 0.0). It should say "closest to 0".

asmeurer · 2021-09-30T20:49:44Z

Basically, I just want you to update the existing elementwise tests to accept arbitrary arrays. The ones written later (nearer the bottom of the file) will be easier to update because I wrote them with arbitrary arrays in mind, so I actually did things like used masks and iterated all indices where necessary. But for some of the tests nearer the top I did not, so they will need to be fixed to do that.

honno · 2021-10-05T13:01:58Z

So I've added ndarrays support for most of the existing tests. Many worked as-is, some I had to rework a tiny bit. I avoided the bitwise ones for now as they only support 0d arrays.

Notably I've aliased _array_module as xp, which I find significantly easier to use myself. Obviously _array_module isn't technically the array module (i.e. its stubbed and has a few goodies), so I do appreciate if you think this could be confusing. Subsequently I removed the array_helper imports which was just pointing to xp anyway. I think this is useful to distinguish what is a helper method and what is an Array API method... hopefully this is okay, not sure if I'm missing something.

asmeurer · 2021-10-05T20:25:12Z

Notably I've aliased _array_module as xp, which I find significantly easier to use myself. Obviously _array_module isn't technically the array module (i.e. its stubbed and has a few goodies), so I do appreciate if you think this could be confusing. Subsequently I removed the array_helper imports which was just pointing to xp anyway. I think this is useful to distinguish what is a helper method and what is an Array API method... hopefully this is okay, not sure if I'm missing something.

The idea at least in my head was that the array_helper would be the array functions that are allowed to be used in the tests, which would then be kept as minimal as possible. It's not ended up being as minimal as I had initially hoped, so I'm not sure if it was a great idea. Although we could definitely trim it down.

asmeurer · 2021-10-05T20:26:25Z

The bitwise tests will need to just iterate over all the elements using the ndindex helper, similar to test_equal.

honno · 2021-10-05T20:54:41Z

Notably I've aliased _array_module as xp, which I find significantly easier to use myself. Obviously _array_module isn't technically the array module (i.e. its stubbed and has a few goodies), so I do appreciate if you think this could be confusing. Subsequently I removed the array_helper imports which was just pointing to xp anyway. I think this is useful to distinguish what is a helper method and what is an Array API method... hopefully this is okay, not sure if I'm missing something.

The idea at least in my head was that the array_helper would be the array functions that are allowed to be used in the tests, which would then be kept as minimal as possible. It's not ended up being as minimal as I had initially hoped, so I'm not sure if it was a great idea. Although we could definitely trim it down.

Ah I see, it makes sense now thanks. Will revert this change.

The bitwise tests will need to just iterate over all the elements using the ndindex helper, similar to test_equal.

Sweet, I'll take a stab.

asmeurer · 2021-10-05T22:58:07Z

array_api_tests/test_elementwise_functions.py

-    x1, x2 = args
+@given(two_mutual_arrays(numeric_dtype_objects))
+def test_add(x1_and_x2):
+    x1, x2 = x1_and_x2


This is not ideal, but thinking about it, I think it's the best option. The alternative with shared stuff would just make the strategies really messy. It's really too bad that Python dropped the def f((x, y)) syntax that used to be allowed in Python 2.

Yep, was my thinking here. It's a shame matching that matching signatures of array module methods to their respective tests is quite tricky for a lot of these tests.

I wonder if two_mutual_arrays could instead be a function that returns two shared strategies. It would then be

@given(*two_mutual_arrays(numeric_dtype_objects)) def test_add(x1, x2): ...

It would make two_mutual_arrays a little more complex, but at least that complexity would be confined to a single helper function in the hypothesis helpers.

This works great, thanks!

array_api_tests/test_elementwise_functions.py

asmeurer · 2021-10-05T23:01:48Z

Why did GitHub Actions not run on this pull request?

asmeurer · 2021-10-05T23:06:27Z

I checked and the elementwise tests on this PR all pass against the NumPy implementation (except for floor_divide, which also has an issue in master, which is related to data-apis/array-api#264).

Prevents the test failing when using `pytest --max-examples=1`

honno · 2021-10-06T14:25:11Z

I've implemented ndarray support for the bitwise tests. Note I broadcast paired arrays before generating indices, like in test_equal (personally I get very confused with broadcasting heh). As I imagine it's not urgent, you or I can revisit it later.

I've ended up aliasing the following modules: array_helpers as ah, hypothesis_helpers as hh, and _array_module as xp. From there I reverted my previous changes, so now I call array helper imports instead of from the array module (e.g. I use ah.any instead of xp.any). Personally I find using these aliased namespaces makes it much easier to track from what module I'm using something from. What do you think?

I also now create xps in __init__.py, to not confuse a first-party Hypothesis thing with the hypothesis helpers (don't worry, your new linalg tests still work as-is). Per my previous paragraph, this means I can go from . import xps; xps(...) instead of import hypothesis_helpers as hh; hh.xps(...). Hope this is okay, I was just emulating the pattern the internal test suite for extra.array_api uses.

asmeurer · 2021-10-06T22:15:03Z

Broadcasting as in test_equal is fine. Fixing it requires generalizing ndindex a little bit. It probably isn't even that hard, but I didn't want to think about it.

Re the namespacing: being explicit about ah vs. xp is probably a good idea if we really do want to be explicit about the difference between them. The downside to namespacing is more typing, and also it breaks pyflakes. Having the array_helpers be a minimal subset used for the tests is mostly aspirational as I said, and will probably need to be looked at more closely in the future. I do always at least want to be thinking about what the dependencies of a test are, so that we can avoid unnecessary ones, and most importantly, so we can avoid making a test for a function implicitly depend on itself.

asmeurer · 2021-10-06T22:17:42Z

array_api_tests/meta_tests/test_hypothesis_helpers.py

@@ -54,10 +54,11 @@ def test_kwargs():
    results = []

    @given(hh.kwargs(n=st.integers(0, 10), c=st.from_regex("[a-f]")))
+    @settings(max_examples=100)


What is the purpose of this? Isn't max_examples already 100 by default? If we set the --max-examples flag at the command line (see conftest.py) which takes precedence?

Yep @settings takes precedence over everything else. I do this so this test should pass even when we use --max-examples=1. This is actually what a few tests in Hypothesis does, to prevent false-negatives.

I don't understand why you would ever pass max-examples=1. And even if you did, why wouldn't want it to apply to this test? I typically use max-examples to increase the number of examples to do a more rigorous test run.

Hmm, a few times now I've run pytest --max-examples=1 for the whole suite, so I thought forcing max_examples here would reduce noise for myself and generally prevent any noisy false-negatives if someone else had the same idea. Generally I think it clarifies that we're not using Hypothesis to test things, but testing Hypothesis itself i.e. see if our custom strategy will work for a "typical" run. I imagine both of these reasons are why Hypothesis does this internally too.

Does this test fail if it is run with max_examples=1?

Yep, as expected. In Hypothesis this kind of max_example forcing would be covered by their find_any util.

How this test_kwargs test method tries to find that certain things will be generated is essentially what find_any does in their own test suite.

I see. I missed that. I didn't notice that this was on an inner test function, not the outer one.

asmeurer · 2021-10-06T22:28:39Z

array_api_tests/test_elementwise_functions.py

+    xps.arrays(
+        dtype=hh.numeric_dtypes,
+        shape=xps.array_shapes(),
+        elements={"allow_nan": False, "allow_infinity": False},


Let's use masks like in the other tests.

We can think about whether restricting the inputs like this makes more sense than using masks. There are advantages and disadvantages to both ways. The main advantage is that some array libraries might not support masks, which would make the test suite in its current form mostly useless. But the disadvantage is that by doing this, we are technically testing less. Elementwise functions should operate on each element of an array independently, and the tests here check that. The exception is if the spec says that a certain value is not supported or may raise an exception. This sort of thing isn't spelled out for the elementwise functions as well is it could be (data-apis/array-api#199), so there's some degree that the spec needs to be improved here as well.

asmeurer reviewed Sep 30, 2021

View reviewed changes

array_api_tests/test_elementwise_functions.py Outdated Show resolved Hide resolved

honno force-pushed the elementwise-ndarrays branch from bef69cf to 6c5368f Compare October 4, 2021 10:41

honno changed the title ~~Implement stubbed elementwise tests~~ Add multi-dimensional arrays support for existing elementwise tests Oct 5, 2021

honno marked this pull request as ready for review October 5, 2021 12:54

honno requested a review from asmeurer October 5, 2021 13:02

asmeurer reviewed Oct 5, 2021

View reviewed changes

array_api_tests/test_elementwise_functions.py Outdated Show resolved Hide resolved

honno added 15 commits October 6, 2021 10:01

Implemented test_trunc

9d982d8

Improved test_trunc

4439c81

Test with ndarrays for test_round

1ff0af7

Use ndarrays for test_remainder

9f1d58c

Use ndarrays for test_pow

e002817

Use ndarrays for test_positive

f57d8c4

Use ndarrays for test_not_equal

652daa3

Use ndarrays for test_negative

e73866c

Use ndarrays for test_multiply

88118f0

Use ndarrays for logical tests, alias _array_module as xp

14f1dad

Use ndarrays for log tests

cd0a108

Use ndarrays for less/greater tests

6d440af

Use ndarrays for nan and inf tests

5adab4f

Use ndarrays for floor and exp tests

5ec583e

Use ndarrays for cos/ceil/divide tests

d6807b0

honno added 2 commits October 6, 2021 10:01

Use ndarrays for abs and some inverse trig tests

14b056b

Alias array module and helper modules

df5038a

honno mentioned this pull request Oct 6, 2021

Clean way to optional enable/disable extensions #25

Closed

honno added 4 commits October 6, 2021 10:46

Define xps in __init__.py

fb3ddb7

Force high max examples for test_kwargs

5531563

Prevents the test failing when using `pytest --max-examples=1`

Use ah.assert_integral in test_trunc

0bb4d65

Support and use ndarrays for bitwise tests

f7838d3

honno force-pushed the elementwise-ndarrays branch from e9abb15 to f7838d3 Compare October 6, 2021 14:08

honno requested a review from asmeurer October 6, 2021 14:25

asmeurer reviewed Oct 6, 2021

View reviewed changes

honno added 2 commits October 7, 2021 10:10

Use finite mask for test_trunc

5b0c49a

two_mutual_arrays() returns two strategies

3315e18

honno requested a review from asmeurer October 7, 2021 16:10

asmeurer merged commit 00e089b into data-apis:master Oct 7, 2021

honno deleted the elementwise-ndarrays branch October 21, 2021 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi-dimensional arrays support for existing elementwise tests #23

Add multi-dimensional arrays support for existing elementwise tests #23

honno commented Sep 29, 2021 •

edited

Loading

asmeurer commented Sep 30, 2021

asmeurer commented Sep 30, 2021

honno commented Oct 5, 2021 •

edited

Loading

asmeurer commented Oct 5, 2021

asmeurer commented Oct 5, 2021

honno commented Oct 5, 2021

asmeurer Oct 5, 2021

honno Oct 6, 2021 •

edited

Loading

asmeurer Oct 6, 2021

honno Oct 7, 2021

asmeurer commented Oct 5, 2021

asmeurer commented Oct 5, 2021

honno commented Oct 6, 2021

asmeurer commented Oct 6, 2021

asmeurer Oct 6, 2021

honno Oct 7, 2021

asmeurer Oct 7, 2021

honno Oct 7, 2021 •

edited

Loading

asmeurer Oct 7, 2021

honno Oct 7, 2021

honno Oct 7, 2021

asmeurer Oct 7, 2021

asmeurer Oct 6, 2021

Add multi-dimensional arrays support for existing elementwise tests #23

Add multi-dimensional arrays support for existing elementwise tests #23

Conversation

honno commented Sep 29, 2021 • edited Loading

asmeurer commented Sep 30, 2021

asmeurer commented Sep 30, 2021

honno commented Oct 5, 2021 • edited Loading

asmeurer commented Oct 5, 2021

asmeurer commented Oct 5, 2021

honno commented Oct 5, 2021

Choose a reason for hiding this comment

honno Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmeurer commented Oct 5, 2021

asmeurer commented Oct 5, 2021

honno commented Oct 6, 2021

asmeurer commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honno Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honno commented Sep 29, 2021 •

edited

Loading

honno commented Oct 5, 2021 •

edited

Loading

honno Oct 6, 2021 •

edited

Loading

honno Oct 7, 2021 •

edited

Loading