Skip to content

Add dtype and manipulation functions #1566

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

Closed
wants to merge 17 commits into from
Closed

Add dtype and manipulation functions #1566

wants to merge 17 commits into from

Conversation

npolina4
Copy link
Contributor

@npolina4 npolina4 commented Sep 21, 2023

RP will separate:
#1582 Leverage dpctl.tensor.iinfo() and dpctl.tensor.finfo() implementation.
#1583 Clean up old implementation of logic functions
#1585 Updated atleast_2d and atleast_3d functions
#1586 Implemented dpnp.tile function.

Add dtype and manipulation functions and tests.
Manipulation functions: dpnp.broadcast_arrays, dpnp.repeat, dpnp.vstack, dpnp.ravel
Data Type functions: dpnp.can_cast

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

@github-actions
Copy link
Contributor

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

@vtavana
Copy link
Collaborator

vtavana commented Sep 25, 2023

It seems we do not to skip the fallowing tests anymore.

tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_0_{shapes=[(), ()]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_1_{shapes=[(0,), (0,)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_2_{shapes=[(1,), (1,)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_3_{shapes=[(2,), (2,)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_4_{shapes=[(0,), (1,)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_5_{shapes=[(2, 3), (1, 3)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_6_{shapes=[(2, 1, 3, 4), (3, 1, 4)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_7_{shapes=[(4, 3, 2, 3), (2, 3)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_8_{shapes=[(2, 0, 1, 1, 3), (2, 1, 0, 0, 3)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_9_{shapes=[(0, 1, 1, 3), (2, 1, 0, 0, 3)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestBroadcast_param_10_{shapes=[(0, 1, 1, 0, 3), (5, 2, 0, 1, 0, 0, 3), (2, 1, 0, 0, 0, 3)]}::test_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestInvalidBroadcast_param_0_{shapes=[(3,), (2,)]}::test_invalid_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestInvalidBroadcast_param_1_{shapes=[(3, 2), (2, 3)]}::test_invalid_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestInvalidBroadcast_param_2_{shapes=[(3, 2), (3, 4)]}::test_invalid_broadcast_arrays
tests/third_party/cupy/manipulation_tests/test_dims.py::TestInvalidBroadcast_param_3_{shapes=[(0,),(2,)]}::test_invalid_broadcast_arrays

@ndgrigorian
Copy link
Collaborator

@npolina4
With IntelPython/dpctl#1427 you will no longer need to flatten the repeated array for the axis=None case. Array API spec seems to be supporting this behavior.

@antonwolfy
Copy link
Contributor

The below example works differently with numpy and description:

a = numpy.ones((2, 3), dtype='i4')
b = a.astype('i4', copy=False)

b is a
# Out: True

a = dpnp.ones((2, 3), dtype='i4')
b = a.astype('i4', copy=False)

b is a
# Out: False

Parameters
----------
dtype : dtype
Target data type.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is about None value? It would be good to state clearly, since dpctl and numpy supports that but might behave differently.

return dpt.astype(x1, dtype, order=order, casting=casting, copy=copy)
Parameters
----------
dtype : dtype
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to describe x1 and casting

Otherwise, a copy is returned.
Parameters
----------
dtype : dtype
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to describe casting

Comment on lines +621 to +624
Limitations
-----------
Parameter `subok` is supported with default value.
Otherwise the function will be executed sequentially on CPU.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably it would be better to get rid of the falling back on CPU and to raise an exception explicitly, like cupy does?

new_array = self.__new__(dpnp_array)
new_array._array_obj = dpt.astype(
self._array_obj, dtype, order=order, casting=casting, copy=copy
return dpnp.astype(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have main implementation here and then dpnp.astype will be equal to

if dpnp.is_supported_array_type(x1):
    return x1.astype(...)
else:
    raise TypeError(...)

Also it will allow us to avoid invoking dpnp.get_usm_ndarray(...) and dpnp_array._create_from_usm_ndarray(...).

@@ -150,30 +151,41 @@ def asnumpy(input, order="C"):


def astype(x1, dtype, order="K", casting="unsafe", subok=True, copy=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither numpy or cupy has astype method (they have it only in the context of ndarray), so I would propose to remove unsupported subok parameter here.

-------
out : dpnp.array
If ``copy`` is False and no cast is required, then the array itself is returned.
Otherwise, it returns a (possibly casted) copy of the array.
Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds a bit unclear what "possibly casted" means. Could you please clarify that in the description or rephrase it.
In general, numpy and dpctl returns a view where possible.

@@ -597,32 +597,46 @@ def asnumpy(self):
return dpt.asnumpy(self._array_obj)

def astype(self, dtype, order="K", casting="unsafe", subok=True, copy=True):
"""Copy the array with data type casting.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please check tests/third_party/cupy/core_tests/test_ndarray_copy_and_view.py::TestArrayCopyAndView and unmute where possible?
Also it would be great refresh test_ndarray_copy_and_view.py file.

@npolina4 npolina4 closed this Nov 28, 2023
@npolina4 npolina4 deleted the dtype_functions branch November 28, 2023 15:55
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.

5 participants