API: No timedelta/datetimes are not integers and do not promote #22568
Conversation
|
Actually, at least pandas doesn't see to notice really. |
b054f57 to
d6d8c55
Compare
739faf8 to
c127d12
Compare
|
I am tempted to say that this is best to "just do", rather than try to deprecate (yes, its not quite finished). There are some issues though, i.e. The other issue is the infinite deprecation/futurewarning that we have on comparisons, and I am considering looking into that a bit... |
c127d12 to
e9b98d6
Compare
|
close/reopen |
e9b98d6 to
de6293d
Compare
|
Fixing up the typing is proving more complicated and probably needs another approach (and is not finished yet). I added a release note, the two biggest changes are:
So I am wondering if we should go through a deprecation tedious or not? The problem with a FutureWarning is that it may be a bit noisy. Considering that warning filters are annoying, I guess the best thing might be to have a helper context var to enable |
|
Just a comment from the sidelines: astropy's choice to treat |
|
Yeah, I don't really like the implication either. Generalizing that Some random thoughts:
|
|
Yes, it is tricky... Probably good to start strict, as it is much easier to change after. But definitely have a warning for |
|
I think I may implement the warning there (unfortunately), and then revive it as a "2.0 PR" i.e. only active with opt-in... Maybe we could pull it off otherwise, but I am not sure I want to deal with all that complexity (but can be convinced if anyone wants to). This could be a trial baloon for it. (We might yet get away with doing the "right" thing and just erroring out for |
|
This has merge conflicts and has the "needs a release note" label. |
|
This mainly needs a decision of whether we can get away with this. Otherwise, I will try to do this soonish, but hide it behind "numpy 2.0"... |
|
Needs rebase. |
|
FWIW both the stdlib timedelta and pandas.Timedelta do not promote |
None of that makes sense, so lets get rid of it!
And yes, the printing elif logic thought it was...
… for floats Sure, it barely makes sense there as well, but at least this way we can bypass it and only do the "nonzero" check, which also makes sense for timedelta64. And we need timedelta64 to work, because `np.nanmedian` uses the Masked median internally, and that ends up here 🎉 (sarcasm).
de6293d to
981d3a7
Compare
| _SupportsArray["dtype[timedelta64]"], | ||
| _NestedSequence[_SupportsArray["dtype[timedelta64]"]], | ||
| ] | ||
|
|
There was a problem hiding this comment.
Rebased (no version switch). I wouldn't mind pushing this forward before branching, but I will doubt that it is meant to be. In either case, need to figure out the typing...
@BvB93 do you have quick insight? The typing changes seem to make the failure test(s):
AR_f: np.ndarray[Any, np.dtype[np.float64]]
AR_m: np.ndarray[Any, np.dtype[np.timedelta64]]
AR_f > AR_m # E: Unsupported operand types
```
pass, when floating point comparisons never worked. It is not clear to me what change could have accidentally allowed comparisons of floats/numbers with timedelta?
There was a problem hiding this comment.
So a _ArrayLikeTD64_comp_co re-export is missing to the numpy._typing namespace; that's all.
There was a problem hiding this comment.
Thanks! Do I see it right that the comparisons are typed so broadly for scalars that at the moment they will simply never fail?
There was a problem hiding this comment.
Thanks! Do I see it right that the comparisons are typed so broadly for scalars that at the moment they will simply never fail?
Only for__eq__/__ne__; for the others they, in the example oftimedelta64, accepts one of four types:
- A numeric scalar (the first parameter)
- A numeric array-like (second parameter)
- An object implementing
__lt__or__gt__(hard coded in_ComparisonOp) - A (nested) sequence whose elements implement
__lt__or__gt__(also hard coded in_ComparisonOp)
The __lt__ / __gt__ escape hatch here is rather broad, though it doesn't quite allow any arbitrary type. Unfortunately it's very difficult to narrow the types down to something more specific, and we need these ones in particular for certain object-based comparison paths (e.g. those involving fractions.Fraction or decimal.Decimal; xref #21939).
__lt__: _ComparisonOp[_NumberLike_co, _ArrayLikeNumber_co]
__le__: _ComparisonOp[_NumberLike_co, _ArrayLikeNumber_co]
__gt__: _ComparisonOp[_NumberLike_co, _ArrayLikeNumber_co]
__ge__: _ComparisonOp[_NumberLike_co, _ArrayLikeNumber_co]|
Any chance this could be resurrected, @seberg? I wouldn't mind taking care of the typing stuff, if that helps. |
|
@jorenham I would like this. The code changes really should still be practically good, it's just that The problem here is a decision problem and maybe checking for fallout (or just seeing what happens, since the astropy/pandas as the most interesting projects test in nightly CI anyway). Unlike fixing string promotion, I suppose this might have fewer things that lead to weird stuff. |
Draft: But only because it depends on gh-22566What is the fallout of this? No idea, lets find out! The main thing that magically stops working, are comparisons like
timedelta64(3, "s") > 0. Of course it seems they didn't work for 0-D scalars anyway most of the time.(My 1.19.5 NumPy cannot do it for timdelta's if there is a unit attached. Somewhere along the way I fixed the promotion to not drop the unit and made it work.)
Now, astropy's units do allow explicitly comparisons to 0, because can be somewhat defined. But that isn't really plausible at this time (it might be in a NEP 50 future).
Is this the right thing to do? Yes, most definitely. Can we do this? I guess we see what astropy and pandas notice, but I guess it has to wait until after branching :(.