Skip to content

clip: ambiguous wording for scalar min/max #925

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.

Sign up for GitHub

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

Jump to bottom
Open
crusaderky opened this issue Apr 11, 2025 · 3 comments · May be fixed by #926
Open

clip: ambiguous wording for scalar min/max #925

crusaderky opened this issue Apr 11, 2025 · 3 comments · May be fixed by #926

Comments

@crusaderky
Copy link
Contributor

clip states:

  • min [...] Should have the same data type as x. Default: None.
  • max [...] Should have the same data type as x. Default: None.
  • For scalar min and/or max, the scalar values should follow type promotion rules for operations involving arrays and scalar operands (see Type Promotion Rules). Hence, if x and either min or max have different data type kinds (e.g., integer versus floating-point), behavior is unspecified and thus implementation-dependent.

The third point could use rewording in my opinion.
I believe that the intended meaning was that clip(Array[int64], 1.0) has unspecified behaviour, as per type promotion rules.
However, the wording as it is could be interpreted that clip(Array[float64], 1) also has unspecified behaviour, which doesn't make sense to me.

Proposal

Change

Hence, if x and either min or max have different data type kinds (e.g., integer versus floating-point), behavior is unspecified and thus implementation-dependent.

To

Hence, if the scalar type of either min or max is incompatible with the data type of x (x has integer data type and either min or max are scalar floats), behavior is unspecified and thus implementation-dependent.

Backporting

As I believe this is only a clarification, it should be backported to previous versions. Note that, unlike most binops, this function has always accepted scalars for min/max and wording was the same in 2023.12.

XREFs
This was referenced Apr 11, 2025
Merged
Open
@ev-br
Copy link
Member

ev-br commented Apr 11, 2025

Several other things that'd be great to clarify:

  • if x is integer array, and min or max is a python int and is outside of bounds for x.dtype, is the behavior unspecified or are min/max implicitly combined with the dtype bounds?

    xp.clip(xp.arange(3, dtype=xp.uint8), -1, 256)

  • if x is a float array, and min or max is a python int, what is the behavior if, say, max exceeds xp.finfo(x.dtype).max?

@crusaderky
Copy link
Contributor Author

  • if x is integer array, and min or max is a python int and is outside of bounds for x.dtype, is the behavior unspecified or are min/max implicitly combined with the dtype bounds?

It does say this one though:

If x has an integral data type and a broadcasted element in min or max is outside the bounds of the data type of x, behavior is unspecified and thus implementation-dependent.

@kgryte
Copy link
Contributor

kgryte commented Apr 11, 2025

I believe we'd just need to remove the sentence:

Hence, if x and either min or max have different data type kinds (e.g., integer versus floating-point), behavior is unspecified and thus implementation-dependent.

The type promotion rules governing mixing scalars and arrays already covers the case of scalar int and a floating-point array. And already covers a scalar float and an integer array.

@crusaderky crusaderky linked a pull request Apr 14, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

docs: clip() with scalar min/max clarifications crusaderky/array-api
3 participants
@ev-br @kgryte @crusaderky