Skip to content

Update reduction data types for 2023.12 array API specification, update __array_api_version__ #1621

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

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

ndgrigorian
Copy link
Collaborator

This pull request updates reductions over floating point data to be performed in the input type by default as per the 2023.12 array API spec.

This pull request also changes __array_api_version__ to 2023.12. As cumulative_sum is a requirement of the 2023.12 spec, this PR should be redirected to master after the base branch is merged.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • If this PR is a work in progress, are you opening the PR as a draft?

Copy link

github-actions bot commented Mar 29, 2024

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_190 ran successfully.
Passed: 869
Failed: 9
Skipped: 92

@oleksandr-pavlyk
Copy link
Contributor

oleksandr-pavlyk commented Mar 29, 2024

I think the spec changed default output dtype of reduction operations only for the floating point input dtypes.

For integrals previous behavior is correct, but I will double-check.

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_191 ran successfully.
Passed: 869
Failed: 9
Skipped: 92

@oleksandr-pavlyk
Copy link
Contributor

I think the spec changed default output dtype of reduction operations only for the floating point input dtypes.

For integrals previous behavior is correct, but I will double-check.

Yes, the change to dtype=None behavior was made in data-apis/array-api#744, The PR works as specified though:

In [13]: def check(dt): return dpt.sum(dpt.zeros((10**3, 10**2), dtype=dt, device="cpu"), axis=0).dtype

In [14]: { inp:check(ty) for inp, ty in dpt.__array_namespace_info__().dtypes().items()}
Out[14]:
{'bool': dtype('int64'),
 'float32': dtype('float32'),
 'complex64': dtype('complex64'),
 'complex128': dtype('complex128'),
 'int8': dtype('int64'),
 'int16': dtype('int64'),
 'int32': dtype('int64'),
 'int64': dtype('int64'),
 'uint8': dtype('uint64'),
 'uint16': dtype('uint64'),
 'uint32': dtype('uint64'),
 'uint64': dtype('uint64')}

@oleksandr-pavlyk oleksandr-pavlyk self-requested a review March 29, 2024 12:26
@oleksandr-pavlyk
Copy link
Contributor

The "array_api_tests/test_special_cases.py::test_unary[signbit(x_i is NaN) -> True]" failure seems like a bug in the test suite.

This is the output of print(case, ": ", x.shape, x.strides, x._element_offset, x.dtype, x.device, res.shape, res.dtype, res.device, x, res, idx, hex(int(x_v[idx])), in_, out, binary(in_)) added to the test_unary function:

x_i is NaN -> True :  () () 0 float32 Device(opencl:cpu:0) () bool Device(opencl:cpu:0) nan False () 0x7fc00000 nan 0.0 01111111110000000000000000000000

The "0x7fc00000" is hexadecimal view into float32, and it indeed does not have the sign bit set.

I filed data-apis/array-api-tests#250

@ndgrigorian
Copy link
Collaborator Author

The "array_api_tests/test_special_cases.py::test_unary[signbit(x_i is NaN) -> True]" failure seems like a bug in the test suite.

This is the output of print(case, ": ", x.shape, x.strides, x._element_offset, x.dtype, x.device, res.shape, res.dtype, res.device, x, res, idx, hex(int(x_v[idx])), in_, out, binary(in_)) added to the test_unary function:

x_i is NaN -> True :  () () 0 float32 Device(opencl:cpu:0) () bool Device(opencl:cpu:0) nan False () 0x7fc00000 nan 0.0 01111111110000000000000000000000

The "0x7fc00000" is hexadecimal view into float32, and it indeed does not have the sign bit set.

I filed data-apis/array-api-tests#250

Yes, there are a couple more cases too: the copysign failure also seems to be incorrect, both inputs appear to be zero but the output is expected to be NaN, and the cumulative_sum test failure comes from an assumption that cumulative_sum(M) for M size 1 and 1D should return an array with empty shape (i.e., a scalar).

The prod failure is a result of a test not yet updated for the 2023.12 spec.

If these failures are still around come time to merge this branch, I will introduce them to the array API test skips file.

@oleksandr-pavlyk oleksandr-pavlyk force-pushed the feature/tensor-accumulation branch 2 times, most recently from b1219af to 4bd02b4 Compare March 31, 2024 16:39
@ndgrigorian ndgrigorian force-pushed the increment-array-api-version branch from 7704aa5 to 65f8e53 Compare March 31, 2024 23:46
Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_195 ran successfully.
Passed: 869
Failed: 9
Skipped: 92

@ndgrigorian ndgrigorian force-pushed the increment-array-api-version branch from 65f8e53 to d783a2c Compare April 1, 2024 06:25
Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_196 ran successfully.
Passed: 869
Failed: 9
Skipped: 92

Also changes docstrings in _array_api.py
Floating point data types are no longer promoted based on item size
@ndgrigorian ndgrigorian changed the base branch from feature/tensor-accumulation to master April 1, 2024 16:53
@ndgrigorian ndgrigorian changed the base branch from master to feature/tensor-accumulation April 1, 2024 16:58
@ndgrigorian ndgrigorian force-pushed the increment-array-api-version branch from d783a2c to aa033eb Compare April 1, 2024 16:59
@ndgrigorian ndgrigorian changed the base branch from feature/tensor-accumulation to master April 1, 2024 17:00
@ndgrigorian ndgrigorian force-pushed the increment-array-api-version branch from aa033eb to 0722cdf Compare April 1, 2024 17:12
@coveralls
Copy link
Collaborator

coveralls commented Apr 1, 2024

Coverage Status

coverage: 88.014% (+0.05%) from 87.967%
when pulling 0722cdf on increment-array-api-version
into 65bb9ef on master.

Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_198 ran successfully.
Passed: 869
Failed: 9
Skipped: 92

Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_163 ran successfully.
Passed: 869
Failed: 9
Skipped: 92

@ndgrigorian ndgrigorian marked this pull request as ready for review April 3, 2024 20:39
Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the a future PR, we could parametrize default dtype functions to take callables that return needed default type given q. In tests we can use mock functions to reach branches left out by CI runs.

@ndgrigorian ndgrigorian merged commit a0c2aac into master Apr 3, 2024
31 of 37 checks passed
@ndgrigorian ndgrigorian deleted the increment-array-api-version branch April 3, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants