Repository navigation
Conversation
ev-br
left a comment
ev-br
left a comment
There was a problem hiding this comment.
Thank you for the PR!
Given that you seem serious about it (which is appreciated a lot indeed!), could you please refactor the tests to be less different from other tests here.
Specifically:
- filtering out
xp._UndefinedStubis probably not needed, since dtype listings should already react toARRAY_API_SKIP_DTYPESenv variable. - for
array rop scalartests, could you reuse thearray_and_py_scalarstrategy, which non-reflected operators use. arrays_no_scalarsis fishy, you probably want to usearraysinstead forarray rop array, and it'll generate both arrays and array scalars on numpy.- parametrization on
left_shape, right_shapelooks excessive---please double-check that the usual strategies generate broadcasted values. If not, that'd be an enhancement for both reflected and non-reflected ops (and having common ways of generating inputs helps making common enhancements)
Oh and, please don't let your bot speak for you. For example, I've no idea what this passage is meant to mean:
An isolated subtraction-order mutation passes the old scalar/forward checks and fails the new test.
| if func_name == "subtract" and dtype in dh.uint_dtypes: | ||
| left_elements = st.integers(3, 7) | ||
| x1 = data.draw( | ||
| hh.arrays_no_scalars(dtype=dtype, shape=left_shape, elements=left_elements), |
There was a problem hiding this comment.
This looks fishy TBH. Why do we need arrays_no_scalars at all. A usual arrays strategy should be enough for taking care of numpy array scalars, no?
Or are you saying that "other" tests miss some cases around broadcasting? Then please explain!
Also elements are handled in a non-standard way, could you make it closer to what other tests do for limiting the range of elements.
| @given(data=st.data()) | ||
| def test_reflected_array_operator(method, func_name, dtypes, left_shape, right_shape, data): | ||
| dtype = data.draw( | ||
| st.sampled_from([d for d in dtypes if not isinstance(d, xp._UndefinedStub)]), |
There was a problem hiding this comment.
I think dtype lists should already filter out undefined dtypes, via https://github.com/data-apis/array-api-tests/blob/master/array_api_tests/dtype_helpers.py#L109
Here and in make_reflected_scalar_params.
A simple way to check it is to run with torch, $ ARRAY_API_TESTS_SKIP_DTYPES=uint16,uint32,uint64 ARRAY_API_TESTS_MODULE=array_api_compat.torch ...., which is needed before merging anyway,
| elements = st.integers(1, 3).map(dh.get_scalar_type(dtype)) | ||
| left_elements = elements | ||
| if func_name == "subtract" and dtype in dh.uint_dtypes: | ||
| left_elements = st.integers(3, 7) |
| pytest.param((2, 1), (1, 3), id="broadcast"), | ||
| ], | ||
| ) | ||
| @given(data=st.data()) |
There was a problem hiding this comment.
Would be nice to draw the dtype here in @given not in data.draw, if possible.
Adds array/array coverage for the reflected elementwise operators in #301. Compares
a.__op__(b)andb.__rop__(a)for dtype, broadcast shape, and values, including 0-D arrays, while retaining the scalar cases.Validated with array-api-strict 2.6.1 and NumPy 2.4.6: 60 new cases passed per backend, plus 170 related tests with 5 existing remainder skips. Meta-tests: 70 passed, 1 existing skip; flake8 passes. An isolated subtraction-order mutation passes the old scalar/forward checks and fails the new test.