feat!: allow clip to have int min or max when x is floating-point #811
Conversation
This is for consistency with operators, which allow combining an int with an array that has a floating-point data type. See the discussion at data-apis#807.
This was somewhat implicit, but it should be clearer now that if all three arguments are arrays that there is a three-way broadcast.
|
I also made it clearer that if all three arguments are arrays then there is a three-way broadcast. |
|
Note that if we decide to fix #807 more generally we may want to just remove the specific language for promotion behavior for Python scalars from |
rgommers
added
the
API change
There was a problem hiding this comment.
Thanks @asmeurer. A question about the dtype change inline.
I also made it clearer that if all three arguments are arrays then there is a three-way broadcast.
This change seems reasonable to me, and I verified that numpy behaves that way. It may be worth including this in the test suite to ensure this actually is the case for all tested implementations?
| @@ -806,7 +806,7 @@ def clip( | |||
|
|
|||
| - If both ``min`` and ``max`` are ``None``, the elements of the returned array must equal the respective elements in ``x``. | |||
| - If a broadcasted element in ``min`` is greater than a corresponding broadcasted element in ``max``, behavior is unspecified and thus implementation-dependent. | |||
| - 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. | |||
| - If ``x`` has an integer data type and either ``min`` or ``max`` is floating-point, behavior is unspecified and thus implementation-dependent. | |||
There was a problem hiding this comment.
This isn't just about scalars, as written. What if x is a floating-point array, and min an integer one? Seems ill-defined, and in numpy it upcasts right now:
>>> np.clip(np.ones((3,), dtype=np.float32), a_min=np.zeros((2, 1), dtype=np.int32), a_max=1.5).dtype
dtype('float64')
kgryte
changed the title
kgryte
added
the
Needs Changes
This is for consistency with operators, which allow combining an int with an array that has a floating-point data type.
See the discussion at #807.
I also mentioned this at the meeting last week.