feat!: allow clip to have int min or max when x is floating-point#811
feat!: allow clip to have int min or max when x is floating-point#811asmeurer wants to merge 5 commits intodata-apis:mainfrom
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
rgommers
left a comment
rgommers
left a comment
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?
| - 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
|
I've opened another PR which should supersede this one (see #896) and includes the 3-way merge edits (including backport) which was included here. |
3 participants
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.