Skip to content
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

Fixes element-wise comparisons of mixed signed-unsigned integer inputs #1650

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

ndgrigorian
Copy link
Collaborator

@ndgrigorian ndgrigorian commented Apr 22, 2024

This pull request addresses issues in comparison functions with mixed integral kinds (signed and unsigned) by explicitly adding overloads for combinations of uint64 and int64 and adding a unique weak-type resolver function for the comparisons.

The resolver function guarantees that negative Python scalars are not used to initialize an array with an unsigned integer data type in the binary comparisons, which makes comparisons between negative Python scalars and unsigned integer arrays work as expected.

Old behavior:

In [1]: import dpctl.tensor as dpt, dpctl, numpy as np

In [2]: x = dpt.arange(10, dtype="u8")

In [3]: -1 < x
Out[3]:
usm_ndarray([ True,  True,  True,  True,  True,  True,  True,  True,
              True,  True])

Now:

In [3]: -1 < x
Out[3]:
usm_ndarray([False, False, False, False, False, False, False, False,
             False, False])

Also slips in a change removing an unnecessary overload to not_equal for mixed float and complex float data types, which are safe for basic casting and not present in other elementwise comparisons.

  • 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?

Overloads for combinations of complex and real valued floats are unnecessary, as floats can be safely cast to complex
@ndgrigorian
Copy link
Collaborator Author

@oleksandr-pavlyk
This PR does not yet address similar issues in clip, minimum, and maximum.

For minimum and maximum a similar solution for negative Python scalars is possible. clip may be a bit trickier.

Copy link

github-actions bot commented Apr 22, 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_303 ran successfully.
Passed: 868
Failed: 10
Skipped: 92

Comparions between signed and unsigned integer data previously did not work correctly in some cases, as signed integers could be promoted to uint64 if one input was uint64

Additionally, `-1 < x` for some `x` with unsigned integer data type would always fail, as the -1 would initialize an array of `x.dtype` which would always underflow, leading to undefined behavior

These problems were addressed by adding signed and unsigned 64-bit integer combinations to the type maps for the comparisons, and adding constexpr branches to the comparisons between mixed-kind integers
@ndgrigorian ndgrigorian force-pushed the mixed-kind-integer-comparisons branch from 9429a64 to 2331c1c Compare April 22, 2024 22:14
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_304 ran successfully.
Passed: 870
Failed: 8
Skipped: 92

@coveralls
Copy link
Collaborator

coveralls commented Apr 22, 2024

Coverage Status

coverage: 88.218% (+0.008%) from 88.21%
when pulling b559f0a on mixed-kind-integer-comparisons
into 7757857 on master.

@ndgrigorian
Copy link
Collaborator Author

ndgrigorian commented Apr 23, 2024

@oleksandr-pavlyk
I've added tests and added the _is_weak_dtype utility function.

As of Numpy 2, Numpy will no longer permit converting Python integers to integer arrays where the Python integer is out-of-bounds for the integer array's data type (at least in ufuncs). This is how they resolve the issues with minimum, maximum, and clip with unsigned integer arrays and negative Python integers.

Imitating that behavior would align with the future of Numpy (and therefore should be acceptable to DPNP) and seems like the most sensible path forward since array API disallows promotion for those functions, but will take a little more work. Therefore, I will handle it in a separate PR, and this is ready for review pending CI success.

@ndgrigorian ndgrigorian marked this pull request as ready for review April 23, 2024 21:10
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_312 ran successfully.
Passed: 871
Failed: 7
Skipped: 92

Copy link
Collaborator

@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.

Thank you @ndgrigorian ! Looks good to me. Local testing worked well too.

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