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

Permit "same_kind" casting for element-wise in-place operators #2170

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

antonwolfy
Copy link
Contributor

@antonwolfy antonwolfy commented Nov 13, 2024

The PR proposes to permit "same_kind" casting for element-wise in-place operators. The implementation leverages on dpctl changes added in scope of PR#1827.

It also adds callbacks to support in-place bit-wise operators (leverages on dpctl changes from RR#1447).

The PR removes a temporary workaround from dpnp.wrap which depends on the implemented changes.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to 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 filing the PR as a draft?

@antonwolfy antonwolfy self-assigned this Nov 13, 2024
Copy link
Contributor

View rendered docs @ https://intelpython.github.io/dpnp/pull/2170/index.html

@antonwolfy antonwolfy force-pushed the in-place-element-wise-func-casting branch 2 times, most recently from 5cb5319 to 9a41bf9 Compare November 25, 2024 15:49
@@ -347,22 +347,22 @@ def __gt__(self, other):

def __iadd__(self, other):
"""Return ``self+=value``."""
dpnp.add(self, other, out=self)
dpnp.add._inplace_op(self, other)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a good idea?
If I call dpnp.add(a, b, out=a) the code would follows some different path?
So we have two different code paths for inplace ops?

Copy link
Contributor Author

@antonwolfy antonwolfy Nov 26, 2024

Choose a reason for hiding this comment

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

This is how it was designed in dpctl. That PR is attempt to adopt to the API extension exposed by dpctl library.
Also dpnp.add._inplace_op path is much more simplier: some of the validation checks is not necessary there.

For sure, dpnp.add._inplace_op path can be subpart of dpnp.add(a, b, out=a) path in case when out refers to same memory as an input array, but I believe that has to be done by dpctl, not in dpnp.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK array api doesn't support out parameter, so dpctl does not support it either. So it cannot be implemented inside dpctl.

I believe separating these two paths in dpnp is extremely bad idea. Which also requires more code changes than just making add._inplace_op as subpath of add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dpctl supports out keyword, but it's true that python array API doesn't.

Copy link
Contributor Author

@antonwolfy antonwolfy Nov 26, 2024

Choose a reason for hiding this comment

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

The reason why we have to separate the paths it is a deference in allowed type promotion in

  1. dpt.add(a, b, out=a)
  2. a += 2

dpctl requires "safe" type casting in the 1st use case, while for the 2nd one it permits "same_kind".
It can be illustrated below

a = dpt.ones(10, dtype='f4')
b = dpt.ones(10, dtype='f8')

a += b # it works

dpt.add(a, b, out=a)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[9], line 1
----> 1 dpt.add(a, b, out=a)

File /localdisk/work/antonvol/soft/miniforge3/envs/numpy_20/lib/python3.12/site-packages/dpctl/tensor/_elementwise_common.py:653, in BinaryElementwiseFunc.__call__(self, o1, o2, out, order)
    647     raise ValueError(
    648         "The shape of input and output arrays are inconsistent. "
    649         f"Expected output shape is {res_shape}, got {out.shape}"
    650     )
    652 if res_dt != out.dtype:
--> 653     raise ValueError(
    654         f"Output array of type {res_dt} is needed, "
    655         f"got {out.dtype}"
    656     )
    658 if (
    659     dpctl.utils.get_execution_queue((exec_q, out.sycl_queue))
    660     is None
    661 ):
    662     raise ExecutionPlacementError(
    663         "Input and output allocation queues are not compatible"
    664     )

ValueError: Output array of type float64 is needed, got float32

That way we can't leverage on dpt.add(a, b, out=a) behavior while implementing in-place operations in dpnp. It has more strict limitations then we need to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And what numpy do in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NumPy allows both use cases.

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