Skip to content
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

Add searchsorted to the specification #699

Merged
merged 11 commits into from
Jan 11, 2024

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Nov 6, 2023

This PR

  • resolves RFC: add searchsorted to the specification #688 by adding a specification for searchsorted to the Array API specification.
  • requires that x1 be a one-dimensional array. PyTorch and TensorFlow allow x1 to be multi-dimensional; however, NumPy et al do not. In this PR, I've chosen to restrict x1 to a one-dimensional array, as this is the lowest common denominator. The specification can be extended in the future to accommodate multi-dimensional x1 (stacking) in the future.
  • requires that x2 be an array. CuPy and TensorFlow require that x2 be an array, while NumPy, PyTorch, and others allow x2 to be a scalar. In this PR, I've chosen to restrict x2 to be an array. For the scalar use case, downstream consumers can wrap scalar values as zero-dimensional arrays.

@kgryte kgryte added the API extension Adds new functions or objects to the API. label Nov 6, 2023
@kgryte kgryte added this to the v2023 milestone Nov 6, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @kgryte. I have a few minor comments, but +1 overall to this addition.

The issue is linked to from a scikit-learn PR where searchsorted was needed, and SciPy needs it as well (Cc @mdhaber for visibility).

src/array_api_stubs/_draft/searching_functions.py Outdated Show resolved Hide resolved
src/array_api_stubs/_draft/searching_functions.py Outdated Show resolved Hide resolved
@kgryte
Copy link
Contributor Author

kgryte commented Jan 11, 2024

@rgommers This PR should be ready for final sign off.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Reviewed again, all comments were addressed and this PR looks good to me. I'll plan to merge it later today or tomorrow.

@rgommers rgommers merged commit 72f86f3 into data-apis:main Jan 11, 2024
3 checks passed
@asmeurer
Copy link
Member

In this PR, I've chosen to restrict x1 to a one-dimensional array, as this is the lowest common denominator. The specification can be extended in the future to accommodate multi-dimensional x1 (stacking) in the future.

Shouldn't the language be "x1 should be a one-dimensional array" instead of "x1 must be a one-dimensional array"?


Default: ``'left'``.
sorter: Optional[array]
array of indices that sort ``x1`` in ascending order. The array must have the same shape as ``x1`` and have an integer data type. Default: ``None``.
Copy link
Member

Choose a reason for hiding this comment

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

Should we only require this to be the default array index type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would that be necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Adds new functions or objects to the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: add searchsorted to the specification
4 participants