Skip to content
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

BUG: Fix timedelta64+Timestamp, closes #24775 #26916

Merged
merged 9 commits into from Jun 25, 2019

Conversation

@jbrockmendel
Copy link
Member

commented Jun 18, 2019

  • closes #24775
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@@ -112,3 +112,9 @@ def test_addition_subtraction_preserve_frequency(self):
td64 = np.timedelta64(1, 'D')
assert (ts + td64).freq == original_freq
assert (ts - td64).freq == original_freq

def test_radd_timedelta64(self):

This comment has been minimized.

Copy link
@jreback

jreback Jun 18, 2019

Contributor

seems like a lone test; can u co-locate with same for Timedelta and (or parametrize)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 18, 2019

Author Member

This is definitely the appropriate place. I think there are other tests in tests.scalar.timestamp that belong in this file

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2019

Contributor

my point is to also include pd.Timedelta here as well via parameterization. I suspect we already test this elsewhere, so rather than add a one-off, should add elsewhere (or simply combine / move them here if this is the final destination).

(pandas) bash-3.2$ grep -r _radd pandas/tests/ --include '*.py'
pandas/tests//tseries/offsets/test_offsets.py:    def test_radd(self):
pandas/tests//frame/test_query_eval.py:            for op_str, op, rop in [('+', '__add__', '__radd__'),
pandas/tests//arithmetic/test_datetime64.py:        if op_str not in ['__add__', '__radd__', '__sub__']:
pandas/tests//arithmetic/test_datetime64.py:        if op_str not in ['__add__', '__radd__', '__sub__', '__rsub__']:
pandas/tests//arithmetic/test_datetime64.py:    @pytest.mark.parametrize('op', ['__add__', '__radd__',
pandas/tests//arithmetic/test_timedelta64.py:    # Tests for timedelta64[ns] __add__, __sub__, __radd__, __rsub__
pandas/tests//arithmetic/test_numeric.py:    # __add__, __sub__, __radd__, __rsub__, __iadd__, __isub__
pandas/tests//arithmetic/test_numeric.py:    def test_series_frame_radd_bug(self):
pandas/tests//arithmetic/test_object.py:    def test_objarr_radd_str(self, box):
pandas/tests//arithmetic/test_object.py:    def test_objarr_radd_str_invalid(self, dtype, data, box):
pandas/tests//arithmetic/test_object.py:    def test_series_with_dtype_radd_timedelta(self, dtype):
pandas/tests//scalar/timedelta/test_arithmetic.py:        __add__, __radd__,
pandas/tests//scalar/timedelta/test_arithmetic.py:            # datetime + Timedelta does _not_ call Timedelta.__radd__,
pandas/tests//indexes/test_category.py:        (lambda idx: ['a', 'b'] + idx, '__radd__'),

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 19, 2019

Author Member

I suspect we already test this elsewhere, so rather than add a one-off, should add elsewhere (or simply combine / move them here if this is the final destination).

well we don't already test this specifically, since this test fails on master. Most of the grep results posted are for non-scalars; we definitely don't want to start mixing the scalar tests in with those. I'll collect other Timestamp arithmetic tests that are currently misplaced and put them in this file.

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2019

Contributor

I suspect we already test this elsewhere, so rather than add a one-off, should add elsewhere (or simply combine / move them here if this is the final destination).

well we don't already test this specifically, since this test fails on master. Most of the grep results posted are for non-scalars; we definitely don't want to start mixing the scalar tests in with those. I'll collect other Timestamp arithmetic tests that are currently misplaced and put them in this file.

If you read my point is that we certaily test this for Timedelta. so these belong together.

@codecov

This comment has been minimized.

Copy link

commented Jun 18, 2019

Codecov Report

Merging #26916 into master will decrease coverage by 50.76%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #26916       +/-   ##
===========================================
- Coverage   91.87%    41.1%   -50.77%     
===========================================
  Files         180      180               
  Lines       50712    50712               
===========================================
- Hits        46590    20845    -25745     
- Misses       4122    29867    +25745
Flag Coverage Δ
#multiple ?
#single 41.1% <ø> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/plotting/_matplotlib/__init__.py 0% <0%> (-100%) ⬇️
pandas/io/gcs.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/io/s3.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.16%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.1%) ⬇️
... and 132 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baa77c3...9666416. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Jun 18, 2019

Codecov Report

Merging #26916 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26916      +/-   ##
==========================================
- Coverage   91.99%   91.98%   -0.01%     
==========================================
  Files         180      180              
  Lines       50774    50774              
==========================================
- Hits        46712    46707       -5     
- Misses       4062     4067       +5
Flag Coverage Δ
#multiple 90.63% <ø> (ø) ⬆️
#single 41.83% <ø> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 88.88% <0%> (-11.12%) ⬇️
pandas/core/frame.py 96.89% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.84% <0%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83fe8d7...534db4c. Read the comment docs.

@@ -112,3 +112,9 @@ def test_addition_subtraction_preserve_frequency(self):
td64 = np.timedelta64(1, 'D')
assert (ts + td64).freq == original_freq
assert (ts - td64).freq == original_freq

def test_radd_timedelta64(self):

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2019

Contributor

my point is to also include pd.Timedelta here as well via parameterization. I suspect we already test this elsewhere, so rather than add a one-off, should add elsewhere (or simply combine / move them here if this is the final destination).

(pandas) bash-3.2$ grep -r _radd pandas/tests/ --include '*.py'
pandas/tests//tseries/offsets/test_offsets.py:    def test_radd(self):
pandas/tests//frame/test_query_eval.py:            for op_str, op, rop in [('+', '__add__', '__radd__'),
pandas/tests//arithmetic/test_datetime64.py:        if op_str not in ['__add__', '__radd__', '__sub__']:
pandas/tests//arithmetic/test_datetime64.py:        if op_str not in ['__add__', '__radd__', '__sub__', '__rsub__']:
pandas/tests//arithmetic/test_datetime64.py:    @pytest.mark.parametrize('op', ['__add__', '__radd__',
pandas/tests//arithmetic/test_timedelta64.py:    # Tests for timedelta64[ns] __add__, __sub__, __radd__, __rsub__
pandas/tests//arithmetic/test_numeric.py:    # __add__, __sub__, __radd__, __rsub__, __iadd__, __isub__
pandas/tests//arithmetic/test_numeric.py:    def test_series_frame_radd_bug(self):
pandas/tests//arithmetic/test_object.py:    def test_objarr_radd_str(self, box):
pandas/tests//arithmetic/test_object.py:    def test_objarr_radd_str_invalid(self, dtype, data, box):
pandas/tests//arithmetic/test_object.py:    def test_series_with_dtype_radd_timedelta(self, dtype):
pandas/tests//scalar/timedelta/test_arithmetic.py:        __add__, __radd__,
pandas/tests//scalar/timedelta/test_arithmetic.py:            # datetime + Timedelta does _not_ call Timedelta.__radd__,
pandas/tests//indexes/test_category.py:        (lambda idx: ['a', 'b'] + idx, '__radd__'),
def test_compare_zerodim_array(self):
# GH#26916
ts = Timestamp.now()
td64 = np.datetime64('2016-01-01', 'ns')

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2019

Contributor

this is odd named td64 no?

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 19, 2019

Author Member

typo, will update

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Fixed typo, determined there's only a couple other dedicated Timestamp arith tests, moved them over to the appropriate place and parametrized them.

We've done a good job at making tslibs self-contained, less so with the tests. At some point I'd like to change that, hopefully make it easier to identify gaps in isolated coverage.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Fixed typo, determined there's only a couple other dedicated Timestamp arith tests, moved them over to the appropriate place and parametrized them.

We've done a good job at making tslibs self-contained, less so with the tests. At some point I'd like to change that, hopefully make it easier to identify gaps in isolated coverage.

yep i here ya increasing coverage always +1

@jreback jreback added this to the 0.25.0 milestone Jun 21, 2019

td = np.timedelta64(3, 'h')
assert td + ts == ts + td

@pytest.mark.parametrize('other,exdiff', [

This comment has been minimized.

Copy link
@jreback

jreback Jun 21, 2019

Contributor

can you do: exdiff -> expected_difference

@TomAugspurger
Copy link
Contributor

left a comment

This seems good. Can you fix the merge conflict and merge on green?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

i have some open requests here

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

i have some open requests here

I think your open request is that we put this with the Timedelta arithmetic tests. But doing so would break the organization of the Timedelta tests.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

no my requests are to parametrize over a Timedelta the tests u added

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

no my requests are to parametrize over a Timedelta the tests u added

added.

@jreback jreback merged commit f0919f2 into pandas-dev:master Jun 25, 2019

12 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20190624.19 succeeded
Details
pandas-dev.pandas (Checks) Checks succeeded
Details
pandas-dev.pandas (Docs) Docs succeeded
Details
pandas-dev.pandas (Linux py35_compat) Linux py35_compat succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow) Linux py36_locale_slow succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow_old_np) Linux py36_locale_slow_old_np succeeded
Details
pandas-dev.pandas (Linux py37_locale) Linux py37_locale succeeded
Details
pandas-dev.pandas (Linux py37_np_dev) Linux py37_np_dev succeeded
Details
pandas-dev.pandas (Windows py36_np15) Windows py36_np15 succeeded
Details
pandas-dev.pandas (Windows py37_np141) Windows py37_np141 succeeded
Details
pandas-dev.pandas (macOS py35_macos) macOS py35_macos succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the jbrockmendel:tsadd branch Jun 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.