[Bug] Comparison between two PeriodIndexes doesn't validate length (GH #23078) #23896

makbigc · 2018-11-25T14:23:09Z

closes comparison between two PeriodIndexes doesn't validate length #23078
tests added
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-11-25T14:23:12Z

Hello @makbigc! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 25, 2018 at 11:36 Hours UTC

codecov · 2018-11-25T14:56:22Z

Codecov Report

Merging #23896 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23896      +/-   ##
==========================================
+ Coverage    92.3%   92.31%   +<.01%     
==========================================
  Files         163      163              
  Lines       51950    51954       +4     
==========================================
+ Hits        47953    47961       +8     
+ Misses       3997     3993       -4

Flag	Coverage Δ
#multiple	`90.72% <100%> (ø)`	⬆️
#single	`42.99% <25%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/base.py	`97.59% <100%> (+0.04%)`	⬆️
pandas/core/arrays/period.py	`98.48% <100%> (ø)`	⬆️
pandas/util/testing.py	`87.84% <0%> (+0.09%)`	⬆️
pandas/core/arrays/datetimelike.py	`96.57% <0%> (+0.21%)`	⬆️
pandas/core/arrays/integer.py	`96.23% <0%> (+0.68%)`	⬆️

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 159772d...ac94453. Read the comment docs.

pandas/tests/indexes/period/test_period.py

jreback

let's make this a general check in the base class instead

jreback · 2018-11-26T13:20:01Z

pandas/core/arrays/period.py

@@ -73,6 +73,9 @@ def wrapper(self, other):
                msg = DIFFERENT_FREQ_INDEX.format(self.freqstr, other.freqstr)
                raise IncompatibleFrequency(msg)

+            if other.ndim > 0 and len(self) != len(other):


this should fail if ndim of other != 1

we don't generally check this for other arrays (though we should). So I would maybe add this check on the base array class and just call it here, _validate_equal(self, other) or something

By base class, do you mean ExtensionOpsMixin?

And I would call it something like _validate_shape or _check_shape.

yes, I think we have an implicit guarantee on 1-D ness now (or at the very least the pandas extension types do).

Related #23853

TomAugspurger · 2018-11-26T13:29:01Z

@makbigc when you update for @jreback's feedback, can you add a couple new base tests to BaseComparisonOpsTests and BaseArithmeticOpsTests ensure that a ValueError is raised when comparing / operating on two EAs with different lengths? These are in tests/extension/base/ops.py.

jreback · 2018-12-09T19:52:43Z

@makbigc can you merge master and update

jreback · 2018-12-23T15:51:08Z

this overlaps with #24397 @jbrockmendel can you comment here

jbrockmendel · 2018-12-24T02:06:53Z

pandas/tests/extension/decimal/test_decimal.py

@@ -299,6 +299,12 @@ def _check_divmod_op(self, s, op, other, exc=NotImplementedError):
    def test_error(self):
        pass

+    def test_arith_diff_lengths(self):


are these tests passed because they would fail? if so, mark with @pytest.mark.xfail.

jbrockmendel · 2018-12-24T02:08:20Z

pandas/tests/extension/base/ops.py

@@ -119,6 +119,12 @@ def test_direct_arith_with_series_returns_not_implemented(self, data):
                "{} does not implement add".format(data.__class__.__name__)
            )

+    def test_arith_diff_lengths(self, data, all_arithmetic_operators):
+        op = self.get_op_from_name(all_arithmetic_operators)
+        other = data[:3]


This test actually wouldn't catch the behavior in #23078, which is caused specifically by comparisons of PeriodIndex with length-1 objects.

The issue in #23078 was treated in the function test_comp_op (pandas/tests/indexes/period/test_period.py).

jreback

why are you adding tests to each of the EA subclasses? the base test should work for each of these.

jreback · 2018-12-25T17:13:48Z

doc/source/whatsnew/v0.24.0.rst

@@ -1319,6 +1319,7 @@ Datetimelike
 - Bug in :class:`DatetimeIndex` and :class:`TimedeltaIndex` where indexing with ``Ellipsis`` would incorrectly lose the index's ``freq`` attribute (:issue:`21282`)
 - Clarified error message produced when passing an incorrect ``freq`` argument to :class:`DatetimeIndex` with ``NaT`` as the first entry in the passed data (:issue:`11587`)
 - Bug in :func:`to_datetime` where ``box`` and ``utc`` arguments were ignored when passing a :class:`DataFrame` or ``dict`` of unit mappings (:issue:`23760`)
+- Bug in :class:`PeriodIndex` when comparing indexes of different lengths, ValueError is not raised (:issue:`23078`)


use double backticks around ValueError

jreback · 2018-12-25T17:14:44Z

pandas/tests/extension/base/ops.py

+    def test_arith_diff_lengths(self, data, all_arithmetic_operators):
+        op = self.get_op_from_name(all_arithmetic_operators)
+        other = data[:3]
+        with pytest.raises(ValueError):


can you match the message as well

jreback · 2018-12-25T17:15:07Z

pandas/tests/extension/base/ops.py

+    def test_compare_diff_lengths(self, data, all_compare_operators):
+        op = self.get_op_from_name(all_compare_operators)
+        other = data[:3]
+        with pytest.raises(ValueError):


match the message

jreback · 2018-12-25T17:15:35Z

pandas/tests/extension/decimal/test_decimal.py

@@ -324,6 +331,12 @@ def test_compare_array(self, data, all_compare_operators):
                                   for i in alter]
        self._compare_other(s, data, op_name, other)

+    # TODO:


why are these xfail? make the change as needed in the decimal code

jreback · 2018-12-25T17:15:45Z

pandas/tests/extension/json/test_json.py

@@ -291,9 +291,13 @@ def _check_divmod_op(self, s, op, other, exc=NotImplementedError):
            s, op, other, exc=TypeError
        )

+    def test_arith_diff_lengths(self):


why are these added?

makbigc · 2018-12-26T14:04:38Z

Should I close this PR? Because #23078 was already resolved by #24397. The length check is done in the wrapper of _period_array_cmp. This is not need to add _validate_shape additionally.

The generic base test cannot handle all six EA subclasses:

Integer array: deprecationwarning raised by numpy
category: only eq and ne comparison are implemented
json: no json comparison

The raise of ValueError for DecimalArray could be implemented in a new PR.
@jreback @TomAugspurger Thank you for your review.

makbigc changed the title ~~Bug fix (GH #23078)~~ [Bug] Comparison between two PeriodIndexes doesn't validate length (GH #23078) Nov 25, 2018

TomAugspurger reviewed Nov 25, 2018

View reviewed changes

pandas/tests/indexes/period/test_period.py Show resolved Hide resolved

TomAugspurger approved these changes Nov 25, 2018

View reviewed changes

TomAugspurger added the Period Period data type label Nov 25, 2018

TomAugspurger added this to the 0.24.0 milestone Nov 25, 2018

jreback requested changes Nov 26, 2018

View reviewed changes

jorisvandenbossche added the ExtensionArray Extending pandas with custom dtypes or arrays. label Nov 26, 2018

jreback mentioned this pull request Nov 26, 2018

REF/CLN: ops boilerplate #23853

Open

4 tasks

jreback removed this from the 0.24.0 milestone Nov 29, 2018

makbigc force-pushed the bug-fix-23078 branch 2 times, most recently from 9b0b723 to 47bf1bf Compare December 23, 2018 07:58

jbrockmendel reviewed Dec 24, 2018

View reviewed changes

makbigc added 6 commits December 25, 2018 19:35

Bug fix (GH pandas-dev#23078)

9bca2c9

Add _validate_shape

d64c7df

Add test: ValueError raised when operating arrays of diff lengths

2892ee3

Add filterwarnings to test_compare_diff_lengths in test_integer.py

83c619f

Change the argument of filterwarning

154e279

Add xfail remark

ac94453

makbigc force-pushed the bug-fix-23078 branch from 4d2ea4e to ac94453 Compare December 25, 2018 11:36

jreback requested changes Dec 25, 2018

View reviewed changes

makbigc closed this Dec 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Comparison between two PeriodIndexes doesn't validate length (GH #23078) #23896

[Bug] Comparison between two PeriodIndexes doesn't validate length (GH #23078) #23896

makbigc commented Nov 25, 2018 •

edited

Loading

pep8speaks commented Nov 25, 2018 •

edited

Loading

codecov bot commented Nov 25, 2018 •

edited

Loading

jreback left a comment

jreback Nov 26, 2018

TomAugspurger Nov 26, 2018

TomAugspurger Nov 26, 2018

jreback Nov 26, 2018

jbrockmendel Nov 26, 2018

TomAugspurger commented Nov 26, 2018 •

edited

Loading

jreback commented Dec 9, 2018

jreback commented Dec 23, 2018

jbrockmendel Dec 24, 2018

jbrockmendel Dec 24, 2018

makbigc Dec 25, 2018

jreback left a comment

jreback Dec 25, 2018

jreback Dec 25, 2018

jreback Dec 25, 2018

jreback Dec 25, 2018

jreback Dec 25, 2018

makbigc commented Dec 26, 2018

[Bug] Comparison between two PeriodIndexes doesn't validate length (GH #23078) #23896

[Bug] Comparison between two PeriodIndexes doesn't validate length (GH #23078) #23896

Conversation

makbigc commented Nov 25, 2018 • edited Loading

pep8speaks commented Nov 25, 2018 • edited Loading

Comment last updated on December 25, 2018 at 11:36 Hours UTC

codecov bot commented Nov 25, 2018 • edited Loading

Codecov Report

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

TomAugspurger commented Nov 26, 2018 • edited Loading

jreback commented Dec 9, 2018

jreback commented Dec 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

makbigc commented Dec 26, 2018

makbigc commented Nov 25, 2018 •

edited

Loading

pep8speaks commented Nov 25, 2018 •

edited

Loading

codecov bot commented Nov 25, 2018 •

edited

Loading

TomAugspurger commented Nov 26, 2018 •

edited

Loading