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

Fix Index __mul__-like ops with timedelta scalars #19333

Merged
merged 29 commits into from
Feb 22, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
fecc906
update whatsnew
jbrockmendel Jan 21, 2018
7e8d5ff
add rmul test to timedeltas
jbrockmendel Jan 21, 2018
d2423ca
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 21, 2018
1dd4ccf
handle Index * timedelta, with tests
jbrockmendel Jan 21, 2018
71811c8
whatsnew note
jbrockmendel Jan 21, 2018
4c2c74b
division tests, fix typos
jbrockmendel Jan 21, 2018
d45ba92
Fix typo
jbrockmendel Jan 21, 2018
4abd61a
restore whitespace
jbrockmendel Jan 21, 2018
11424b0
fill in GH issue
jbrockmendel Jan 21, 2018
ee48c8b
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 21, 2018
1c97489
whitespace fixup from merge
jbrockmendel Jan 21, 2018
23b7f44
arrange whatsnew notes
jbrockmendel Jan 22, 2018
086be08
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 22, 2018
6e04aba
comment, requested edits
jbrockmendel Jan 22, 2018
1012939
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 23, 2018
2fb3688
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 25, 2018
0ed9276
xfail RangeIndex test
jbrockmendel Jan 25, 2018
e798653
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 25, 2018
a04f970
fix pytest xfail problem
jbrockmendel Jan 25, 2018
878be31
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 31, 2018
d62983d
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Feb 1, 2018
040b1e4
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Feb 2, 2018
d7dfba9
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Feb 5, 2018
a46acb9
remove xfail
jbrockmendel Feb 13, 2018
e6fa53b
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Feb 13, 2018
814e858
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Feb 21, 2018
080c06c
troubleshoot RangeIndex test failure
jbrockmendel Feb 21, 2018
22d155e
fixup reversed op
jbrockmendel Feb 21, 2018
9e3dec4
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Feb 21, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/source/whatsnew/v0.23.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ Timezones
- Bug in comparing :class:`DatetimeIndex`, which failed to raise ``TypeError`` when attempting to compare timezone-aware and timezone-naive datetimelike objects (:issue:`18162`)
- Bug in localization of a naive, datetime string in a ``Series`` constructor with a ``datetime64[ns, tz]`` dtype (:issue:`174151`)
- :func:`Timestamp.replace` will now handle Daylight Savings transitions gracefully (:issue:`18319`)
-
- Bug in tz-aware :class:`DatetimeIndex` where addition/subtraction with a :class:`TimedeltaIndex` or array with ``dtype='timedelta64[ns]'`` was incorrect (:issue:`17558`)
- Bug in :func:`DatetimeIndex.insert` where inserting ``NaT`` into a timezone-aware index incorrectly raised (:issue:`16357`)

Expand All @@ -446,6 +447,9 @@ Numeric
- Bug in :class:`Index` multiplication and division methods where operating with a ``Series`` would return an ``Index`` object instead of a ``Series`` object (:issue:`19042`)
- Bug in the :class:`DataFrame` constructor in which data containing very large positive or very large negative numbers was causing ``OverflowError`` (:issue:`18584`)
- Bug in :class:`Index` constructor with ``dtype='uint64'`` where int-like floats were not coerced to :class:`UInt64Index` (:issue:`18400`)
- Bug in :class:`Index` multiplication and division methods where operating with a ``Series`` would return an ``Index`` object instead of a ``Series`` object (:issue:`19042`)
- Multiplication of :class:`TimedeltaIndex` by ``TimedeltaIndex`` will now raise ``TypeError`` instead of raising ``ValueError`` in cases of length mis-match (:issue`19333`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st one ok, other 2 in datetimelike section

- Multiplication and division of numeric-dtyped :class:`Index` objects with timedelta-like scalars returns ``TimedeltaIndex`` instead of raising ``TypeError`` (:issue:`19333`)

-

Expand Down
23 changes: 20 additions & 3 deletions pandas/core/indexes/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import numpy as np
from pandas._libs import (lib, index as libindex, tslib as libts,
algos as libalgos, join as libjoin,
Timestamp)
Timestamp, Timedelta)
from pandas._libs.lib import is_datetime_array

from pandas.compat import range, u, set_function_name
Expand All @@ -16,7 +16,7 @@
from pandas.core.dtypes.generic import (
ABCSeries, ABCDataFrame,
ABCMultiIndex,
ABCPeriodIndex,
ABCPeriodIndex, ABCTimedeltaIndex,
ABCDateOffset)
from pandas.core.dtypes.missing import isna, array_equivalent
from pandas.core.dtypes.common import (
Expand Down Expand Up @@ -3864,7 +3864,21 @@ def dropna(self, how='any'):
return self._shallow_copy()

def _evaluate_with_timedelta_like(self, other, op, opstr, reversed=False):
raise TypeError("can only perform ops with timedelta like values")
# Timedelta knows how to operate with np.array, so dispatch to that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be left alone, and rather define this in TimedeltaIndex?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for for numeric-dtyped indexes. e.g. pd.Index(range(3)) * pd.Timedelta(days=1) goes through this path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why doesn't this just raise NotImplementedError? which then Timedelta would handle? or is that too recursive a path?

Copy link
Member Author

@jbrockmendel jbrockmendel Jan 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For one thing b/c there’s pytimedelta and timedelta64 that need to be handled.

# operation and then wrap the results
other = Timedelta(other)
values = self.values
if reversed:
values, other = other, values

with np.errstate(all='ignore'):
result = op(values, other)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add some comments (or expand the top ones), this code does a lot


attrs = self._get_attributes_dict()
attrs = self._maybe_update_attributes(attrs)
if op == divmod:
return Index(result[0], **attrs), Index(result[1], **attrs)
return Index(result, **attrs)

def _evaluate_with_datetime_like(self, other, op, opstr):
raise TypeError("can only perform ops with datetime like values")
Expand Down Expand Up @@ -4024,6 +4038,9 @@ def _make_evaluate_binop(op, opstr, reversed=False, constructor=Index):
def _evaluate_numeric_binop(self, other):
if isinstance(other, (ABCSeries, ABCDataFrame)):
return NotImplemented
elif isinstance(other, ABCTimedeltaIndex):
# Defer to subclass implementation
return NotImplemented

other = self._validate_for_numeric_binop(other, op, opstr)

Expand Down
5 changes: 4 additions & 1 deletion pandas/core/indexes/range.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
is_integer,
is_scalar,
is_int64_dtype)
from pandas.core.dtypes.generic import ABCSeries
from pandas.core.dtypes.generic import ABCSeries, ABCTimedeltaIndex

from pandas import compat
from pandas.compat import lrange, range, get_range_parameters
Expand Down Expand Up @@ -587,6 +587,9 @@ def _make_evaluate_binop(op, opstr, reversed=False, step=False):
def _evaluate_numeric_binop(self, other):
if isinstance(other, ABCSeries):
return NotImplemented
elif isinstance(other, ABCTimedeltaIndex):
# Defer to TimedeltaIndex implementation
return NotImplemented

other = self._validate_for_numeric_binop(other, op, opstr)
attrs = self._get_attributes_dict()
Expand Down
32 changes: 31 additions & 1 deletion pandas/tests/indexes/test_numeric.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import pandas.util.testing as tm

import pandas as pd
from pandas._libs.lib import Timestamp
from pandas._libs.lib import Timestamp, Timedelta

from pandas.tests.indexes.common import Base

Expand All @@ -26,6 +26,36 @@ def full_like(array, value):
return ret


class TestIndexArithmetic(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this class its index with timedelta scalar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment. future readers are going to have no idea of where to find tests

@pytest.mark.parametrize('index', [pd.Int64Index(range(1, 11)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the issue number as a comment (or PR number if no issue)

pd.UInt64Index(range(1, 11)),
pd.Float64Index(range(1, 11)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this how we are importing in this module, e.g. pd.*? usually we don't do this (its ok whichever, just want to be consistent)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I'm being inconsistent here, will change.

pd.RangeIndex(1, 11)])
@pytest.mark.parametrize('scalar_td', [Timedelta(days=1),
Timedelta(days=1).to_timedelta64(),
Timedelta(days=1).to_pytimedelta()])
def test_index_mul_timedelta(self, scalar_td, index):
expected = pd.timedelta_range('1 days', '10 days')

result = index * scalar_td
tm.assert_index_equal(result, expected)
commute = scalar_td * index
tm.assert_index_equal(commute, expected)

@pytest.mark.parametrize('index', [pd.Int64Index(range(1, 3)),
pd.UInt64Index(range(1, 3)),
pd.Float64Index(range(1, 3)),
pd.RangeIndex(1, 3)])
@pytest.mark.parametrize('scalar_td', [Timedelta(days=1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests really belong with the scalar timedelta tests I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah I thought about that and eventually decided that since the affected code was in the Index class, the test belonged in the Index tests. But either way works for me.

Timedelta(days=1).to_timedelta64(),
Timedelta(days=1).to_pytimedelta()])
def test_index_rdiv_timedelta(self, scalar_td, index):
expected = pd.TimedeltaIndex(['1 Day', '12 Hours'])

result = scalar_td / index
tm.assert_index_equal(result, expected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also test that the commute raises here



class Numeric(Base):

def test_numeric_compat(self):
Expand Down
16 changes: 15 additions & 1 deletion pandas/tests/indexes/timedeltas/test_arithmetic.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ def test_dti_mul_dti_raises(self):

def test_dti_mul_too_short_raises(self):
idx = self._holder(np.arange(5, dtype='int64'))
with pytest.raises(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the problem is now caught in TimedeltaIndex when checking what it is operating against, whereas before the problem was caught in numpy when trying to operate on differently-sized arrays.

with pytest.raises(TypeError):
idx * self._holder(np.arange(3))
with pytest.raises(ValueError):
idx * np.array([1, 2])
Expand Down Expand Up @@ -476,6 +476,20 @@ def test_ops_compat(self):
# don't allow division by NaT (make could in the future)
pytest.raises(TypeError, lambda: rng / pd.NaT)

@pytest.mark.parametrize('other', [np.arange(1, 11),
pd.Int64Index(range(1, 11)),
pd.UInt64Index(range(1, 11)),
pd.Float64Index(range(1, 11)),
pd.RangeIndex(1, 11)])
def test_tdi_rmul_arraylike(self, other):
tdi = TimedeltaIndex(['1 Day'] * 10)
expected = timedelta_range('1 days', '10 days')

result = other * tdi
tm.assert_index_equal(result, expected)
commute = tdi * other
tm.assert_index_equal(commute, expected)

def test_subtraction_ops(self):
# with datetimes/timedelta and tdi/dti
tdi = TimedeltaIndex(['1 days', pd.NaT, '2 days'], name='foo')
Expand Down