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

Feature/elementwise functions out keyword #1209

Conversation

vtavana
Copy link
Collaborator

@vtavana vtavana commented May 19, 2023

Adding out keyword for elementwise functions

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

@vtavana vtavana self-assigned this May 19, 2023
@vtavana vtavana changed the base branch from master to feature/elementwise-functions May 19, 2023 01:58
@github-actions
Copy link

@vtavana vtavana requested a review from oleksandr-pavlyk May 19, 2023 02:25
dpctl/tests/elementwise/test_add.py Outdated Show resolved Hide resolved
dpctl/tests/elementwise/test_add.py Outdated Show resolved Hide resolved
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_98 ran successfully.
Passed: 220
Failed: 780
Skipped: 116

@vtavana vtavana force-pushed the feature/elementwise-functions-out-keyword branch from 8d1f68b to f774228 Compare May 19, 2023 03:20
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_108 ran successfully.
Passed: 226
Failed: 774
Skipped: 116

@oleksandr-pavlyk
Copy link
Collaborator

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

In [2]: m = dpt.ones((10, 10), dtype="f4")

In [3]: v = dpt.ones(10, dtype="i4")

In [4]: r = dpt.empty_like(m)

In [5]: dpt.add(m, v, out=r)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [5], in <cell line: 1>()
----> 1 dpt.add(m, v, out=r)

File ~/repos/dpctl/dpctl/tensor/_elementwise_common.py:406, in BinaryElementwiseFunc.__call__(self, o1, o2, out, order)
    401     raise TypeError(
    402         f"output array must be of usm_ndarray type, got {type(out)}"
    403     )
    405 if out.shape != o1_shape or out.shape != o2_shape:
--> 406     raise TypeError(
    407         "The shape of input and output arrays are inconsistent."
    408         f"Expected output shape is {o1_shape}, got {out.shape}"
    409     )
    411 if ti._array_overlap(o1, out) or ti._array_overlap(o2, out):
    412     raise TypeError("Input and output arrays have memory overlap")

TypeError: The shape of input and output arrays are inconsistent.Expected output shape is (10, 10), got (10, 10)

@vtavana vtavana force-pushed the feature/elementwise-functions-out-keyword branch from f774228 to 21e1415 Compare May 19, 2023 15:50
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_108 ran successfully.
Passed: 227
Failed: 773
Skipped: 116

@oleksandr-pavlyk
Copy link
Collaborator

Changes in this PR lead to a drop in coverage (84.036% in the target branch vs. 83.897% in this branch). Since your changes only modify _elementwise_common.py, please use pytest-cov to identify gaps and add required tests.

@vtavana
Copy link
Collaborator Author

vtavana commented May 20, 2023

Changes in this PR lead to a drop in coverage (84.036% in the target branch vs. 83.897% in this branch). Since your changes only modify _elementwise_common.py, please use pytest-cov to identify gaps and add required tests.

I added new tests and now the coverage for _elementwise_common.py is 91% compared to 89%. All the lines that are added in this PR are tested now.

after adding new tests
dpctl/tensor/_elementwise_common.py 300 26 176 15 91% 83, 151, 161, 185-186, 229, 239, 243, 254, 259, 277, 315, 318, 341, 361, 372-373, 390, 475-477, 487, 517-519, 529

before adding new tests:
dpctl/tensor/_elementwise_common.py 300 30 176 20 89% 83, 95, 119->124, 151, 161, 185-186, 229, 239, 243, 254, 259, 277, 315, 318, 341, 361, 372-373, 390, 459, 475-477, 487, 496, 517-519, 529, 538

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_110 ran successfully.
Passed: 227
Failed: 773
Skipped: 116

@oleksandr-pavlyk
Copy link
Collaborator

oleksandr-pavlyk commented May 20, 2023

Tests fail on my WSL where GPU does not support fp64.

Specifically test_add_dtype_matrix where NumPy returns "double" and test_add_dtype_error which attempts to use "f8" regardless target device capabilities.


UPD: I pushed fixes for this and added tests to increase coverage.

Also ensure that test_add_order exercises non-same dtypes to
improve coverage.
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_111 ran successfully.
Passed: 228
Failed: 772
Skipped: 116

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_112 ran successfully.
Passed: 227
Failed: 773
Skipped: 116

@oleksandr-pavlyk oleksandr-pavlyk self-requested a review May 20, 2023 17:37
@@ -22,9 +22,17 @@ def test_abs_out_type(dtype):
np.dtype("c16"): np.dtype("f8"),
}
assert dpt.abs(X).dtype == type_map[arg_dt]

out = dpt.empty_like(X, dtype=type_map[arg_dt])
dpt.abs(X, out)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think out should not be allowed to be a positional argument in array API. Could you please change it to a keyword argument in the test suite?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure!

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.3dev1=py310h76be34b_113 ran successfully.
Passed: 228
Failed: 772
Skipped: 116

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 @vtavana ! Please merge when ready

@vtavana vtavana merged commit cc75cd2 into feature/elementwise-functions May 20, 2023
@github-actions
Copy link

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

@vtavana vtavana deleted the feature/elementwise-functions-out-keyword branch May 21, 2023 00:11
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