BUG: Categorical comparison with unordered #16339

TomAugspurger · 2017-05-12T13:07:14Z

Fixes categorical comparison operations improperly considering
ordering when two unordered categoricals are compared.

codecov · 2017-05-12T13:41:28Z

Codecov Report

Merging #16339 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16339      +/-   ##
==========================================
- Coverage   90.36%   90.34%   -0.02%     
==========================================
  Files         161      161              
  Lines       50897    50905       +8     
==========================================
- Hits        45993    45992       -1     
- Misses       4904     4913       +9

Flag	Coverage Δ
#multiple	`88.13% <100%> (ø)`	⬆️
#single	`40.18% <0%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/categorical.py	`95.9% <100%> (+0.04%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.59% <0%> (-0.1%)`	⬇️

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 50e95e0...2d47107. Read the comment docs.

codecov · 2017-05-12T13:41:34Z

Codecov Report

❗ No coverage uploaded for pull request base (master@539de79). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #16339   +/-   ##
=========================================
  Coverage          ?    90.4%           
=========================================
  Files             ?      161           
  Lines             ?    50975           
  Branches          ?        0           
=========================================
  Hits              ?    46086           
  Misses            ?     4889           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`88.18% <100%> (?)`
#single	`40.19% <0%> (?)`

Impacted Files	Coverage Δ
pandas/core/categorical.py	`95.91% <100%> (ø)`

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 539de79...4ec26d4. Read the comment docs.

jorisvandenbossche

Looks good!

jorisvandenbossche · 2017-05-12T15:50:14Z

pandas/tests/test_categorical.py

+        result = c1 == c2
+
+        assert result[0]
+        assert not result[1]


I personally find tm.assert_array_equal(c1 == c2, np.array([True, False])) clearer in this case (but that is maybe taste :-))

jorisvandenbossche · 2017-05-12T16:10:12Z

pandas/tests/test_categorical.py

+    def test_unordered_different_order_equal(self):
+        # https://github.com/pandas-dev/pandas/issues/16014
+        c1 = Categorical(['a', 'b'], categories=['a', 'b'], ordered=False)
+        c2 = Categorical(['a', 'b'], categories=['b', 'a'], ordered=False)


A lot of the tests above are with Series instead of Categorical.
Don't need to change all, but maybe good to test this with a Series as well

Hopefully the new version isn't too weird: https://github.com/pandas-dev/pandas/pull/16339/files#diff-ed4f442894a2f521dfac3193a3a8d8a0R3825

jreback

minor comments, otherwise lgtm.

jreback · 2017-05-12T22:59:48Z

doc/source/categorical.rst

@@ -453,6 +453,14 @@ the original values:

    np.asarray(cat) > base

+When you compare two unordered categoricals with the same categories, the order is not considered:
+


versionadded tag

I think not necessary since it's a bugfix.

jreback · 2017-05-12T23:00:42Z

pandas/core/categorical.py

+            msg = ("Categoricals can only be compared if "
+                   "'categories' are the same")
+            if len(self.categories) != len(other.categories):
+                raise TypeError(msg)


maybe this could be a more specialized message (e.g the len comparison)

jorisvandenbossche · 2017-05-15T20:44:48Z

pandas/tests/test_categorical.py

+        c2 = Categorical(['a', 'c'], categories=['c', 'a'], ordered=False)
+        with pytest.raises(TypeError) as rec:
+            c1 == c2
+        assert rec.match("Categoricals can only be compared")


I think we should choose a way to do this, this way or the tm.assert_raises_regex

jorisvandenbossche · 2017-05-15T20:54:59Z

pandas/core/categorical.py

-            na_mask = (self._codes == -1) | (other._codes == -1)
+            if not self.ordered:
+                # Comparison uses codes, so align theirs to ours
+                other_codes = _get_codes_for_values(other, self.categories)


This is only needed when the categories are not equal?

Might be good to do a quick perf check for the basic case of comparing categoricals with equal categories

good catch. Just pushed a commit fixing this to only do the recoding when the categories don't match.

jreback · 2017-05-15T23:55:53Z

pandas/core/categorical.py

+            msg = ("Categoricals can only be compared if "
+                   "'categories' are the same.")
+            if len(self.categories) != len(other.categories):
+                raise TypeError(msg + " Categories are different lengths")


I think you are including the origninal message here, but it its a little awkward

What did you have in mind? I like how it's currently "Categoricals can only be compared if 'categories' are the same. Categories are different lengths." Since it's the general problem (different categories) and a specific hint
as to what's wrong (different lengths)

actually this is fine.

jreback

minor comment, lgtm.

jorisvandenbossche · 2017-05-17T07:54:53Z

pandas/core/categorical.py

            if not (self.ordered == other.ordered):
                raise TypeError("Categoricals can only be compared if "
                                "'ordered' is the same")
-            na_mask = (self._codes == -1) | (other._codes == -1)
+            if not self.categories.equals(other.categories):


maybe you can do a if not self.ordered and and not self.categories.equals(..) to avoid doing this check if the ordered case (when the check is not needed, as it was already checked above)

Fixes categorical comparison operations improperly considering ordering when two unordered categoricals are compared. Closes pandas-dev#16014

* upstream/master: (48 commits) BUG: Categorical comparison with unordered (pandas-dev#16339) ENH: Adding 'protocol' parameter to 'to_pickle'. PERF: improve MultiIndex get_loc performance (pandas-dev#16346) TST: remove pandas-datareader xfail as 0.4.0 works (pandas-dev#16374) TST: followup to pandas-dev#16364, catch errstate warnings (pandas-dev#16373) DOC: new oauth token TST: Add test for clip-na (pandas-dev#16369) ENH: Draft metadata specification doc for Apache Parquet (pandas-dev#16315) MAINT: Add .iml to .gitignore (pandas-dev#16368) BUG/API: Categorical constructor scalar categories (pandas-dev#16340) ENH: Provide dict object for to_dict() pandas-dev#16122 (pandas-dev#16220) PERF: improved clip performance (pandas-dev#16364) DOC: try new token for docs DOC: try with new secure token DOC: add developer section to the docs DEPS: Drop Python 3.4 support (pandas-dev#16303) DOC: remove credential helper DOC: force fetch on build docs DOC: redo dev docs access token DOC: add dataframe construction in merge_asof example (pandas-dev#16348) ...

Fixes categorical comparison operations improperly considering ordering when two unordered categoricals are compared. Closes pandas-dev#16014

Fixes categorical comparison operations improperly considering ordering when two unordered categoricals are compared. Closes pandas-dev#16014 (cherry picked from commit 91e9e52)

Fixes categorical comparison operations improperly considering ordering when two unordered categoricals are compared. Closes #16014 (cherry picked from commit 91e9e52)

Fixes categorical comparison operations improperly considering ordering when two unordered categoricals are compared. Closes pandas-dev#16014

TomAugspurger added the Categorical Categorical Data Type label May 12, 2017

TomAugspurger added this to the 0.20.2 milestone May 12, 2017

jorisvandenbossche reviewed May 12, 2017

View reviewed changes

jreback approved these changes May 12, 2017

View reviewed changes

TomAugspurger force-pushed the unordered-equal branch from 2d47107 to 9910de4 Compare May 15, 2017 18:56

jorisvandenbossche reviewed May 15, 2017

View reviewed changes

jreback reviewed May 15, 2017

View reviewed changes

jorisvandenbossche reviewed May 17, 2017

View reviewed changes

BUG: Categorical comparison with unordered

4ec26d4

Fixes categorical comparison operations improperly considering ordering when two unordered categoricals are compared. Closes pandas-dev#16014

TomAugspurger force-pushed the unordered-equal branch from d619a76 to 4ec26d4 Compare May 18, 2017 11:27

TomAugspurger merged commit 91e9e52 into pandas-dev:master May 18, 2017

TomAugspurger deleted the unordered-equal branch May 29, 2017 20:44

TomAugspurger added Backported and removed Needs Backport labels May 30, 2017

TomAugspurger mentioned this pull request Jun 5, 2017

CategoricalIndex.equals incorrectly considers category order when unordered #16603

Closed

TomAugspurger mentioned this pull request Mar 20, 2018

Major Bug: Concatenating Categorical Columns Yields Jumbled Result #20425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Categorical comparison with unordered #16339

BUG: Categorical comparison with unordered #16339

TomAugspurger commented May 12, 2017

codecov bot commented May 12, 2017

codecov bot commented May 12, 2017 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche May 12, 2017

jorisvandenbossche May 12, 2017

TomAugspurger May 15, 2017

jreback left a comment

jreback May 12, 2017

TomAugspurger May 15, 2017

jreback May 12, 2017

jorisvandenbossche May 15, 2017

jorisvandenbossche May 15, 2017

TomAugspurger May 15, 2017

jreback May 15, 2017

TomAugspurger May 16, 2017

jreback May 17, 2017

jreback left a comment

jorisvandenbossche May 17, 2017

		@@ -453,6 +453,14 @@ the original values:

		np.asarray(cat) > base

		When you compare two unordered categoricals with the same categories, the order is not considered:

BUG: Categorical comparison with unordered #16339

BUG: Categorical comparison with unordered #16339

Conversation

TomAugspurger commented May 12, 2017

codecov bot commented May 12, 2017

Codecov Report

codecov bot commented May 12, 2017 • edited Loading

Codecov Report

jorisvandenbossche 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

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

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 12, 2017 •

edited

Loading