ENH: Support integer dtype inputs in rounding functions#26766
ENH: Support integer dtype inputs in rounding functions#26766mattip merged 1 commit intonumpy:mainfrom
Conversation
mtsokol
changed the title
|
What about booleans? The Array API spec doesn't mention these here, but it seems like it would be consistent to return boolean outputs for boolean inputs to these functions. |
|
I suppose we have to make a choice for them, and it could go three ways:
But if you propagate, does |
|
|
|
I think no-op support for bool dtype also makes sense to me. |
| UNARY_LOOP_FAST(@type@, @type@, *out = 1.0 / in); | ||
| } | ||
|
|
||
| /**begin repeat1 |
There was a problem hiding this comment.
Not a major worry, but just wondering: Aren't there loops for np.positive already? Is it possible to reuse those rather than define new ones?
There was a problem hiding this comment.
I think there's a loop per each dtype and function, like @TYPE@_@kind@.
Here, for positive we have:
NPY_NO_EXPORT void NPY_CPU_DISPATCH_CURFX(@TYPE@_positive)
(char **args, npy_intp const *dimensions, npy_intp const *steps, void *NPY_UNUSED(func))
{
UNARY_LOOP_FAST(@type@, @type@, *out = +in);
}I think for floor, ceil, and trunc we need separate ones anyway to follow the convention dtype_func. Or do you mean changing @TYPE@_positive to @TYPE@_@kind@ and adding an inner repeat1 loop here? (with #kind = floor, ceil, trunc, positive#)
There was a problem hiding this comment.
You can just add an alias define to the header file.
There was a problem hiding this comment.
yes, an alias seems the right solution, to avoid compiling the same code four times!
There was a problem hiding this comment.
Done! I added #define alias pointing to positive loops for floor, ceil, and trunc ufuncs.
But this alias covers only ints, as positive doesn't support boolean dtype. For boolean dtype I added separate no-op loops (I didn't find other ufunc that would be a no-op for booleans), is it OK?
There was a problem hiding this comment.
Sure! But raising would be a new backward incompatible behavior. Then I propose to leave bool and continue to cast (floor, ceil, and trunc cast bool dtype right now in main). Is it correct?
There was a problem hiding this comment.
Ah, I see what you mean, I hadn't quite realized currently it cast to float. If you do nothing about bool in your PR, does it still cast to float, or does it become int? If we don't pass on bool or raise, I prefer casting to int to casting to float...
If this becomes unclear, one way out is simply not to touch it here, but raise a separate issue to discuss, hoping still to decide before 5.1.
There was a problem hiding this comment.
It would go to the first type that matches, which is either uint8 or int8 (dunno).
There was a problem hiding this comment.
It casts to int8:
In [1]: np.ceil(np.array([1, 0, 1]).astype(bool))
Out[1]: array([1, 0, 1], dtype=int8)Then let me update it here.
There was a problem hiding this comment.
Done! Ready from my side!
mtsokol
force-pushed
the
rounding-dtypes
branch
4 times, most recently
from
93d4ebe to
c084f64
Compare
|
It's ready for another review! (I think CI failures are unrelated as they occur in other PRs as well.) |
mtsokol
force-pushed
the
rounding-dtypes
branch
2 times, most recently
from
9f625d8 to
52792d4
Compare
|
@mhvk, we discussed this a bit and slightly leaned towards pass through being maybe the simplest (even if we don't touch I'll say that I might slightly prefer just deprecating it, but I just don't think it matters much and I won't have squirms with deprecating it later. I do not like the current There are a lot more float only functions with similar wonky behavior for ints and bools. However, for most of them, the return dtype should just be changed to promote to float64. |
|
@seberg - I don't really mind much either way, just felt the easiest way to get this (nice) addition in was not to worry about |
|
@seberg I added no-op for boolean dtype - it's completed from my side. |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
This looks good to me given the decision to make the bool ops a no-op. Also nice and simple.
|
Thanks @mtsokol |
`rv_discrete._cdf` relied on `np.floor` promoting its integer input to `np.float64`. This is no longer the case since numpy/numpy#26766. [skip cirrus] [skip circle]
* BUG: stats: adapt to `np.floor` type promotion removal `rv_discrete._cdf` relied on `np.floor` promoting its integer input to `np.float64`. This is no longer the case since numpy/numpy#26766.
* BUG: stats: adapt to `np.floor` type promotion removal `rv_discrete._cdf` relied on `np.floor` promoting its integer input to `np.float64`. This is no longer the case since numpy/numpy#26766.
as they are removed in numpy/numpy#26766
This one changed with recent numpy upgrade, see numpy/numpy#26766
This one changed with recent numpy upgrade, see numpy/numpy#26766
* BUG: stats: adapt to `np.floor` type promotion removal `rv_discrete._cdf` relied on `np.floor` promoting its integer input to `np.float64`. This is no longer the case since numpy/numpy#26766.
6 participants
Hi @seberg @ngoldbaum,
This PR adds support for integer dtype inputs in
floor,ceil, andtruncufuncs to match the Array API standard.For integer dtype inputs an identity is returned in a loop instead of casting to the float dtype.