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

Improve built-in ops and dpnp universal functions testing #1283

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

ZzEeKkAa
Copy link
Contributor

@ZzEeKkAa ZzEeKkAa commented Jan 19, 2024

Add full coverage on ufuncs and built-in operators. Motivated by #178

It appears that we were not aware that these functions are not supported:

  • "cbrt"

We also does not support (may be we need proper input data):

  • matmul operator (a @ b)
  • imatmul operator (a @= b)
  • not_ operator (not a) (because numpy does not support it)

Open question:

  • Does it mean if the operator works in parfor, than it will work in kernel?

Checklist:

  • 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?
  • If this PR is a work in progress, are you filing the PR as a draft?

@ZzEeKkAa ZzEeKkAa self-assigned this Jan 19, 2024
@ZzEeKkAa ZzEeKkAa force-pushed the feature/improve_parfor_ufunc_testing branch 2 times, most recently from efe2d6d to dd1e75f Compare January 19, 2024 10:05
@diptorupd diptorupd changed the title Improve built-in ops and dpnp user functions testing Improve built-in ops and dpnp universal functions testing Jan 19, 2024
@diptorupd
Copy link
Contributor

@ZzEeKkAa Thank you for the comprehensive evaluation. I am replying to some of your questions and comments in the PR description.

  1. It appears that we were not aware that these functions are not supported:
    "cbrt"
    "mod"

Please log separate issues to track the gap.

  1. We also does not support (may be we need proper input data):

    matmul operator (a @ b)
    imatmul operator (a @= b)
    not_ operator (not a)

The matmul operators not being supported is expected. I believe the matmul operators are not treated as parfor and Numba lowers them as library calls. Numba-dpex used to have the same functionality at one point.

The not_ not being supported needs investigation. Please open an issue so that we can keep track of it.

  1. Does it mean if the operator works in parfor, than it will work in kernel?

That is a correct assumption. However, we should note a subtle difference. There are various types of expressions that can be lowered to parfor nodes: numpy calls on arrays and built-in operations on arrays (both of which are vector operations), numpy calls and built in operations on scalars inside a prange loop (the operation here is a scalar op). In a kernel, only the scalar versions of all these operators and ufuncs can be used.

It is a good idea to add similar unit tests for the kernel decorator as well. We will be explicitly verifying that lowering a scalar operation works.

@ZzEeKkAa ZzEeKkAa force-pushed the feature/improve_parfor_ufunc_testing branch from dd1e75f to b242513 Compare January 24, 2024 10:12
@ZzEeKkAa ZzEeKkAa force-pushed the feature/improve_parfor_ufunc_testing branch from b242513 to 583f1a1 Compare January 24, 2024 12:19
@ZzEeKkAa ZzEeKkAa marked this pull request as ready for review January 24, 2024 12:19
@ZzEeKkAa ZzEeKkAa requested a review from diptorupd as a code owner January 24, 2024 12:19
@ZzEeKkAa
Copy link
Contributor Author

ZzEeKkAa commented Jan 24, 2024

not is not supported by numpy, I've managed to make mod work and opened issue for cbrt.

Let's test kernel in separate PR.

@ZzEeKkAa ZzEeKkAa force-pushed the feature/improve_parfor_ufunc_testing branch from 583f1a1 to 002c153 Compare January 24, 2024 12:26
@ZzEeKkAa ZzEeKkAa force-pushed the feature/improve_parfor_ufunc_testing branch from 002c153 to 6a341d2 Compare January 24, 2024 17:52
Copy link
Contributor

@diptorupd diptorupd left a comment

Choose a reason for hiding this comment

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

nice changes. Making unit testing more comprehensive is always welcome!

@diptorupd diptorupd enabled auto-merge January 24, 2024 18:00
@diptorupd diptorupd merged commit 8fdfc1b into main Jan 24, 2024
38 of 46 checks passed
@diptorupd diptorupd deleted the feature/improve_parfor_ufunc_testing branch January 24, 2024 18:34
github-actions bot added a commit that referenced this pull request Jan 24, 2024
…c_testing

Improve built-in ops and dpnp universal functions testing 8fdfc1b
github-actions bot added a commit to diptorupd/numba-dpex that referenced this pull request Jan 26, 2024
…parfor_ufunc_testing

Improve built-in ops and dpnp universal functions testing 8fdfc1b
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.

2 participants