Implement missing offset comparison methods #18738

jbrockmendel · 2017-12-12T02:19:26Z

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Not sure if it closes #8386, but its most of the way there.

Fix some missing comparisons, including Timedelta with Tick and Tick with (timedelta|Timedelta|Week)

Attach correct names for comparison methods.

If we can resolve #18510, we can make Week(weekday=None) just return a Tick, avoid some special casing.

jreback · 2017-12-12T11:07:40Z

pandas/tests/scalar/test_timedelta.py

+        assert isinstance(delta, Timedelta)
+        assert delta < offset
+        assert delta <= offset
+        assert not delta == offset


these are not correct comparisons, eg.. not a == b is equiv to a != b and the linter complains about the former.

If the linting is complaining I can change it to explicitly call e.g not a.__ne__(b). The methods have distinct implementations, need distinct tests.

jreback · 2017-12-12T11:08:36Z

pandas/tests/tseries/offsets/test_ticks.py

+    with pytest.raises(TypeError):
+        off < 3
+
+    # Unfortunately there is no good way to make the reverse inequality work


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.

jreback · 2017-12-12T11:11:28Z

pandas/tseries/offsets.py

+            if isinstance(other, DateOffset):
+                return f(self, other)
+            else:
+                if op == operator.eq:


why are you repeating

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

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.

jreback · 2017-12-12T11:11:55Z

pandas/tseries/offsets.py

+                                        other=other.__class__.__name__))
+        elif op == operator.eq:
+            # TODO: Consider changing this older behavior for
+            # __eq__ and __ne__to match other comparisons


what is this about?

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.

jreback · 2017-12-12T11:12:46Z

Not sure if it closes #8386, but its most of the way there.

if most of the way there, then let's take it over the line.

jbrockmendel · 2017-12-12T19:08:00Z

if most of the way there, then let's take it over the line.

#8386 can be split into two parts. First is unimplemented comparison methods, which this PR addresses as many of as I could find.

Second is that author implicitly expects to_offset('W') to return pd.offsets.Week(weekday=None) (which will after this PR be comparable as that author expects), but to_offset('W') actually returns pd.offsets.Week(weekday=6), which remains non-comparable. This latter part of that issue is not addressed in this PR, and I want to leave it up to @edrevo to decide if this fully solves the problem.

codecov · 2017-12-12T22:04:11Z

Codecov Report

Merging #18738 into master will decrease coverage by <.01%.
The diff coverage is 82.05%.

@@            Coverage Diff             @@
##           master   #18738      +/-   ##
==========================================
- Coverage   91.67%   91.66%   -0.01%     
==========================================
  Files         154      154              
  Lines       51368    51388      +20     
==========================================
+ Hits        47092    47106      +14     
- Misses       4276     4282       +6

Flag	Coverage Δ
#multiple	`89.53% <82.05%> (-0.01%)`	⬇️
#single	`40.83% <35.89%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`96.53% <82.05%> (-0.42%)`	⬇️

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 75b97a7...c3f7a1f. Read the comment docs.

edrevo · 2017-12-13T07:41:43Z

@jbrockmendel , I'm OK with this fix. Many thanks for the PR!!

jreback · 2017-12-16T00:01:28Z

pandas/tests/tseries/offsets/test_offsets.py

+        off <= Day(n=7)
+
+
+def test_comparison_names(offset_types):


can you parametrize

…fsets_cmp

jreback

add a whatsnew note for #8386 see if anything listed there which should be a tests, if not lmk that.

jreback · 2017-12-21T18:49:15Z

pandas/tests/tseries/offsets/test_offsets.py

@@ -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):


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

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

jreback · 2017-12-21T18:49:58Z

pandas/tests/tseries/offsets/test_offsets.py

+        off < DateOffset(month=7)
+
+
+def test_week_comparison():


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

jreback · 2017-12-21T18:51:02Z

pandas/tseries/offsets.py

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


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?

jreback · 2017-12-21T18:51:13Z

pandas/tseries/offsets.py

+            # Week without weekday behaves like a Tick
+            tick = Day(n=7 * self.n, normalize=self.normalize)
+            return op(tick, other)
+        else:


no need for the else

jreback · 2017-12-21T18:52:12Z

pandas/tseries/offsets.py

+            other_delta = other.delta
+        elif isinstance(other, (timedelta, np.timedelta64)):
+            other_delta = other
+        elif isinstance(other, Week) and other.weekday is None:


what happens when other.weekday is not None?

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

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.

jbrockmendel · 2017-12-28T19:49:08Z

Handling array-like comparisons correctly is going to take some additional work. Temporarily closing to clear the queue.

jbrockmendel added 3 commits December 11, 2017 17:50

handle comparison of Timedelta with Tick

e7d1f4a

implement comparison methods more thoroughly

857db79

tests for timedelta comparisons

e18168b

jreback requested changes Dec 12, 2017

View reviewed changes

jreback added the Frequency DateOffsets label Dec 12, 2017

fix mis-specified test

6a97205

This was referenced Dec 13, 2017

Week.onOffset looks fishy #18510

Closed

Offsets Roundup #18854

Closed

jreback requested changes Dec 21, 2017

View reviewed changes

jbrockmendel added 2 commits December 21, 2017 08:17

parametrize test

343dc4c

Merge branch 'master' of https://github.com/pandas-dev/pandas into of…

c3f7a1f

…fsets_cmp

jreback requested changes Dec 21, 2017

View reviewed changes

jreback added API Design Compat pandas objects compatability with Numpy or Python functions labels Dec 21, 2017

jbrockmendel closed this Dec 28, 2017

jbrockmendel deleted the offsets_cmp branch April 5, 2020 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement missing offset comparison methods #18738

Implement missing offset comparison methods #18738

jbrockmendel commented Dec 12, 2017

jreback Dec 12, 2017

jbrockmendel Dec 12, 2017

jreback Dec 12, 2017

jbrockmendel Dec 12, 2017

jreback Dec 12, 2017

jbrockmendel Dec 12, 2017

jreback Dec 12, 2017

jbrockmendel Dec 12, 2017

jreback commented Dec 12, 2017

jbrockmendel commented Dec 12, 2017

codecov bot commented Dec 12, 2017 •

edited

Loading

edrevo commented Dec 13, 2017

jreback Dec 16, 2017

jreback left a comment

jreback Dec 21, 2017

jreback Dec 21, 2017

jreback Dec 21, 2017

jreback Dec 21, 2017

jbrockmendel Dec 27, 2017

jreback Dec 21, 2017

jreback Dec 21, 2017

jreback Dec 21, 2017

jbrockmendel Dec 27, 2017

jbrockmendel commented Dec 28, 2017

Implement missing offset comparison methods #18738

Implement missing offset comparison methods #18738

Conversation

jbrockmendel commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 12, 2017

jbrockmendel commented Dec 12, 2017

codecov bot commented Dec 12, 2017 • edited Loading

Codecov Report

edrevo commented Dec 13, 2017

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Dec 28, 2017

codecov bot commented Dec 12, 2017 •

edited

Loading