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

Implements dpctl.tensor.clip #1444

Merged
merged 9 commits into from
Oct 25, 2023
Merged

Implements dpctl.tensor.clip #1444

merged 9 commits into from
Oct 25, 2023

Conversation

ndgrigorian
Copy link
Collaborator

This pull request implements the function clip, for clipping elements of an array to a range.
Clip permits both Python scalar and dpctl.tensor.usm_ndarray arguments for its min and max arguments.

An example of the new functionality:

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

In [2]: x = dpt.arange(-2, 8, dtype="i4")

In [3]: dpt.clip(x, 0, 5)
Out[3]: usm_ndarray([0, 0, 0, 1, 2, 3, 4, 5, 5, 5], dtype=int32)
  • 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?

@github-actions
Copy link

@coveralls
Copy link
Collaborator

coveralls commented Oct 17, 2023

Coverage Status

coverage: 85.728% (+0.01%) from 85.718% when pulling d839124 on implement-clip into 386bd8b on master.

@oleksandr-pavlyk
Copy link
Collaborator

Encountered TypeError:

In [1]: import dpctl, dpctl.tensor as dpt, dpctl.utils as du

In [2]: x = dpt.arange(10)

In [3]: dpt.clip(x, 0, 10)
Out[3]: usm_ndarray([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])

In [4]: dpt.clip(x, 0, 5)
Out[4]: usm_ndarray([0, 1, 2, 3, 4, 5, 5, 5, 5, 5])

In [5]: dpt.clip(x, 0, dpt.inf)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_45 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@ndgrigorian
Copy link
Collaborator Author

Encountered TypeError:

In [1]: import dpctl, dpctl.tensor as dpt, dpctl.utils as du

In [2]: x = dpt.arange(10)

In [3]: dpt.clip(x, 0, 10)
Out[3]: usm_ndarray([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])

In [4]: dpt.clip(x, 0, 5)
Out[4]: usm_ndarray([0, 1, 2, 3, 4, 5, 5, 5, 5, 5])

In [5]: dpt.clip(x, 0, dpt.inf)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

This is currently the expected behavior, as the output is expected to be the same type as the input, and a float can't be cast to int.

@antonwolfy
Copy link
Collaborator

Checked with existing dpnp tests around clip and all of them are passed.

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_45 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_46 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_50 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_53 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@ndgrigorian
Copy link
Collaborator Author

@oleksandr-pavlyk
Do you see value in applying the change of TypeError to ValueError for, e.g., dpt.clip(dpt.arange(10, dtype="i8"), 0, dpt.inf) to the elementwise functions as well?

Then for instance dpt.bitwise_and(dpt.arange(10, dtype="i8"), dpt.inf) will also raise a ValueError instead of TypeError.

@oleksandr-pavlyk
Copy link
Collaborator

@ndgrigorian I think ValueError makes more sense in the example dpt.bitwise_and(dpt.arange(10, dtype="i8"), dpt.inf) as well. Thanks for bringing this up @ndgrigorian

ndgrigorian and others added 8 commits October 23, 2023 22:17
sycl::clamp would yield max or min depending on the platform

A test has been added for this behavior
As the result dtype of the out array is already checked when overlap is checked, checking again later is superfluous
Now properly accounts for all three arrays in all branches
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_70 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@ndgrigorian
Copy link
Collaborator Author

ndgrigorian commented Oct 24, 2023

@ndgrigorian I think ValueError makes more sense in the example dpt.bitwise_and(dpt.arange(10, dtype="i8"), dpt.inf) as well. Thanks for bringing this up @ndgrigorian

Making this change was more breaking than expected for elementwise functions. It will belong in a separate PR, as it will have to touch a lot of tests and unary functions. There are also other cases where inappropriate dtype of an input array leads to a TypeError throughout the project. It may make sense to change those as well.

For now, that work is done on just clip. The PR is ready for review.

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_70 ran successfully.
Passed: 935
Failed: 65
Skipped: 119

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.15.1dev0=py310ha25a700_70 ran successfully.
Passed: 934
Failed: 66
Skipped: 119

@ndgrigorian ndgrigorian merged commit 2eba93e into master Oct 25, 2023
26 checks passed
@ndgrigorian ndgrigorian deleted the implement-clip branch October 25, 2023 20:22
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.

4 participants