Skip to content

Implement missing offset comparison methods #18738

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 7 additions & 1 deletion pandas/_libs/tslibs/timedeltas.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,13 @@ cdef class _Timedelta(timedelta):
return PyObject_RichCompare(np.array([self]), other, op)
return PyObject_RichCompare(other, self, reverse_ops[op])
else:
if op == Py_EQ:
if (getattr(other, "_typ", "") == "dateoffset" and
hasattr(other, "delta")):
# offsets.Tick; we catch this fairly late as it is a
# relatively infrequent case
ots = other.delta
return cmp_scalar(self.value, ots.value, op)
elif op == Py_EQ:
return False
elif op == Py_NE:
return True
Expand Down
15 changes: 15 additions & 0 deletions pandas/tests/scalar/test_timedelta.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,24 @@
from pandas.core.tools.timedeltas import _coerce_scalar_to_timedelta_type as ct
from pandas import (Timedelta, TimedeltaIndex, timedelta_range, Series,
to_timedelta, compat)
from pandas.tseries.frequencies import to_offset
from pandas._libs.tslib import iNaT, NaT


class TestTimedeltaComparisons(object):
@pytest.mark.parametrize('freq', ['D', 'H', 'T', 's', 'ms', 'us', 'ns'])
def test_tick_comparison(self, freq):
offset = to_offset(freq) * 2
delta = offset._inc
assert isinstance(delta, Timedelta)
assert delta < offset
assert delta <= offset
assert not delta == offset
assert delta != offset
assert not delta > offset
assert not delta >= offset


class TestTimedeltaArithmetic(object):
_multiprocess_can_split_ = True

Expand Down
49 changes: 49 additions & 0 deletions pandas/tests/tseries/offsets/test_offsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -3147,3 +3147,52 @@ def test_require_integers(offset_types):
cls = offset_types
with pytest.raises(ValueError):
cls(n=1.5)


def test_comparisons(offset_types):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be broken up a bit, its very hard to read / determine what is allowed and what is not.

IOW you need 3 tests
Week, Tick, Everything else

Copy link
Contributor

Choose a reason for hiding this comment

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

and the Tick test should be in tests_ticks (and you have one already there)

cls = offset_types

if cls is WeekOfMonth:
# TODO: The default values for week and weekday should be non-raising
off = cls(n=1, week=1, weekday=2)
elif cls is LastWeekOfMonth:
# TODO: The default value for weekday should be non-raising
off = cls(n=1, weekday=4)
else:
off = cls(n=1)

if cls is Week:
assert off < timedelta(days=8)
assert off > timedelta(days=6)
assert off <= Day(n=7)
elif issubclass(cls, offsets.Tick):
pass
else:
with pytest.raises(TypeError):
off < timedelta(days=8)
with pytest.raises(TypeError):
off > timedelta(days=6)
with pytest.raises(TypeError):
off <= Day(n=7)
with pytest.raises(TypeError):
off < DateOffset(month=7)


def test_week_comparison():
Copy link
Contributor

Choose a reason for hiding this comment

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

and you already have week (yes this is constructed differently), but pls group these together.

# Only Week with weekday == None is special
off = Week(weekday=3)
with pytest.raises(TypeError):
off < timedelta(days=8)
with pytest.raises(TypeError):
off > timedelta(days=6)
with pytest.raises(TypeError):
off <= Day(n=7)


@pytest.mark.parametrize('opname', ['__eq__', '__ne__',
'__lt__', '__le__',
'__gt__', '__ge__'])
def test_comparison_names(offset_types, opname):
cls = offset_types
method = getattr(cls, opname)
assert method.__name__ == opname
21 changes: 20 additions & 1 deletion pandas/tests/tseries/offsets/test_ticks.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@

from pandas import Timedelta, Timestamp
from pandas.tseries import offsets
from pandas.tseries.offsets import Hour, Minute, Second, Milli, Micro, Nano
from pandas.tseries.offsets import (Hour, Minute, Second, Milli, Micro, Nano,
Week)

from .common import assert_offset_equal

Expand All @@ -35,6 +36,24 @@ def test_delta_to_tick():
assert (tick == offsets.Day(3))


@pytest.mark.parametrize('cls', tick_classes)
def test_tick_comparisons(cls):
off = cls(n=2)
with pytest.raises(TypeError):
off < 3

# Unfortunately there is no good way to make the reverse inequality work
Copy link
Contributor

Choose a reason for hiding this comment

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

huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can make Tick.__richmp__(timedelta) work, but we can't make timedelta.__richcmp__(Tick) work. AFAICT datetime for deferring to other but timedelta does not.

assert off > timedelta(-1)
assert off >= timedelta(-1)
assert off < off._inc * 3 # Timedelta object
assert off <= off._inc * 3 # Timedelta object
assert off == off.delta
assert off.delta == off
assert off != -1 * off

assert off < Week()


# ---------------------------------------------------------------------


Expand Down
93 changes: 62 additions & 31 deletions pandas/tseries/offsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,24 @@ def wrapper(self, other):
return wrapper


def _make_cmp_func(op):
assert op not in [operator.eq, operator.ne]
# __eq__ and __ne__ have slightly different behavior, returning
# False and True, respectively, instead of raising

def cmp_func(self, other):
if type(self) == Week and self.weekday is None:
# Week without weekday behaves like a Tick
Copy link
Contributor

Choose a reason for hiding this comment

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

blank line

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but for clarification, I thought that wasn't necessary when the pre-comment line has different indentation. Is there a reference for this I can bookmark?

tick = Day(n=7 * self.n, normalize=self.normalize)
return op(tick, other)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the else

raise TypeError('Cannot compare type {self} with {other}'
.format(self=self.__class__.__name__,
other=other.__class__.__name__))

cmp_func.__name__ = '__{name}__'.format(name=op.__name__)
return cmp_func

# ---------------------------------------------------------------------
# DateOffset

Expand Down Expand Up @@ -299,6 +317,11 @@ def _repr_attrs(self):
def name(self):
return self.rule_code

__lt__ = _make_cmp_func(operator.lt)
__le__ = _make_cmp_func(operator.le)
__ge__ = _make_cmp_func(operator.ge)
__gt__ = _make_cmp_func(operator.gt)

def __eq__(self, other):
if other is None:
return False
Expand All @@ -320,9 +343,7 @@ def __hash__(self):
return hash(self._params())

def __add__(self, other):
if isinstance(other, (ABCDatetimeIndex, ABCSeries)):
return other + self
elif isinstance(other, ABCPeriod):
if isinstance(other, (ABCDatetimeIndex, ABCSeries, ABCPeriod)):
return other + self
try:
return self.apply(other)
Expand Down Expand Up @@ -2146,8 +2167,41 @@ def onOffset(self, dt):

def _tick_comp(op):
def f(self, other):
return op(self.delta, other.delta)
if isinstance(other, Tick):
# Note we cannot just try/except other.delta because Tick.delta
# returns a Timedelta while Timedelta.delta returns an int
other_delta = other.delta
elif isinstance(other, (timedelta, np.timedelta64)):
other_delta = other
elif isinstance(other, Week) and other.weekday is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens when other.weekday is not None?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would lump all Week in this case (use if statements inside if needed)

Copy link
Member Author

Choose a reason for hiding this comment

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

what happens when other.weekday is not None?

TypeError

I would lump all Week in this case (use if statements inside if needed)

Leads to a bunch of duplicated code if we do it that way.

other_delta = timedelta(weeks=other.n)
elif isinstance(other, compat.string_types):
from pandas.tseries.frequencies import to_offset
other = to_offset(other)
if isinstance(other, DateOffset):
return f(self, other)
else:
if op == operator.eq:
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you repeating

isn't
return op(self, other) enough here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean in line 2181 returning f(self, other) instead of op(self, other)? Because the the behavior still depends on which DateOffset subclass other is.

return False
elif op == operator.ne:
return True
raise TypeError('Cannot compare type {self} and {other}'
.format(self=self.__class__.__name__,
other=other.__class__.__name__))
elif op == operator.eq:
# TODO: Consider changing this older behavior for
# __eq__ and __ne__to match other comparisons
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this about?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the status quo __eq__ returns False (and __ne__ returns True) as the fallback when other cannot be converted to a DateOffset. The other comparisons raise in this situation. (I think Timedelta behaves the same way, so this might be OK). This is just a note to double-check that we __eq__ and __ne__ should continue to be treated as special cases.

return False
elif op == operator.ne:
return True
else:
raise TypeError('Cannot compare type {self} and {other}'
.format(self=self.__class__.__name__,
other=other.__class__.__name__))

return op(self.delta, other_delta)

f.__name__ = '__{name}__'.format(name=op.__name__)
return f


Expand Down Expand Up @@ -2184,34 +2238,11 @@ def __add__(self, other):
raise OverflowError("the add operation between {self} and {other} "
"will overflow".format(self=self, other=other))

def __eq__(self, other):
if isinstance(other, compat.string_types):
from pandas.tseries.frequencies import to_offset

other = to_offset(other)

if isinstance(other, Tick):
return self.delta == other.delta
else:
# TODO: Are there cases where this should raise TypeError?
return False

# This is identical to DateOffset.__hash__, but has to be redefined here
# for Python 3, because we've redefined __eq__.
def __hash__(self):
return hash(self._params())

def __ne__(self, other):
if isinstance(other, compat.string_types):
from pandas.tseries.frequencies import to_offset

other = to_offset(other)

if isinstance(other, Tick):
return self.delta != other.delta
else:
# TODO: Are there cases where this should raise TypeError?
return True
# This is identical to DateOffset.__hash__, but has to be redefined
# here for Python 3, because we've redefined __eq__.
tup = (str(self.__class__), ('n', self.n))
return hash(tup)

@property
def delta(self):
Expand Down