IntelPython / dpctl

Python SYCL bindings and SYCL-based Python Array API library
https://intelpython.github.io/dpctl/
Apache License 2.0
97 stars 29 forks source link

Missing description of `out` keyword for accumulation functions #1634

Closed antonwolfy closed 3 months ago

antonwolfy commented 3 months ago

There is no description of out keyword for accumulation functions and so the expected behavior might not be clear. In Python array API spec there is no such keyword.

The code below shows different behavior between dpctl and numpy in case when dtype of out array mismatches type x array or default integer type (while numpy supports type casting for out array if necessary):

import numpy, dpctl, dpctl.tensor as dpt

a = dpt.ones(10, dtype='i4')
out = dpt.ones_like(a, dtype='c8')
dpt.cumulative_sum(a, out=out) # expected default integer type != a.dtype
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[7], line 1
----> 1 dpt.cumulative_sum(a, out=out).dtype

File ~/miniconda3/envs/dpnp_dev/lib/python3.9/site-packages/dpctl/tensor/_accumulation.py:281, in cumulative_sum(x, axis, dtype, include_initial, out)
    220 def cumulative_sum(
    221     x, /, *, axis=None, dtype=None, include_initial=False, out=None
    222 ):
    223     """
    224     cumulative_sum(x, /, *, axis=None, dtype=None, include_initial=False,
    225                    out=None)
   (...)
    279             along.
    280     """
--> 281     return _accumulate_common(
    282         x,
    283         axis,
    284         dtype,
    285         include_initial,
    286         out,
    287         tai._cumsum_over_axis,
    288         tai._cumsum_final_axis_include_initial,
    289         tai._cumsum_dtype_supported,
    290         _default_accumulation_dtype,
    291     )

File ~/miniconda3/envs/dpnp_dev/lib/python3.9/site-packages/dpctl/tensor/_accumulation.py:108, in _accumulate_common(x, axis, dtype, include_initial, out, _accumulate_fn, _accumulate_include_initial_fn, _dtype_supported, _default_accumulation_type_fn)
    103     raise ValueError(
    104         "The shape of input and output arrays are inconsistent. "
    105         f"Expected output shape is {final_res_sh}, got {out_sh}"
    106     )
    107 if res_dt != out.dtype:
--> 108     raise ValueError(
    109         f"Output array of type {res_dt} is needed, " f"got {out.dtype}"
    110     )
    111 if dpctl.utils.get_execution_queue((q, out.sycl_queue)) is None:
    112     raise ExecutionPlacementError(
    113         "Input and output allocation queues are not compatible"
    114     )

ValueError: Output array of type int64 is needed, got complex64

na = dpt.asnumpy(a)
nout = dpt.asnumpt(out)
numpy.cumsum(a, out=nout).dtype
# Out: dtype('complex64')

Thus the question arises whether dpctl (as opposed to numpy) was intended to have a strict requirement for data type of the out array.

ndgrigorian commented 3 months ago

This behavior itself is intended, for consistency with how we've chosen to implement the out keyword for elementwise functions and matmul.

The lack of documentation is not, it will need to be added.