Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
ENH: Support integer dtype inputs in rounding functions#26766
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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. |
| @@ -61,6 +61,17 @@ NPY_NO_EXPORT void NPY_CPU_DISPATCH_CURFX(@TYPE@_reciprocal) | |||
| UNARY_LOOP_FAST(@type@, @type@, *out = 1.0 / in); | |||
| } | |||
| /**begin repeat1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not a major worry, but just wondering: Aren't there loops fornp.positive already? Is it possible to reuse those rather than define new ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think there's a loop per each dtype and function, like@TYPE@_@kind@.
Here, forpositive we have:
NPY_NO_EXPORTvoidNPY_CPU_DISPATCH_CURFX(@TYPE@_positive)(char**args,npy_intpconst*dimensions,npy_intpconst*steps,void*NPY_UNUSED(func)){UNARY_LOOP_FAST(@type@, @type@,*out=+in);}
I think forfloor,ceil, andtrunc we need separate ones anyway to follow the conventiondtype_func. Or do you mean changing@TYPE@_positive to@TYPE@_@kind@ and adding an innerrepeat1 loop here? (with#kind = floor, ceil, trunc, positive#)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can just add an alias define to the header file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, an alias seems the right solution, to avoid compiling the same code four times!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done! I added#define alias pointing topositive loops forfloor,ceil, andtrunc ufuncs.
But this alias covers only ints, aspositive 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure! But raising would be a new backward incompatible behavior. Then I propose to leavebool and continue to cast (floor,ceil, andtrunc cast bool dtype right now inmain). Is it correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, I see what you mean, I hadn't quite realized currently it cast tofloat. 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 onbool or raise, I prefer casting toint to casting tofloat...
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would go to the first type that matches, which is either uint8 or int8 (dunno).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It casts toint8:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done! Ready from my side!
93d4ebe toc084f64Comparemtsokol commentedJul 1, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It's ready for another review! (I think CI failures are unrelated as they occur in other PRs as well.) |
9f625d8 to52792d4Compare@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks good to me given the decision to make the bool ops a no-op. Also nice and simple.
5cf7883 intonumpy:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@mtsokol |
`rv_discrete._cdf` relied on `np.floor` promoting its integer input to `np.float64`. This is no longer the case sincenumpy/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 sincenumpy/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 sincenumpy/numpy#26766.
as they are removed innumpy/numpy#26766
This one changed with recent numpy upgrade, seenumpy/numpy#26766
This one changed with recent numpy upgrade, seenumpy/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 sincenumpy/numpy#26766.
Uh oh!
There was an error while loading.Please reload this page.
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.