task: add broadcast class implementation - #2901
Conversation
jharlow-intel
changed the title
aa39048 to
1b251bf
Compare
1b251bf to
a2f50c3
Compare
|
View rendered docs @ https://intelpython.github.io/dpnp/pull/2901/index.html |
|
Array API standard conformance tests for dpnp=0.21.0dev3=py314h509198e_41 ran successfully. |
| if len(dpnp_arrays) > 1: | ||
| exec_q = dpt.get_execution_queue( | ||
| tuple(array.sycl_queue for array in dpnp_arrays) | ||
| ) | ||
| if exec_q is None: | ||
| raise dpt.ExecutionPlacementError( | ||
| "Execution placement can not be unambiguously inferred " | ||
| "from input arguments." | ||
| ) |
There was a problem hiding this comment.
do we need to check for compute follows data here? The arrays can be broadcast even if they aren't, it just means device routines can't be run with both as inputs
There was a problem hiding this comment.
if we do keep this check, we would need to check if they are dpnp.tensor.usm_ndarray as well
| The number of iterators. | ||
|
|
||
| """ | ||
| return len(self._arrays) |
There was a problem hiding this comment.
primarily a question for @antonwolfy and @vlad-perevezentsev since it's design-related, but NumPy and CuPy differ drastically in this class implementation
https://numpy.org/doc/2.1/reference/generated/numpy.broadcast.html
https://docs.cupy.dev/en/latest/reference/generated/cupy.broadcast.html
do we want more of the CuPy or NumPy behavior? What is the intended use-case of this class to users?
There was a problem hiding this comment.
I'd say we are not going to fully align with NumPy here, aligning with CuPy looks more preferable as for me, because might help in case of CuPy to DPNP migration for some users.
In that perspective, I'd keep the current implement as it is for now, plus adding values property, which mimics CuPy.
|
coverage: 78.434% (+0.2%) from 78.242% — task/SAT-7028 into master |
| self._size = 1 | ||
| for dim in self._shape: | ||
| self._size *= dim |
There was a problem hiding this comment.
we can use math.prod:
| self._size = 1 | |
| for dim in self._shape: | |
| self._size *= dim | |
| self._size = math.prod(self._shape) |
| The number of iterators. | ||
|
|
||
| """ | ||
| return len(self._arrays) |
There was a problem hiding this comment.
I'd say we are not going to fully align with NumPy here, aligning with CuPy looks more preferable as for me, because might help in case of CuPy to DPNP migration for some users.
In that perspective, I'd keep the current implement as it is for now, plus adding values property, which mimics CuPy.
|
|
||
| See Also | ||
| -------- | ||
| :obj:`dpnp.broadcast_arrays` : Broadcast any number of arrays against |
There was a problem hiding this comment.
Missing back-lins on dpnp.broadcast in docstrings of all three functions below.
| * Added `dpnp.broadcast` class implementation [#2901](https://github.com/IntelPython/dpnp/pull/2901) | ||
|
|
There was a problem hiding this comment.
Remove blank empty line:
| * Added `dpnp.broadcast` class implementation [#2901](https://github.com/IntelPython/dpnp/pull/2901) | |
| * Added `dpnp.broadcast` class implementation [#2901](https://github.com/IntelPython/dpnp/pull/2901) |
| >>> b.size | ||
| 9 | ||
|
|
||
| Limitations |
There was a problem hiding this comment.
Limitations and Notes should be before Examples
| self._size *= dim | ||
| self._nd = len(self._shape) | ||
|
|
||
| @property |
There was a problem hiding this comment.
We probably need to add members:
broadcast.shape
broadcast.size
broadcast.nd
broadcast.ndim
broadcast.numiterto array-manipulation.rst to make them rendered
| from dpnp.tensor._manipulation_functions import _broadcast_shapes | ||
|
|
||
|
|
||
| class broadcast: |
There was a problem hiding this comment.
I don't think we need a new file dpnp/dpnp_broadcast.py. The class can be a part of existing dpnp/dpnp_iface_manipulation.py which currently holds all the broadcast functions
| return self._size | ||
|
|
||
| @property | ||
| def nd(self): |
There was a problem hiding this comment.
I don't think we need nd method, because NumPy states that ndim is preferable:
Number of dimensions of broadcasted result. For code intended for NumPy 1.12.0 and later the more consistent ndim is preferred.
| ------- | ||
| out : tuple | ||
| A tuple containing the shape of the broadcasted result. | ||
|
|
There was a problem hiding this comment.
Missing Examples sections for the methods
4 participants
Adds a
broadcastclass implementation