Update implementation of dpnp.putmask - #3014
Open
vlad-perevezentsev wants to merge 8 commits into
Open
Conversation
Contributor
|
View rendered docs @ https://intelpython.github.io/dpnp/pull/3014/index.html |
vlad-perevezentsev
marked this pull request as ready for review
August 7, 2026 12:25
vlad-perevezentsev
requested review from
antonwolfy and
ndgrigorian
as code owners
August 7, 2026 12:25
Contributor
|
Array API standard conformance tests for dpnp=0.21.0dev3=py314h509198e_43 ran successfully. |
Collaborator
|
coverage: 78.443% (+0.03%) from 78.414% — add_putmask_impl into master |
antonwolfy
reviewed
Aug 19, 2026
| # -*- coding: utf-8 -*- | ||
| # ***************************************************************************** | ||
| # Copyright (c) 2025, Intel Corporation | ||
| # Copyright (c) 2026, Intel Corporation |
Contributor
There was a problem hiding this comment.
It's year when file was created, no need to update
Suggested change
| # Copyright (c) 2026, Intel Corporation | |
| # Copyright (c) 2025, Intel Corporation |
| # values must always be flattened in C-order. | ||
| values_1d = values.ravel(order="C") | ||
| if a.dtype != values_1d.dtype: | ||
| values_1d = dpnp.astype( |
Contributor
There was a problem hiding this comment.
Seems no test is covering that
| @@ -1,5 +1,5 @@ | |||
| //***************************************************************************** | |||
| // Copyright (c) 2025, Intel Corporation | |||
| // Copyright (c) 2026, Intel Corporation | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // Copyright (c) 2026, Intel Corporation | |
| // Copyright (c) 2025, Intel Corporation |
| array([[ 0, 1, 2], | ||
| [ 9, 16, 25]]) | ||
|
|
||
| If `values` is smaller than `x1` it is repeated: |
Contributor
There was a problem hiding this comment.
Suggested change
| If `values` is smaller than `x1` it is repeated: | |
| If `values` is smaller than `a` it is repeated: |
| :obj:`dpnp.take` : Take elements from an array along an axis. | ||
| :obj:`dpnp.copyto` : Copies values from one array to another. | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
Double blank like:
Suggested change
| for xp in (numpy, cupy): | ||
| a = xp.array([1, 2, 3]) | ||
| mask = xp.array([True, False]) | ||
| with pytest.raises((ValueError, IndexError)): |
Contributor
There was a problem hiding this comment.
Can be aligned now:
Suggested change
| with pytest.raises(ValueError): |
| const std::size_t values_size = values.get_size(); | ||
|
|
||
| // the contig kernel cycles `values` by the memory-linear index, which | ||
| // matches numpy's C-order `values.flat` only for C-contiguous data |
Contributor
There was a problem hiding this comment.
but no check here that values.is_c_contiguous() has to be true
Comment on lines
+106
to
+107
| std::size_t nelems, | ||
| std::size_t val_size) |
Contributor
There was a problem hiding this comment.
Suggested change
| std::size_t nelems, | |
| std::size_t val_size) | |
| const std::size_t nelems, | |
| const std::size_t val_size) |
| elems_per_wi * (ndit.get_group(0) * ndit.get_local_range(0) + | ||
| sg.get_group_id()[0] * sgSize); | ||
|
|
||
| const bool values_no_repeat = (val_size_ >= nelems_); |
Contributor
There was a problem hiding this comment.
can be moved away of if-else block to reduce duplication
| #pragma unroll | ||
| for (std::uint8_t vec_id = 0; vec_id < vec_sz; ++vec_id) { | ||
| out_vec[vec_id] = | ||
| (mask_vec[vec_id] != static_cast<std::uint8_t>(0)) |
Contributor
There was a problem hiding this comment.
for consistency with other places in the kernel:
Suggested change
| (mask_vec[vec_id] != static_cast<std::uint8_t>(0)) | |
| (mask_vec[vec_id]) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: