BUG: GH17464 MultiIndex now raises an error when levels aren't unique, tests changed #17971

cmazzullo · 2017-10-25T02:25:18Z

closes ERR: invalid MultiIndex construction #17464
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2017-10-25T03:22:53Z

Codecov Report

Merging #17971 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #17971      +/-   ##
==========================================
- Coverage   91.23%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       50113    50115       +2     
==========================================
- Hits        45723    45715       -8     
- Misses       4390     4400      +10

Flag	Coverage Δ
#multiple	`89.03% <50%> (-0.01%)`	⬇️
#single	`40.31% <50%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`96.32% <50%> (-0.08%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <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 e1dabf3...f957e51. Read the comment docs.

codecov · 2017-10-25T03:23:00Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17971      +/-   ##
==========================================
+ Coverage   91.44%   91.45%   +<.01%     
==========================================
  Files         157      157              
  Lines       51378    51441      +63     
==========================================
+ Hits        46985    47044      +59     
- Misses       4393     4397       +4

Flag	Coverage Δ
#multiple	`89.32% <100%> (+0.02%)`	⬆️
#single	`40.6% <50%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`96.28% <100%> (-0.16%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/indexes/interval.py	`93.05% <0%> (-0.17%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/core/indexes/base.py	`96.45% <0%> (-0.03%)`	⬇️
pandas/core/indexes/datetimes.py	`95.69% <0%> (-0.03%)`	⬇️
pandas/core/groupby.py	`92.04% <0%> (ø)`	⬆️
pandas/core/internals.py	`94.47% <0%> (ø)`	⬆️
pandas/core/series.py	`94.8% <0%> (+0.04%)`	⬆️
... and 5 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 1eedcf6...703ff1e. Read the comment docs.

jschendel · 2017-10-25T15:05:33Z

doc/source/whatsnew/v0.21.0.txt

- Bug in :func:`Series.rename` when called with a callable, incorrectly alters the name of the ``Series``, rather than the name of the ``Index``. (:issue:`17407`)
- Bug in :func:`String.str_get` raises ``IndexError`` instead of inserting NaNs when using a negative index. (:issue:`17704`)
+- Bug in :func:`Series.rename` when called with a `callable`, incorrectly alters the name of the `Series`, rather than the name of the `Index`. (:issue:`17407`)
+- Bug in :func:`String.str_get` raises `index out of range` error instead of inserting NaNs when using a negative index. (:issue:`17704`)


Is there a reason you changed these two entries? The previous versions look correct to me (double backticks instead of single, "callable" unticked, etc.).

jschendel · 2017-10-25T15:06:22Z

doc/source/whatsnew/v0.21.0.txt

- Bug in :func:`String.str_get` raises ``IndexError`` instead of inserting NaNs when using a negative index. (:issue:`17704`)
+- Bug in :func:`Series.rename` when called with a `callable`, incorrectly alters the name of the `Series`, rather than the name of the `Index`. (:issue:`17407`)
+- Bug in :func:`String.str_get` raises `index out of range` error instead of inserting NaNs when using a negative index. (:issue:`17704`)
+- When created with duplicate labels, ``MultiIndex`` now raises a `ValueError`. (:issue:`17464`)


ValueError should have two backticks: ``ValueError``

jreback · 2017-10-25T10:28:11Z

pandas/tests/indexes/test_multi.py

@@ -1573,7 +1573,8 @@ def test_is_(self):
        # shouldn't change
        assert mi2.is_(mi)
        mi4 = mi3.view()


can you add an explicit test for the issue, e.g that you are raising (use the example from the original issue).

jreback · 2017-10-25T10:28:25Z

pandas/tests/indexes/test_multi.py

@@ -1573,7 +1573,8 @@ def test_is_(self):
        # shouldn't change
        assert mi2.is_(mi)
        mi4 = mi3.view()
-        mi4.set_levels([[1 for _ in range(10)], lrange(10)], inplace=True)
+        # GH 17464 - Remove duplicate MultiIndex levels


you don't need these comments here, rather they go on a new test (see above)

jreback · 2017-10-25T10:29:20Z

pandas/tests/indexes/test_multi.py

-        ind.set_levels([['A', 'B', 'A', 'A', 'B'], [2, 1, 3, -2, 5]],
-                       inplace=True)
-
+        # GH 17464 - Remove duplicate MultiIndex levels


ok changing this, but remove the comment, and add the original as another example of where things should raise (in the test you are adding above)

jreback · 2017-10-27T10:23:58Z

doc/source/whatsnew/v0.21.0.txt

@@ -978,6 +978,7 @@ Indexing
 - Bug in :meth:`DataFrame.first_valid_index` and :meth:`DataFrame.last_valid_index` when no valid entry (:issue:`17400`)
 - Bug in :func:`Series.rename` when called with a callable, incorrectly alters the name of the ``Series``, rather than the name of the ``Index``. (:issue:`17407`)
 - Bug in :func:`String.str_get` raises ``IndexError`` instead of inserting NaNs when using a negative index. (:issue:`17704`)
+- When created with duplicate labels, ``MultiIndex`` now raises a ``ValueError``. (:issue:`17464`)


move to 0.22.0 breaking changes

cmazzullo · 2017-10-27T12:44:08Z

Sure thing

jreback · 2017-10-28T00:05:27Z

looks like you need to rebase against master, seems you have pulled in other commits

git pull --rebase origin/master -i

then force push

cmazzullo · 2017-10-28T20:50:13Z

My mistake, latest push should clear that up

jreback · 2017-10-28T21:03:48Z

I rebased you. I still need to look at this for review.

jreback · 2017-10-29T23:12:01Z

can you run the asv for multi and see if any perf changes with this? (prob no, but just checking)

cmazzullo · 2017-11-02T01:34:52Z

asv shows nasty perf changes though I think it may be a problem with my setup. I'll see if I can work it out. Here's the output for good measure.

       before           after         ratio
     [4af9a8b6]       [cdea7fec]
+     16.9±0.03ms      40.2±0.02ms     2.38  categoricals.Categoricals.time_constructor_all_nan
+      23.7±0.2ms         27.4±2ms     1.16  frame_methods.Dropna.time_dropna_axis0_all
+     9.42±0.05μs       10.9±0.2μs     1.15  indexing.Int64Indexing.time_iloc_scalar
+      38.3±0.1ms       43.5±0.8ms     1.14  stat_ops.stats_rolling_mean.time_rolling_median
+         724±5ns          821±8ns     1.13  period.Properties.time_year
+      43.0±0.2ms       48.8±0.2ms     1.13  timedelta.ToTimedelta.time_convert_coerce
+       427±0.4μs          474±3μs     1.11  groupby.GroupBySuite.time_std('int', 100)
+        1.61±0ms      1.77±0.02ms     1.10  strings.StringMethods.time_contains_many_noregex
-         856±8ns          747±4ns     0.87  period.Properties.time_is_leap_year
-        45.9±1ms       38.3±0.2ms     0.83  gil.nogil_take1d_int64.time_nogil_take1d_int64

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

jreback · 2017-11-25T16:21:37Z

can you rebase

cmazzullo · 2017-11-29T01:44:06Z

Rebased

cmazzullo · 2017-11-29T04:07:04Z

Looks like some tests from test_groupby were duplicated in test_functional, without the modifications. I updated the duplicated function in test_functional to get rid of the error - let me know if you think it would be better to remove all the duped tests instead.

… levels

jreback · 2017-12-01T15:38:29Z

doc/source/whatsnew/v0.22.0.txt

@@ -81,6 +81,9 @@ Other API Changes
 - Inserting missing values into indexes will work for all types of indexes and automatically insert the correct type of missing value (``NaN``, ``NaT``, etc.) regardless of the type passed in (:issue:`18295`)
 - Restricted ``DateOffset`` keyword arguments. Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior. Now, only valid arguments will be accepted. (:issue:`17176`, :issue:`18226`).
 - :func:`DataFrame.from_items` provides a more informative error message when passed scalar values (:issue:`17312`)
+- :class:`Timestamp` will no longer silently ignore unused or invalid `tz` or `tzinfo` arguments (:issue:`17690`)


looks like some duplication here

jreback · 2017-12-01T15:39:00Z

pandas/core/indexes/multi.py

@@ -198,6 +198,10 @@ def _verify_integrity(self, labels=None, levels=None):
                                 " level  (%d). NOTE: this index is in an"
                                 " inconsistent state" % (i, label.max(),
                                                          len(level)))
+            if not level.is_unique:
+                raise ValueError("Level values must be unique: {0}"


can you use kwargs rather than positional here

jreback · 2017-12-01T15:42:37Z

pandas/tests/groupby/test_functional.py

@@ -67,8 +67,10 @@ def test_frame_describe_multikey(self):
                                         'C': 1, 'D': 1}, axis=1)
        result = groupedT.describe()
        expected = self.tsframe.describe().T
-        expected.index = pd.MultiIndex([[0, 0, 1, 1], expected.index],
-                                       [range(4), range(len(expected.index))])
+        # GH 17464 - Remove duplicate MultiIndex levels


you don't need this comment here

jreback · 2017-12-01T15:43:47Z

pandas/tests/groupby/test_groupby.py

@@ -385,6 +385,83 @@ def test_attr_wrapper(self):
        # make sure raises error
        pytest.raises(AttributeError, getattr, grouped, 'foo')

+    def test_series_describe_multikey(self):


these tests were moved, revert this file

jreback · 2017-12-01T15:44:13Z

pandas/tests/indexes/test_multi.py

+        # Make sure that a MultiIndex with duplicate levels throws a ValueError
+        with pytest.raises(ValueError):
+            ind = pd.MultiIndex([['A'] * 10, range(10)], [[0] * 10, range(10)])
+        # And that using set_levels with duplicate levels fails


cmazzullo · 2017-12-03T14:51:30Z

@jreback Okay, addressed those issues

jreback · 2017-12-03T16:36:36Z

thanks @cmazzullo

cmazzullo · 2017-12-04T03:27:38Z

No problem

jreback added Bug MultiIndex labels Oct 25, 2017

jschendel reviewed Oct 25, 2017

View reviewed changes

jreback requested changes Oct 27, 2017

View reviewed changes

jreback force-pushed the gh-17464 branch from 9522071 to cdea7fe Compare October 28, 2017 21:03

jreback added this to the 0.22.0 milestone Oct 29, 2017

jreback approved these changes Oct 29, 2017

View reviewed changes

jreback mentioned this pull request Oct 29, 2017

Raise ValueError when using levels with non-unique values in MultiIndex constructor #17557

Closed

4 tasks

cmazzullo added 10 commits November 30, 2017 21:22

BUG: GH17464 Add error checking for duplicate levels in MultiIndex

a9a5177

Remove duplicate levels from test_is_

91c8388

Remove duplicate levels from test_level_setting_resets_attributes

392ce8a

Remove duplicate levels from test_frame_describe_multikey

3c0812e

Add comments about change

0983453

Added comments changes w/ bug number

b39421f

PEP8 compliance

5771426

whatsnew entry

48f9429

Remove duplicate levels from test_is_

9ffc8ad

Remove duplicate levels from test_level_setting_resets_attributes

e70a2be

cmazzullo and others added 16 commits November 30, 2017 21:31

Added comments changes w/ bug number

8b400dc

PEP8 compliance

0a8e9f2

whatsnew entry

cc7ebc7

Remove duplicate levels from test_is_

b02114f

Remove duplicate levels from test_level_setting_resets_attributes

b56eca0

Remove duplicate levels from test_frame_describe_multikey

9f179e6

Add comments about change

2af9aba

Added comments changes w/ bug number

3e56aba

PEP8 compliance

85d6379

whatsnew entry

49b731d

Whatsnew backticks

ec4f971

whatsnew merging

2684855

Removed comments about this issue from other tests

c36c236

Added test to make sure a ValueError is thrown

2b3f4d4

move whatsnew to 0.22.0

386daaf

Updated test_frame_describe_multikey to remove duplicate MultiIndex…

c169645

… levels

cmazzullo force-pushed the gh-17464 branch from a2a6697 to c169645 Compare December 1, 2017 02:34

Fixed linting issue

073e629

jreback requested changes Dec 1, 2017

View reviewed changes

cmazzullo added 5 commits December 2, 2017 17:04

whatsnew changes

869157d

Kwargs in error message

44e4552

Removed unnecessary comment

297216b

Added blank line

fead79f

Got rid of duplicated tests

703ff1e

jreback approved these changes Dec 3, 2017

View reviewed changes

jreback merged commit 8172565 into pandas-dev:master Dec 3, 2017

cmazzullo deleted the gh-17464 branch December 4, 2017 03:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: GH17464 MultiIndex now raises an error when levels aren't unique, tests changed #17971

BUG: GH17464 MultiIndex now raises an error when levels aren't unique, tests changed #17971

cmazzullo commented Oct 25, 2017

codecov bot commented Oct 25, 2017

codecov bot commented Oct 25, 2017 •

edited

Loading

jschendel Oct 25, 2017

jschendel Oct 25, 2017

jreback Oct 25, 2017

jreback Oct 25, 2017

jreback Oct 25, 2017

jreback Oct 27, 2017

cmazzullo commented Oct 27, 2017

jreback commented Oct 28, 2017

cmazzullo commented Oct 28, 2017

jreback commented Oct 28, 2017 •

edited

Loading

jreback commented Oct 29, 2017

cmazzullo commented Nov 2, 2017

jreback commented Nov 25, 2017

cmazzullo commented Nov 29, 2017

cmazzullo commented Nov 29, 2017 •

edited

Loading

jreback Dec 1, 2017

jreback Dec 1, 2017

jreback Dec 1, 2017

jreback Dec 1, 2017

jreback Dec 1, 2017

cmazzullo commented Dec 3, 2017

jreback commented Dec 3, 2017

cmazzullo commented Dec 4, 2017

BUG: GH17464 MultiIndex now raises an error when levels aren't unique, tests changed #17971

BUG: GH17464 MultiIndex now raises an error when levels aren't unique, tests changed #17971

Conversation

cmazzullo commented Oct 25, 2017

codecov bot commented Oct 25, 2017

Codecov Report

codecov bot commented Oct 25, 2017 • edited Loading

Codecov Report

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

cmazzullo commented Oct 27, 2017

jreback commented Oct 28, 2017

cmazzullo commented Oct 28, 2017

jreback commented Oct 28, 2017 • edited Loading

jreback commented Oct 29, 2017

cmazzullo commented Nov 2, 2017

jreback commented Nov 25, 2017

cmazzullo commented Nov 29, 2017

cmazzullo commented Nov 29, 2017 • 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

cmazzullo commented Dec 3, 2017

jreback commented Dec 3, 2017

cmazzullo commented Dec 4, 2017

codecov bot commented Oct 25, 2017 •

edited

Loading

jreback commented Oct 28, 2017 •

edited

Loading

cmazzullo commented Nov 29, 2017 •

edited

Loading