BUG: Ensure sign(timedelta64('nat')) returns timedelta64('nat')
#29498
Conversation
WarrenWeckesser
force-pushed
the
sign-nat
branch
from
0ffb0be to
90fff9a
Compare
jorenham
changed the title
sign(timedelta64('nat')) returns timedelta64('nat')
|
Oops... thanks @jorenham. If this gets merged, the commit message should also be fixed to say 'timedelta64' instead of 'datetime64'. I can force-push an updated commit, but I'll hold off until someone has a chance to review. |
squash merging is also an option |
| nat = np.timedelta64('nat', *args) | ||
| sgn = np.sign(nat) | ||
| assert sgn.dtype == nat.dtype | ||
| assert np.isnat(sgn) |
There was a problem hiding this comment.
I confirmed that the test fails locally when reverting the source patch.
I also checked that on this branch, with multiple array entries, the behavior still looks sensible (I think):
nat = np.timedelta64('nat', *args)
arr = np.array([np.timedelta64(-1, 'h'), nat, nat, np.timedelta64(1, 'h')])
sgn = np.sign(arr)
print(sgn)
# [ -1 'NaT' 'NaT' 1] This seems almost straightforward enough for me to merge directly, but I'm probably sufficiently inactive that I should let another dev take a quick look.
| @@ -581,6 +581,15 @@ def test_timedelta_scalar_construction_units(self): | |||
| assert_equal(np.datetime64('now').dtype, | |||
| np.dtype('M8[s]')) | |||
|
|
|||
| @pytest.mark.parametrize('args', [(), ('s',)]) | |||
There was a problem hiding this comment.
In case of non-NaT values, with 'ns' and 'M' you get _.item() -> int, but with 's' you het _.item() -> datetime.timedelta. If that's irrelevant for the C changes here, then feel free to ignore me, but otherwise something like this could help test it:
| @pytest.mark.parametrize('args', [(), ('s',)]) | |
| @pytest.mark.parametrize('args', [(), ('ns', 's', 'M')]) |
|
I am confused, did you actually land on the sign of timedelta should be a timedelta at all? |
|
@seberg wrote
Returning timedelta64 is already the current behavior. This PR only changes the specific value returned when the input to Changing the return type for |
|
I suppose it's not clear to me that there is a point in fixing this if we want to change the return type. Unless we have a rough agreement to make the return type float, so that we can preserve NaT there by going to NaN. But OK, if you think this is best, since it clearly returns nonsense right now, fine. I was just slightly worried that transitioning away from it might just get harder with this. |
|
Yeah, I get your point. I'll mark this as draft for now, and return to it (if necessary) when the bigger issue of the |
Closes gh-29496.