Support for use of Enums in MultiIndex #21348

benediamond · 2018-06-07T00:49:51Z

closes Support for Enums in MultiIndex #21298
tests added / passed: added test_use_enum_in_multiindex in tests/indexes/test_multi.py, see diff
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

The boolean flag in

pandas/pandas/core/arrays/categorical.py

Lines 2526 to 2528 in 9f95f7d

    
           cat = Categorical(values, ordered=True) 
        
           categories = cat.categories 
        
           codes = cat.codes

incorrectly caused a TypeError to be raised in

pandas/pandas/core/arrays/categorical.py

Lines 352 to 354 in 9f95f7d

    
           raise TypeError("'values' is not ordered, please " 
        
                           "explicitly specify the categories order " 
        
                           "by passing in a categories argument.")

when an Enum was used in a MultiIndex. No tests fail when the flag is flipped back. This is fixed in this PR.

Due to an unnecessary "orderedness" check in `arrays/categorical.py`'s method `_factorize_from_iterable`, MultiIndex's constructor inappropriately threw a `TypeError` when passed an Enum as one of its iterables. This has been fixed.

benediamond · 2018-06-07T02:37:31Z

CircleCI failed due to unavailability of module enum in test environment. I don't know how else to test this---should I simply remove the new test I added?

gfyoung · 2018-06-07T05:09:01Z

CircleCI failed due to unavailability of module enum in test environment. I don't know how else to test this---should I simply remove the new test I added?

@benediamond : I believe the build that's failing is a Python 2.7 build. I would use pytest's skip functionality to skip whenever we're in Python 2 land.

jreback · 2018-06-07T11:24:40Z

pandas/tests/indexes/test_multi.py

+        # cannot actually be built yet.
+        from pandas.core.algorithms import take_1d
+
+        def _get_ilevel_values(index, level):


what is all this for?

simply construct the expected index directly

The thought here is that the expected index cannot be constructed directly, due to the exact bug I'm trying to fix. Thus instead I create an expected "level", and verify that it matches the relevant level of df's index. This is done by replicating a sub-step of tm.assert_index_equal.

jreback · 2018-06-07T11:24:46Z

pandas/tests/indexes/test_multi.py

+            values = unique._shallow_copy(filled, name=index.names[level])
+            return values
+
+        from enum import Enum


all imports go at the top

jreback · 2018-06-07T11:27:29Z

pandas/tests/indexes/test_multi.py

+            return values
+
+        from enum import Enum
+        MyEnum = Enum("MyEnum", "A B")


you would have to add enum34 (the 2.7 backport) in our 2.7 builds

(pandas) bash-3.2$ grep python=2.7 ci/*.yaml appveyor-27.yaml: - python=2.7.* circle-27-compat.yaml: - python=2.7* travis-27-locale.yaml: - python=2.7 travis-27.yaml: - python=2.7*

ok, I've hopefully done this correctly.

codecov · 2018-06-07T13:12:03Z

Codecov Report

Merging #21348 into master will decrease coverage by 0.39%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #21348     +/-   ##
=========================================
- Coverage   92.28%   91.89%   -0.4%     
=========================================
  Files         161      153      -8     
  Lines       51500    49596   -1904     
=========================================
- Hits        47528    45576   -1952     
- Misses       3972     4020     +48

Flag	Coverage Δ
#multiple	`90.29% <100%> (-0.39%)`	⬇️
#single	`41.86% <100%> (-0.45%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/categorical.py	`95.69% <100%> (+0.34%)`	⬆️
pandas/core/arrays/base.py	`83.95% <0%> (-13.41%)`	⬇️
pandas/util/_doctools.py	`0% <0%> (-12.88%)`	⬇️
pandas/io/parquet.py	`71.79% <0%> (-12.83%)`	⬇️
pandas/core/reshape/util.py	`90.32% <0%> (-9.68%)`	⬇️
pandas/util/_decorators.py	`82.25% <0%> (-9.09%)`	⬇️
pandas/io/formats/html.py	`88.85% <0%> (-8.83%)`	⬇️
pandas/core/dtypes/base.py	`92.1% <0%> (-7.9%)`	⬇️
pandas/errors/__init__.py	`92.3% <0%> (-7.7%)`	⬇️
pandas/core/common.py	`92.09% <0%> (-6.32%)`	⬇️
... and 144 more

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 20ae454...06437cd. Read the comment docs.

Added `enum34` dependency

benediamond · 2018-06-07T21:32:05Z

@gfyoung I'm stumped by this whatsnew conflict, any thoughts?

gfyoung · 2018-06-08T03:58:22Z

doc/source/whatsnew/v0.23.1.txt

@@ -88,6 +88,7 @@ Indexing
 - Bug in :meth:`MultiIndex.set_names` where error raised for a ``MultiIndex`` with ``nlevels == 1`` (:issue:`21149`)
 - Bug in :class:`IntervalIndex` constructors where creating an ``IntervalIndex`` from categorical data was not fully supported (:issue:`21243`, issue:`21253`)
 - Bug in :meth:`MultiIndex.sort_index` which was not guaranteed to sort correctly with ``level=1``; this was also causing data misalignment in particular :meth:`DataFrame.stack` operations (:issue:`20994`, :issue:`20945`, :issue:`21052`)
+- Bug in :func:`pandas.core.arrays.categorical._factorize_from_iterable` inappropriately caused ``TypeError`` to be raised when an ``Enum`` was used as a factor in a ``MultiIndex`` (:issue:`21298`)


@gfyoung I'm stumped by this whatsnew conflict, any thoughts?

@benediamond : I would actually move your whatsnew addition to 0.24.0. That will suffice. 😄

I've gone ahead and just removed it---I'm not sure where it belongs, as it appeared that this PR (when it was passing) was slated for the next milestone.

@benediamond : I would put it in 0.24.0, as I suggested above. We will need a whatsnew for this for sure, and that would be the best place to put it.

@gfyoung Done. this one didn't create merge conflicts, thankfully

gfyoung · 2018-06-08T17:36:54Z

pandas/tests/indexes/test_multi.py

+                labels, fill_value=unique._na_value
+            )
+            values = unique._shallow_copy(filled, name=index.names[level])
+            return values


Given that you use this function only once, I think this abstraction is unnecessary as @jreback alluded to in an earlier comment.

Ok, amended. Thanks for your guidance.

gfyoung · 2018-06-08T17:37:16Z

pandas/tests/indexes/test_multi.py

+        # GH 21298
+        # Allow use of Enums as one of the factors in a MultiIndex.
+        from enum import Enum
+        from pandas.core.algorithms import take_1d


By "top", @jreback meant move the imports to "top of the file" 😄

Hah whoops. done

This nested function had been drawn directly from _within_ `pands.util.testing.assert_index_equal`. As there is no way to construct the target dataframe's (multi) index directly (hence this PR), I simply compare this index's _0th level_ to a manually constructed (simple) index. This comparison is given by a subroutine of `assert_index_equal` (which applies the subroutine to _each_ level).

gfyoung · 2018-06-08T23:17:15Z

pandas/core/arrays/categorical.py

@@ -2523,7 +2523,7 @@ def _factorize_from_iterable(values):
                                      ordered=values.ordered)
        codes = values.codes
    else:
-        cat = Categorical(values, ordered=True)
+        cat = Categorical(values, ordered=False)


Interesting that this didn't cause any testing issues...

Yes, it's striking. It's possible that thatindexes.multi is essentially the only caller of categorical._factorize_from_iterable, and that the calls that multi makes happen to be such that within Categorical's constructor, control is always routed so that the only time that ordered is used is to incorrectly raise the TypeError on lines 352-354. That's my best-guess explanation, at least.

gfyoung

Well, this looks pretty good to me!

cc @jreback

benediamond · 2018-06-12T15:27:17Z

@jreback bumping this.

jreback

pls add a whatsnew note for 0.24

jreback · 2018-06-19T00:12:49Z

ci/circle-27-compat.yaml

@@ -22,6 +22,7 @@ dependencies:
  # universal
  - pytest
  - pytest-xdist
+  - enum34


can you remove from all but 1 of the 27 compat tests (the travis-27) one

jreback · 2018-06-19T00:14:41Z

pandas/tests/indexes/test_multi.py

@@ -3307,3 +3308,28 @@ def test_duplicate_multiindex_labels(self):
        with pytest.raises(ValueError):
            ind.set_levels([['A', 'B', 'A', 'A', 'B'], [2, 1, 3, -2, 5]],
                           inplace=True)
+
+    def test_use_enum_in_multiindex(self):


can you use @td.skip_if_no('enum') here (then do the actual import inside), we have to guard against the tests being run on 2.7 w/o the compat library (by a user).

https://github.com/benediamond/pandas/blob/4cd2dff96775a828687acd7c65919ec135824cf4/pandas/tests/indexes/test_multi.py#L3310-L3314

jreback · 2018-06-19T00:16:47Z

pandas/tests/indexes/test_multi.py

+        filled = take_1d(unique.values, labels, fill_value=unique._na_value)
+        df_index_0 = unique._shallow_copy(filled, name=df.columns.names[0])
+        exp_index_0 = pd.Index([MyEnum.A, MyEnum.A, MyEnum.B, MyEnum.B],
+                               dtype='object')


this is pretty complicated, why cannot you just directly construct the Index here?

https://github.com/benediamond/pandas/blob/4cd2dff96775a828687acd7c65919ec135824cf4/pandas/tests/indexes/test_multi.py#L3321-L3322

benediamond · 2018-06-19T17:29:25Z

for whatsnew, see: https://github.com/benediamond/pandas/blob/4cd2dff96775a828687acd7c65919ec135824cf4/doc/source/whatsnew/v0.24.0.txt#L132-L135

benediamond · 2018-08-12T18:38:07Z

can you use @td.skip_if_no('enum') here (then do the actual import inside), we have to guard against the tests being run on 2.7 w/o the compat library (by a user).

@jreback implemented here:

    @td.skip_if_no('enum')
    def test_use_enum_in_multiindex(self):
        # GH 21298
        # Allow use of Enums as one of the factors in a MultiIndex.
        from enum import Enum
        MyEnum = Enum("MyEnum", "A B")
        ...

but yields:

==================================== ERRORS ====================================
_____________ ERROR collecting pandas/tests/indexes/test_multi.py ______________
pandas/tests/indexes/test_multi.py:30: in <module>
    class TestMultiIndex(Base):
pandas/tests/indexes/test_multi.py:3310: in TestMultiIndex
    @td.skip_if_no('enum')
E   NameError: name 'td' is not defined
------- generated xml file: /tmp/circle-junit.2ZpyFLd/reports/junit.xml --------
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
===================== 3 skipped, 1 error in 43.23 seconds ======================

not sure what's going on 😩

TomAugspurger · 2018-08-12T20:27:10Z

I think you need to import pandas.util._test_decorators as td at the top of test_multi.py

jreback · 2018-11-21T15:40:10Z

closing as stale, if you'd like to continue, pls ping.

benediamond · 2018-11-21T22:18:06Z

@jreback, my understanding was that this commit was ready, but had been waiting for your approval. I found it hard to get responses from you. if you're ready to go, i'm ready to play my part. it should be almost there.

jreback · 2018-11-21T22:25:09Z

i left multiple comments
you can update and rebase in master

pep8speaks · 2018-11-21T22:25:17Z

Hello @benediamond! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/arrays/categorical.py !
There are no PEP8 issues in the file pandas/tests/indexes/test_multi.py !

TomAugspurger · 2018-11-22T03:47:03Z

doc/source/whatsnew/v0.24.0.txt

@@ -132,7 +132,7 @@ Strings
 Indexing
 ^^^^^^^^

-
+- Bug in :func:`pandas.core.arrays.categorical._factorize_from_iterable` inappropriately caused ``TypeError`` to be raised when an ``Enum`` was used as a factor in a ``MultiIndex`` (:issue:`21298`)


Release notes shouldn't reference private methods.

You should be able to reference Enum with :class:enum.Enum, and :class:MultiIndex.

benediamond · 2018-11-22T05:15:44Z

@jreback and others: looks like this issue has since been fixed by PR #22072, see also issue #15457. I have gone ahead and preserved the test I wrote in my rebase, in case you'd like to merge it in---if not no worries and this (as well as issue #21298) can be closed! (note that issue #15457 was a somewhat different problem). thanks for everyone's time and patience.

WillAyd · 2018-11-26T03:51:55Z

@benediamond closing per your latest comment. Ping if we need to reopen

scratchmex · 2022-05-12T01:13:53Z

I want to point out that when merging dataframes with enums as multi-index it throws
RuntimeWarning: The values in the array are unorderable. Pass `sort=False` to suppress this warning.
so I think the support for enums is not yet completed

jreback · 2022-05-12T01:23:23Z

you could open a separate issue with a reproducible example

but we have no official support for enums and would need a champion in the community

benediamond added 4 commits June 6, 2018 19:22

Use of Enums in MultiIndex now supported

0ec3531

Due to an unnecessary "orderedness" check in `arrays/categorical.py`'s method `_factorize_from_iterable`, MultiIndex's constructor inappropriately threw a `TypeError` when passed an Enum as one of its iterables. This has been fixed.

added a test for MultiIndex with Enum

ca40575

Update test_multi.py

3857229

PEP8

9aee04a

gfyoung added Enhancement Multi Dimensional MultiIndex and removed Multi Dimensional labels Jun 7, 2018

jreback requested changes Jun 7, 2018

View reviewed changes

Put imports on top

60c8f2a

benediamond added 10 commits June 7, 2018 09:49

Update appveyor-27.yaml

13d67ca

Update circle-27-compat.yaml

e6e6627

Update travis-27-locale.yaml

d0607f4

Update travis-27.yaml

ec5900a

enum34 dependency

979b61e

Added `enum34` dependency

add whatsnew

1c84176

sync whatsnew with master

e914b20

sync whatsnew with master

5a55fee

attempting to resolve conflict

191fe58

attempting to resolve conflicts in whatsnew

bac47e2

gfyoung reviewed Jun 8, 2018

View reviewed changes

benediamond added 3 commits June 8, 2018 08:35

remove whatsnew

f76d522

remove whatsnew

f125bf7

add to whatsnew 0.24

761a9d1

gfyoung reviewed Jun 8, 2018

View reviewed changes

benediamond added 2 commits June 8, 2018 13:58

move imports to top

21014c6

gfyoung reviewed Jun 8, 2018

View reviewed changes

gfyoung approved these changes Jun 9, 2018

View reviewed changes

jreback requested changes Jun 19, 2018

View reviewed changes

jreback fixes

4cd2dff

import pandas.util._test_decorators as td

702594b

jreback closed this Nov 21, 2018

jreback reopened this Nov 21, 2018

TomAugspurger reviewed Nov 22, 2018

View reviewed changes

benediamond added 2 commits November 21, 2018 23:47

rebase

1950562

added test

6e4485f

typo: remove 'self'

06437cd

WillAyd closed this Nov 26, 2018

benediamond deleted the bd-21298 branch November 26, 2018 18:15

	cat = Categorical(values, ordered=True)
	categories = cat.categories
	codes = cat.codes

	raise TypeError("'values' is not ordered, please "
	"explicitly specify the categories order "
	"by passing in a categories argument.")

Support for use of Enums in MultiIndex #21348

Support for use of Enums in MultiIndex #21348

Conversation

benediamond commented Jun 7, 2018 • edited Loading

benediamond commented Jun 7, 2018 • edited Loading

gfyoung commented Jun 7, 2018 • edited Loading

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

codecov bot commented Jun 7, 2018 • edited Loading

Codecov Report

benediamond commented Jun 7, 2018

Choose a reason for hiding this comment

benediamond Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benediamond Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

gfyoung left a comment

Choose a reason for hiding this comment

benediamond commented Jun 12, 2018

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

benediamond commented Jun 19, 2018

benediamond commented Aug 12, 2018

TomAugspurger commented Aug 12, 2018

jreback commented Nov 21, 2018

benediamond commented Nov 21, 2018

jreback commented Nov 21, 2018

pep8speaks commented Nov 21, 2018

Choose a reason for hiding this comment

benediamond commented Nov 22, 2018 • edited Loading

WillAyd commented Nov 26, 2018

scratchmex commented May 12, 2022 • edited Loading

jreback commented May 12, 2022

benediamond commented Jun 7, 2018 •

edited

Loading

benediamond commented Jun 7, 2018 •

edited

Loading

gfyoung commented Jun 7, 2018 •

edited

Loading

codecov bot commented Jun 7, 2018 •

edited

Loading

benediamond Jun 8, 2018 •

edited

Loading

gfyoung Jun 8, 2018 •

edited

Loading

gfyoung Jun 8, 2018 •

edited

Loading

benediamond Jun 9, 2018 •

edited

Loading

benediamond commented Nov 22, 2018 •

edited

Loading

scratchmex commented May 12, 2022 •

edited

Loading