-
-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
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 |
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
doc/source/whatsnew/v0.18.0.txt
Outdated
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 |
pandas/core/ops.py
Outdated
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! |
|
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