-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Fix Series.__sub__ non-nano datetime64 #18783
Conversation
pandas/core/ops.py
Outdated
@@ -671,7 +676,7 @@ def _arith_method_SERIES(op, name, str_rep, fill_zeros=None, default_axis=None, | |||
""" | |||
def na_op(x, y): | |||
import pandas.core.computation.expressions as expressions | |||
|
|||
# |
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.
pls don't add extraneous things
res = dt64 - ser | ||
tm.assert_series_equal(res, -expected) | ||
|
||
# check for DatetimeIndex and DataFrame while we're at it |
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 the comment relevant
res = dt64 - dti | ||
tm.assert_index_equal(res, pd.Index(-expected)) | ||
|
||
# TODO: This is still broken for ser.to_frame() |
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.
then if you want to add a test with an xfail would be helpful. (and an issue reference), TODO's are only so good.
@@ -28,6 +28,31 @@ | |||
from .common import TestData | |||
|
|||
|
|||
class TestDatetimeLikeArithmetic(object): | |||
def test_sub_datetime64_not_ns(self): | |||
# GH#7996 |
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.
parameterize this
Codecov Report
@@ Coverage Diff @@
## master #18783 +/- ##
==========================================
+ Coverage 91.59% 91.59% +<.01%
==========================================
Files 150 150
Lines 48959 48961 +2
==========================================
+ Hits 44843 44847 +4
+ Misses 4116 4114 -2
Continue to review full report at Codecov.
|
Timeout on circleCI in collection, test_geopandas in travis. Will wait to re-push until given the go-ahead. |
yeah don't re-push anything. |
def test_sub_datetime64_not_ns(self): | ||
# GH#7996 | ||
ser = Series(date_range('20130101', periods=3)) | ||
dt64 = np.datetime64('2013-01-01') |
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.
test with a tz-aware as well (as I think that may hit your path, unless its getting caught earlier)
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 gets caught earlier: tz compat raises ValueError in ops._validate
tm.assert_index_equal(res, pd.Index(-expected)) | ||
|
||
@pytest.mark.xfail(reason='GH#7996 datetime64 units not converted to nano') | ||
def test_frame_sub_datetime64_not_ns(self): |
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.
do we have a separate issue for this? if not let's create one and reference it (as closing #7996 with this PR)
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.
Just opened #18874.
tm.assert_series_equal(res, expected) | ||
|
||
res = dt64 - ser | ||
tm.assert_series_equal(res, -expected) |
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 parametrize
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 without introducing a whole lot of extra boilerplate.
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 sure that is the case. you are repeating Series/DTI testing.
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.
OK, just changed. It is much less obvious to a casual reader exactly what this is testing.
tm.assert_series_equal(res, expected) | ||
|
||
res = dt64 - ser | ||
tm.assert_series_equal(res, -expected) |
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 sure that is the case. you are repeating Series/DTI testing.
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.
small change. ping on green.
@@ -962,6 +962,34 @@ def test_timedelta64_ops_nat(self): | |||
|
|||
|
|||
class TestDatetimeSeriesArithmetic(object): | |||
@pytest.mark.parametrize('box_cls, assert_func', [(Series, |
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 formatting is a bit atypical, put a more like
@pytest.mark.parametrize(
'box, assert_func',
[(Series, tm.assert_series_equal),
(pd.Index, tm.assert_index_equal)])
is much better, also rename box_cls
-> box
(consistent with elsewhere)
Ping |
This somehow doesn't need a whatsnew merge |
thanks! |
The original bug report was for
Series
. This fixes that bug and includes a test that checks forDatetimeIndex
while we're at it. I checked and this does not fix the analogous problem in `DataFrame. I'm hoping someone else will pick up the torch on that b/c the broadcast/dispatch is still something of a mystery to me.git diff upstream/master -u -- "*.py" | flake8 --diff