Update implementation of dpnp.putmask - #3014
Conversation
|
View rendered docs @ https://intelpython.github.io/dpnp/pull/3014/index.html |
|
Array API standard conformance tests for dpnp=0.21.0dev6=py314ha0e2e8e_18 ran successfully. |
|
coverage: 78.507% (+0.04%) from 78.472% — add_putmask_impl into master |
| usm_mask = dpt.astype(usm_mask, dpnp.bool, copy=False) | ||
|
|
||
| if usm_a.shape != usm_mask.shape: | ||
| raise ValueError("mask and data must be the same size") |
There was a problem hiding this comment.
| raise ValueError("mask and data must be the same size") | |
| raise ValueError("mask and data must be the same shape") |
| numpy.putmask(a, m, v) | ||
| dpnp.putmask(ia, im, iv) | ||
| assert_array_equal(a, ia) | ||
| class TestPutMask: |
There was a problem hiding this comment.
Missing cross-queue / usm_type propagation tests in test_sycl_queue.py / test_usm_type.py
| numpy.putmask(a, m, v) | ||
| dpnp.putmask(ia, im, iv) | ||
| assert_array_equal(a, ia) | ||
| class TestPutMask: |
There was a problem hiding this comment.
The vectorized fast path is still untested. The contig vectorized branch needs nelems ≳ 128–256; every array in TestPutMask is ≤ 24 elements.
| assert_array_equal(ia, a) | ||
|
|
||
| @pytest.mark.parametrize("dt", get_all_dtypes(no_none=True)) | ||
| def test_scalar_values(self, dt): |
There was a problem hiding this comment.
The scalar unsafe-cast path is untested. Needs to cover also unsafe scalar truncation/wraparound.
|
|
||
| x1_desc = dpnp.get_dpnp_descriptor( | ||
| x1, copy_when_strides=False, copy_when_nondefault_queue=False | ||
| dpnp.check_supported_arrays_type(a) |
There was a problem hiding this comment.
Not needed, the same check is done by dpnp.get_usm_ndarray
| // THE POSSIBILITY OF SUCH DAMAGE. | ||
| //***************************************************************************** | ||
|
|
||
| #include <algorithm> |
| check_writable({&dst}, names); | ||
|
|
||
| // values must be C-contiguous | ||
| check_c_contig({&values}, names); |
| // the contig kernel cycles `values` by the memory-linear index, which | ||
| // matches numpy's C-order `values.flat` only for C-contiguous data | ||
| const bool all_c_contig = dst.is_c_contiguous() && mask.is_c_contiguous() && | ||
| values.is_c_contiguous(); |
There was a problem hiding this comment.
Do we need to remove values.is_c_contiguous() from here?
| * @tparam T Type of input vector `dst` and `values` and of result vector `dst`. | ||
| */ | ||
| template <typename T> | ||
| struct PutMaskOutputType |
There was a problem hiding this comment.
It does not seem we need that, and so can be dropped.
Similarly to dpnp/tensor/libtensor/include/kernels/where.hpp:
template <typename fnT, typename T, typename condT>
struct WhereStridedFactory
{
fnT get()
{
fnT fn = where_strided_impl<T, condT>;
return fn;
}
};
template <typename fnT, typename T, typename condT>
struct WhereContigFactory
{
fnT get()
{
fnT fn = where_contig_impl<T, condT>;
return fn;
}
};|
|
||
| int eff_nd = nd; | ||
| if (nd == 0) { | ||
| // scalar arrays: single-element 1D iteration |
There was a problem hiding this comment.
It seems unreachable defensive code (0-d dst → contig path).
4 participants
This PR proposes a new implementation of
dpnp.putmaskreplacing the legacydpnp_putmaskimplementation with dedicated SYCL kernels : a vectorized contiguous kernel and a strided kernel for F-contiguous/transposed arrays.It also fully reworks the
putmasktests by addingTestPutMaskPerformance results on PVC are below: