-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: GH11349 where Series.apply and Series.map did not box timedelta64 #11564
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
One test failing OK:
Fail:
I think because Solutions I can think of all seem quite intrusive:
Besides this, is there any need to have |
why would this be failing?
|
I meant if you are to box
|
@kawochen I c. yes, I think that
I think what happens now is that the reversed methods are called and for example |
Addtl cases which should all be the same (e.g. should all raise a Ideally also would have more uniform / better error messages (e.g. [21]) is hard to understand and [22] is deferring to numpy and even more tricky to understand.
Now [20] actually may be impossible to fix as we have the same |
@kawochen lmk progress on this. |
@kawochen can you update |
will update soon. but I think |
@kawochen these interactions are already defined in |
seems to depend on whether
Maybe this is better, since a
Since |
|
OK. How about these? Should all of them be
|
So if we have timedelta/timedelta it returns a float, otherwise tries to preserve type.
I think would be ok to make [3] return |
Should Should |
yes in all |
OK. Lots to change 😄 |
Just realized |
yep suppose that dividing by floats should also work |
c2e6ae5
to
fd4f128
Compare
@@ -879,10 +879,10 @@ def test_timedelta_ops_with_missing_values(self): | |||
actual = s1 - timedelta_NaT | |||
assert_series_equal(actual, sn) | |||
|
|||
actual = s1 + NA |
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 don't see why this should work? Does it make certain operations convenient?
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 just proves coercion of nan
-> NaT
when the dtype is appropriate (e.g. M8[ns]
or m8[ns]
.
I would expect this to work (as opposed to raising an error)
-> actual = s1 + NA
(Pdb) p s1
0 00:00:01
dtype: timedelta64[ns]
(Pdb) p NA
nan
(Pdb) p s1 + NA
0 NaT
dtype: timedelta64[ns]
(Pdb) p s1 + pd.NaT
0 NaT
dtype: timedelta64[ns]
with dates
(Pdb) Series(pd.date_range('20130101',periods=2))
0 2013-01-01
1 2013-01-02
dtype: datetime64[ns]
(Pdb) Series(pd.date_range('20130101',periods=2))+pd.NaT
*** TypeError: can only operate on a datetimes for subtraction, but the operator [__add__] was passed
(Pdb) Series(pd.date_range('20130101',periods=2))+np.nan
*** TypeError: can only operate on a datetimes for subtraction, but the operator [__add__] was passed
(Pdb) Series(pd.date_range('20130101',periods=2))-np.nan
0 NaT
1 NaT
dtype: timedelta64[ns]
(Pdb) Series(pd.date_range('20130101',periods=2))-pd.NaT
0 NaT
1 NaT
dtype: timedelta64[ns]
So a naked |
result = s1.astype("timedelta64[{0}]".format(unit)) | ||
assert_series_equal(result, expected) | ||
|
||
# reverse op | ||
expected = s1.apply(lambda x: np.timedelta64(m,unit) / x) | ||
expected = s1.apply(lambda x: Timedelta(np.timedelta64(m,unit)) / x) |
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.
should you just add a test here? (e.g. for both wrapped np.timedelta64
and Timedelta
)
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.
without wrapping it doesn't work, since np.timedelta64
doesn't return NotImplemented
-- it raises TypeError
directly
fd4f128
to
18dd422
Compare
updated |
18dd422
to
46bd000
Compare
@@ -167,6 +167,56 @@ Backwards incompatible API changes | |||
- The parameter ``out`` has been removed from the ``Series.round()`` method. (:issue:`11763`) | |||
- ``DataFrame.round()`` leaves non-numeric columns unchanged in its return, rather than raises. (:issue:`11885`) | |||
|
|||
NaT and timedelta64 operations | |||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
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.
make underline same len as title (avoids build errors)
@kawochen really just some minor comments. looks really good! lots of edge cases you figured out! |
@kawochen can you check this: This is on master
|
this last fails, xref #11925 |
8f43e6d
to
7f07269
Compare
updated |
@@ -299,7 +300,18 @@ def __init__(self, left, right, name, na_op): | |||
self.is_datetime64tz_rhs = is_datetime64tz_dtype(rvalues) | |||
self.is_datetime_rhs = self.is_datetime64_rhs or self.is_datetime64tz_rhs | |||
self.is_timedelta_rhs = is_timedelta64_dtype(rvalues) | |||
if not (isinstance(rvalues, pd.Series) or |
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.
can you put a comment indicating why this is necessary here (the expression to determine if its a timedelta)
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.
updated. the checking was actually not needed, so I removed it
@kawochen very minor comment. ping when pushed. |
7f07269
to
296c042
Compare
296c042
to
513c5c8
Compare
BUG: GH11349 where Series.apply and Series.map did not box timedelta64
@kawochen awesome fixes! This really cleaned lots of edge cases. keep em coming! |
http://pandas-docs.github.io/pandas-docs-travis/whatsnew.html#nat-and-timedelta-operations There is this naked exception.
can you raise an informative message here? |
yes sorry I made a mistake in the example. will get to it tonight. |
closes #11349
closes #11925