Skip to content

Conversation

@ndgrigorian
Copy link
Collaborator

This PR leverages the recently-added dpctl.tensor.isin to implement dpnp.isin

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an 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?
  • Have you added documentation for your changes, if necessary?
  • Have you added your changes to the changelog?

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025

Array API standard conformance tests for dpnp=0.20.0dev1=py313h509198e_7 ran successfully.
Passed: 1229
Failed: 0
Skipped: 7

@coveralls
Copy link
Collaborator

coveralls commented Sep 25, 2025

Coverage Status

coverage: 81.451% (+0.004%) from 81.447%
when pulling 42e9893 on reuse-tensor-isin
into 131c490 on master.

@antonwolfy antonwolfy added this to the 0.20.0 release milestone Oct 7, 2025
return a.flags.fnc


def isin(element, test_elements, assume_unique=False, invert=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

NumPy supports also kind keyword, but we can limit it with only None and 'sort' values and state a limitation that 'table' is not supported

Suggested change
def isin(element, test_elements, assume_unique=False, invert=False):
def isin(element, test_elements, assume_unique=False, invert=False, *, kind=None):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CuPy ignores this keyword though, which is why I chose to. I can add it in if you'd like, to be ignored like assume_unique

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it'd be good to add.

@ndgrigorian ndgrigorian force-pushed the reuse-tensor-isin branch 4 times, most recently from 07f280b to 7d29652 Compare November 20, 2025 03:16
@ndgrigorian ndgrigorian marked this pull request as ready for review November 20, 2025 04:32
Comment on lines +1223 to +1224
input arrays are unique. Included for compatibility with NumPy.
Default: ``False``.
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to have an empty line before Default: .. to have it always from the new line in the rendered documentation:

Suggested change
input arrays are unique. Included for compatibility with NumPy.
Default: ``False``.
input arrays are unique. Included for compatibility with NumPy.
Default: ``False``.

Default: ``False``.
invert : bool, optional
If ``True``, the values in the returned array are inverted, as if
calculating `element not in test_elements`.
Copy link
Contributor

Choose a reason for hiding this comment

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

To align with the form used at the top of the function description:

Suggested change
calculating `element not in test_elements`.
calculating ``element not in test_elements``.

Comment on lines +1229 to +1230
than) ``dpnp.invert(dpnp.isin(a, b))``.
Default: ``False``.
Copy link
Contributor

Choose a reason for hiding this comment

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

an empty line is missing:

Suggested change
than) ``dpnp.invert(dpnp.isin(a, b))``.
Default: ``False``.
than) ``dpnp.invert(dpnp.isin(a, b))``.
Default: ``False``.

Comment on lines +1238 to +1239
Copy link
Contributor

Choose a reason for hiding this comment

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

excess empty line:

Suggested change


dpnp.check_supported_arrays_type(element, test_elements, scalar_type=True)
usm_element = dpnp.as_usm_ndarray(
element, usm_type=element.usm_type, sycl_queue=element.sycl_queue
Copy link
Contributor

Choose a reason for hiding this comment

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

element might be a scalar, but in that case test_elements is not scalar:

Suggested change
element, usm_type=element.usm_type, sycl_queue=element.sycl_queue
element, usm_type=test_elements.usm_type, sycl_queue=test_elements.sycl_queue

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, alternatively, we need to calculate common SYCL queue and USM type somewhere above and pass here and to as_usm_ndarray below

assert_equal(result, expected)


@pytest.mark.parametrize("dtype", get_all_dtypes())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.parametrize("dtype", get_all_dtypes())
@pytest.mark.parametrize("dtype", get_all_dtypes(no_none=True))

assert_equal(result, expected)


@pytest.mark.parametrize(
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be great to have new tests placed inside a class TestIsin

assert_equal(result, expected)


@pytest.mark.parametrize(
Copy link
Contributor

Choose a reason for hiding this comment

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

CFD tests are missing, need to add one to test_sycl_queue.py and another to test_usm_type.py

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.

3 participants