BUG: set_levels set illegal levels. #14236

bkandel · 2016-09-16T18:53:30Z

closes BUG: failing MultiIndex.set_levels with inplace=True still modifies original index #13754
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

MultiIndex.set_levels, when given illegal level values, raises an error.
When inplace=True, though, the illegal level values are still accepted. This
commit fixes that behavior by checking that the proposed level values are legal
before setting them.

codecov-io · 2016-09-16T19:26:49Z

Current coverage is 85.26% (diff: 100%)

Merging #14236 into master will increase coverage by <.01%

@@             master     #14236   diff @@
==========================================
  Files           140        140          
  Lines         50633      50634     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43170      43172     +2   
+ Misses         7463       7462     -1   
  Partials          0          0

Powered by Codecov. Last update f9ebeae...39b7bde

jreback · 2016-09-20T10:30:03Z

pandas/indexes/multi.py

@@ -116,12 +116,13 @@ def __new__(cls, levels=None, labels=None, sortorder=None, names=None,

        return result

-    def _verify_integrity(self):
+    def _verify_integrity(self, new_labels=None, new_levels=None):
        """Raises ValueError if length of levels and labels don't match or any


can you add a Parameters section

jreback · 2016-09-20T10:30:20Z

pandas/tests/indexes/test_multi.py

+        with assertRaisesRegexp(ValueError, "^On"):
+            self.index.set_levels(['c'], level=0, inplace=True)
+        assert_matching(self.index.levels, original_index.levels)
+


can you test set_labels (with invalid labels)

jreback · 2016-09-21T10:07:40Z

pandas/indexes/multi.py

        # NOTE: Currently does not check, among other things, that cached
        # nlevels matches nor that sortorder matches actually sortorder.
-        labels, levels = self.labels, self.levels
+        labels = new_labels or self.labels


these need a check for list-like, otherwise the len check will give an odd error message. use is_list_like

also add a test which exercises this check

I added a test for this, but didn't add a separate check within _verify_integrity because the check for list-like is already present in set_levels and set_labels: https://github.com/pydata/pandas/blob/master/pandas/indexes/multi.py#L221 and https://github.com/pydata/pandas/blob/master/pandas/indexes/multi.py#L322.

bkandel · 2016-09-22T14:39:28Z

@jreback the Travis OSX build keeps on failing on conda install. Could you maybe delete the cache and restart the build? Or any other ideas? I'm pretty sure the failure is not caused by these changes.

jreback · 2016-09-27T10:51:43Z

pandas/tests/indexes/test_multi.py

+
+        with assertRaisesRegexp(TypeError, "^Levels"):
+            self.index.set_levels('c', level=0, inplace=True)
+        assert_matching(self.index.levels, original_index.levels)


can you add tests for inplace=False as well (you can loop over the param I think)

jreback · 2016-09-27T10:52:02Z

minor comments, ping when green.

jreback · 2016-09-27T22:14:43Z

lgtm. ping on green.

jreback · 2016-09-27T23:41:13Z

pandas/tests/indexes/test_multi.py

@@ -154,7 +154,7 @@ def assert_matching(actual, expected):
            # as much as possible
            self.assertEqual(len(actual), len(expected))
            for act, exp in zip(actual, expected):
-                act = np.asarray(act)
+                act = np.asarray(act, dtype=np.object_)


why did you need to change this?

Because the expected is changed to np.object_ and if I don't change act also the test fails because the expected and actual are not changed.

can you show me an example? it is passing now, so unclear why you had to change it.

Just pushed up commit e0c8897 that undoes this change and fails with the error:

E AssertionError: numpy array are different E E Attribute "dtype" are different E [left]: int8 E [right]: object

int8 is right

But when I take out the coercion to np.object_, the test test_set_levels fails with

E AssertionError: numpy array are different E E Attribute "dtype" are different E [left]: object E [right]: |S4

This is in the test

# level changing [w/o mutation] ind2 = self.index.set_levels(new_levels) assert_matching(ind2.levels, new_levels) assert_matching(self.index.levels, levels)

It looks like maybe _ensure_index is doing the conversion to np.object_ in that test case (https://github.com/pydata/pandas/blob/master/pandas/indexes/base.py#L3616 and https://github.com/pydata/pandas/blob/master/pandas/lib.pyx#L1029).

Fixed this issue.

jreback · 2016-09-27T23:44:51Z

you need to rebase as just merged #14303

TomAugspurger · 2016-09-28T01:32:35Z

pandas/indexes/multi.py

        # NOTE: Currently does not check, among other things, that cached
        # nlevels matches nor that sortorder matches actually sortorder.
-        labels, levels = self.labels, self.levels
+        labels = new_labels or self.labels


Can new_levels or new_labels ever be a NumPy array (e.g. user passes idx.set_levels(np.array['a', 'b']), or will they have been coerced to a list by now? If they're an array this will raise a ValueError. You could use labels = new_labels if new_labels is not None else self.labels.

Ah, looks like it'll be a FrozenList by this point. Never mind then.

bkandel · 2016-09-30T20:58:53Z

@jreback I believe I have addressed all of the comments.

jreback · 2016-10-06T10:36:46Z

doc/source/whatsnew/v0.19.0.txt

 - Bug in ``df.groupby`` where ``.median()`` returns arbitrary values if grouped dataframe contains empty bins (:issue:`13629`)
 - Bug in ``Index.copy()`` where ``name`` parameter was ignored (:issue:`14302`)
+- Bug in ``MultiIndex.set_levels`` where illegal level values were still set after raising an error (:issue:`13754`)


move to 0.19.1

jreback · 2016-10-06T10:37:10Z

pandas/indexes/multi.py

+            Labels to check for validity. Defaults to current labels.
+        new_levels : optional list
+            Levels to check for validity. Defaults to current levels.
+        """
        # NOTE: Currently does not check, among other things, that cached


can you add a Raises section

jreback · 2016-10-06T10:38:15Z

pandas/indexes/multi.py

@@ -116,12 +116,22 @@ def __new__(cls, levels=None, labels=None, sortorder=None, names=None,

        return result

-    def _verify_integrity(self):
+    def _verify_integrity(self, new_labels=None, new_levels=None):


call these labels, levels. the new is confusing.

jreback · 2016-10-06T10:40:00Z

pandas/tests/indexes/test_multi.py

@@ -149,13 +149,16 @@ def test_set_levels(self):
        levels = self.index.levels
        new_levels = [[lev + 'a' for lev in level] for level in levels]

-        def assert_matching(actual, expected):
+        def assert_matching(actual, expected, coerce_to_obj=True):


call this check_dtype and simply pass onto assert_numpy_array_equal as the same parameter name.

bkandel · 2016-10-07T21:58:17Z

@jreback I think all comments are addressed now.

jreback · 2016-10-08T14:44:30Z

thanks @bkandel lgtm.

@jorisvandenbossche can be the first back-port test!

`MultiIndex.set_levels`, when passed in illegal level values, raises an error. When `inplace=True`, though, the illegal level values are still accepted. This commit fixes that behavior by checking that the proposed level values are legal before setting them. added tests and docs. lint added tests. force build force build force build force build for osx

jorisvandenbossche · 2016-10-10T12:30:37Z

@bkandel Thanks a lot!

* commit 'v0.19.0-14-ga40e185': (37 commits) BUG: Bug in localizing an ambiguous timezone when a boolean is passed Convert readthedocs links for their .org -> .io migration for hosted projects (pandas-dev#14406) DOC: formatting in basics.rst BLD/CI: cython cache pxd files (pandas-dev#14363) BUG: set_levels set illegal levels. (pandas-dev#14236) DOC: add whitespace to v0.19.1 bug fix section change impl details slightly for pandas-dev#14292 BUG: Fix concat key name DOC: add 0.19.1 whatsnew file (pandas-dev#14366) DOC: to_csv warns regarding quoting behaviour for floats pandas-dev#14195 (pandas-dev#14228) DOC: fix formatting issue with msgpack table TST: pandas-dev#14345 fixes TestDatetimeIndexOps test_nat AssertionErrors on 32-bit docs: Remove old warning from dsintro.rst (pandas-dev#14365) DOC: minor v0.19.0 whatsnew corrections RLS: v0.19.0 DOC: update release notes DOC: Latest fixes for whatsnew file to_latex encoding follows the documentation (py2 ascii, py3 utf8) (pandas-dev#14329) DOC: fix some sphinx build issues (pandas-dev#14332) TST: fix period tests for numpy 1.9.3 (GH14183) (pandas-dev#14331) ...

`MultiIndex.set_levels`, when passed in illegal level values, raises an error. When `inplace=True`, though, the illegal level values are still accepted. This commit fixes that behavior by checking that the proposed level values are legal before setting them.

bkandel force-pushed the fix_label_integrity branch 2 times, most recently from 89ba77f to fb16c65 Compare September 19, 2016 10:54

jreback changed the title ~~BUG: set_levels set illegal levels.~~ BUG: set_levels set illegal levels. Sep 19, 2016

jreback added Bug MultiIndex labels Sep 20, 2016

jreback requested changes Sep 20, 2016

View reviewed changes

jreback mentioned this pull request Sep 20, 2016

BUG: MultiIndex set_levels is not symmetrical with get_level_values #14258

Closed

jreback requested changes Sep 21, 2016

View reviewed changes

bkandel force-pushed the fix_label_integrity branch 2 times, most recently from 49be0b7 to 2cb6d8c Compare September 23, 2016 10:58

bkandel mentioned this pull request Sep 23, 2016

BUG: set_levels set illegal levels. #14290

Closed

4 tasks

jreback reviewed Sep 27, 2016

View reviewed changes

jreback added this to the 0.19.0 milestone Sep 27, 2016

jreback requested changes Sep 27, 2016

View reviewed changes

bkandel force-pushed the fix_label_integrity branch from d636fdf to 5e8af7c Compare September 27, 2016 23:58

TomAugspurger reviewed Sep 28, 2016

View reviewed changes

jreback modified the milestones: 0.19.0, 0.19.1 Sep 28, 2016

jreback reviewed Oct 6, 2016

View reviewed changes

bkandel force-pushed the fix_label_integrity branch from 1b0037a to b9b08fb Compare October 7, 2016 20:29

jreback added the Error Reporting Incorrect or improved errors from pandas label Oct 8, 2016

jreback approved these changes Oct 8, 2016

View reviewed changes

Ben Kandel added 9 commits October 9, 2016 13:40

force build

fbace39

force build

6e56ff3

add test for inplace=False

40bcdc7

lint

be8dfba

try not setting dtype

30bbeb4

put back in conversion to object

e1a6e0a

fix test coercion

005ab51

addressed comments

39b7bde

bkandel force-pushed the fix_label_integrity branch from b9b08fb to 39b7bde Compare October 9, 2016 17:40

jorisvandenbossche merged commit d98e982 into pandas-dev:master Oct 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: set_levels set illegal levels. #14236

BUG: set_levels set illegal levels. #14236

bkandel commented Sep 16, 2016

codecov-io commented Sep 16, 2016 •

edited

Loading

jreback Sep 20, 2016

jreback Sep 20, 2016

jreback Sep 21, 2016

jreback Sep 21, 2016

bkandel Sep 21, 2016

bkandel commented Sep 22, 2016

jreback Sep 27, 2016

jreback commented Sep 27, 2016

jreback commented Sep 27, 2016

jreback Sep 27, 2016

bkandel Sep 27, 2016

jreback Sep 27, 2016

bkandel Sep 28, 2016

jreback Sep 28, 2016

bkandel Sep 28, 2016

bkandel Sep 28, 2016

bkandel Sep 30, 2016

jreback commented Sep 27, 2016

TomAugspurger Sep 28, 2016

TomAugspurger Sep 28, 2016

bkandel commented Sep 30, 2016

jreback Oct 6, 2016

bkandel Oct 7, 2016

jreback Oct 6, 2016

bkandel Oct 7, 2016

jreback Oct 6, 2016

bkandel Oct 7, 2016

jreback Oct 6, 2016

bkandel Oct 7, 2016

bkandel commented Oct 7, 2016

jreback commented Oct 8, 2016

jorisvandenbossche commented Oct 10, 2016

BUG: set_levels set illegal levels. #14236

BUG: set_levels set illegal levels. #14236

Conversation

bkandel commented Sep 16, 2016

codecov-io commented Sep 16, 2016 • edited Loading

Current coverage is 85.26% (diff: 100%)

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

bkandel commented Sep 22, 2016

Choose a reason for hiding this comment

jreback commented Sep 27, 2016

jreback commented Sep 27, 2016

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 Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkandel commented Sep 30, 2016

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

bkandel commented Oct 7, 2016

jreback commented Oct 8, 2016

jorisvandenbossche commented Oct 10, 2016

codecov-io commented Sep 16, 2016 •

edited

Loading