BUG: .reset_index() should raise with an invalid level name (GH20925) #21016

KalyanGokhale · 2018-05-12T14:21:23Z

#20925 Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and an invalid level is supplied

closes Series.reset_index(level_name, drop=True) accepts invalid name when index is flat #20925
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2018-05-12T15:03:55Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #21016      +/-   ##
==========================================
+ Coverage   91.83%   91.83%   +<.01%     
==========================================
  Files         153      153              
  Lines       49497    49498       +1     
==========================================
+ Hits        45456    45458       +2     
+ Misses       4041     4040       -1

Flag	Coverage Δ
#multiple	`90.23% <100%> (ø)`	⬆️
#single	`41.88% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/series.py	`94.12% <100%> (+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 d623ffd...c9afed3. Read the comment docs.

TomAugspurger · 2018-05-12T15:16:54Z

Could you add a test that the correct error and error message is raised? Use tm.assert_raises_regex

pep8speaks · 2018-05-12T17:55:13Z

Hello @KalyanGokhale! Thanks for updating the PR.

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

Comment last updated on May 18, 2018 at 01:27 Hours UTC

KalyanGokhale · 2018-05-12T18:57:58Z

@TomAugspurger have added the test - please tell if any further edits are needed.

toobaz

My test case was wrong... removing the set() will maybe fix the correct one?

toobaz · 2018-05-13T06:02:22Z

pandas/tests/series/indexing/test_indexing.py

+    with tm.assert_raises_regex(KeyError, 'not found'):
+        s.reset_index('wrong', drop=True)
+    # Data for Test Case 4
+    s = pd.Series(range(4), name='valid')


This (and I mean: my testcase) doesn't make much sense, as we are giving a name to s itself, not to its index. I should have written

s = pd.Series(range(4), index=pd.RangeIndex(4, name='valid'))

Notice that if you replace with the above, the following doesn't raise any more. But it might be a bug in MultiIndex.droplevel, in which case OK to postpone.

Will delete this particular test case now

toobaz · 2018-05-13T06:03:03Z

pandas/core/series.py

                if not isinstance(level, (tuple, list)):
                    level = [level]
+                level = set(level)


level = set(level) is not required now. Did not remove it after combining two separate blocks post-testing. Will delete it.

toobaz · 2018-05-13T06:24:17Z

pandas/core/series.py

-                if len(level) < len(self.index.levels):
-                    new_index = self.index.droplevel(level)
+                if isinstance(self.index, MultiIndex):
+                    if (len(level) < len(self.index.levels)):


len(self.index.levels) can be replaced with self.index.nlevels (and then I guess you'll be able to drop the MultiIndex test above). But is this test actually needed? (It's not a rhetorical question)

Thanks for the suggestion.

Yes replacing len(self.index.levels) with self.index.nlevels helps and MultiIndex test can then be removed. However, have retained the MultiIndex test to avoid any unforeseen outcomes - retaining it also produces the same outcomes (at least in the test cases checked).

len(level) < self.index.nlevels test is needed, in case this is dropped (assuming MultiIndex test is also dropped) then following errors are observed for these cases:
Test Data
arrays = [np.array(['bar', 'bar', 'baz', 'baz']), np.array(['one', 'two', 'one', 'two'])]
s = pd.Series(range(4), name='foo', index=pd.MultiIndex.from_arrays(arrays, names=['a', 'b']))
Test Case
s.reset_index(['a', 'b'], drop=True)
Outcome
ValueError: Must pass non-zero number of levels/labels

Test Data
s = pd.Series(range(4), index=pd.RangeIndex(4, name='valid'))
Test Cases
s.reset_index(['valid', 'valid'], drop=True)
s.reset_index(['valid'], drop=True)
s.reset_index('valid', drop=True)
Outcome for each of 3 test cases above
AttributeError: 'RangeIndex' object has no attribute 'droplevel'

len(level) < self.index.nlevels test is needed

Right, hadn't considered that zero-levels MultiIndexes can't exist.

The isinstance(self.index, MultiIndex) doesn't make much sense but it's OK to leave it, as it guarantees that if pd.Series(range(4)).reset_index([], drop=True) doesn't fail (pd.Index.droplevel() doesn't exist). Could you add a test case for this?

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and an invalid level is supplied

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and invalid level is supplied. Made edits as requested in the review.

jreback · 2018-05-13T13:30:56Z

pandas/tests/series/indexing/test_indexing.py

+    # https://github.com/pandas-dev/pandas/issues/20925
+    # Data for Test Case 1 and 2
+    s = pd.Series(range(4))
+    # Test Case 1


put blank lines between cases. Don't use a 'Test Case 1' as a lable, either remove of put what it is testing

Done. Have introduced blank cases and, replaced labels with comments of what is being tested

Made edits to test file for labels and introduced blank spaces between test cases.

toobaz · 2018-05-16T15:57:43Z

pandas/tests/series/indexing/test_indexing.py

+def test_reset_index_drop_errmsg():
+    # https://github.com/pandas-dev/pandas/issues/20925
+
+    # Check KeyError raised for series where no 'level' name is defined


series -> series index
no 'level' name is defined -> passed level name is missing

toobaz · 2018-05-16T15:57:58Z

pandas/tests/series/indexing/test_indexing.py

+    with tm.assert_raises_regex(KeyError, 'must be same as name'):
+        s.reset_index('wrong')
+
+    # Check KeyError raised for series where 'level' to be dropped is undefined


undefined -> missing

toobaz · 2018-05-16T15:59:39Z

I mistakenly wrote in an obsolete discussion, copying here:

len(level) < self.index.nlevels test is needed

Right, hadn't considered that zero-levels MultiIndexes can't exist.

The isinstance(self.index, MultiIndex) doesn't make much sense but it's OK to leave it, as it guarantees that pd.Series(range(4)).reset_index([], drop=True) doesn't fail (pd.Index.droplevel() doesn't exist). Could you add a test case for this?

After that and a couple of minor fixes to comments, I think we're ready.

made suggested edits to tests

toobaz

Changes OK, just move to correct file

toobaz · 2018-05-17T04:06:24Z

pandas/tests/series/indexing/test_indexing.py

@@ -768,3 +768,24 @@ def test_head_tail(test_data):
    assert_series_equal(test_data.series.head(0), test_data.series[0:0])
    assert_series_equal(test_data.series.tail(), test_data.series[-5:])
    assert_series_equal(test_data.series.tail(0), test_data.series[0:0])
+
+
+def test_reset_index_drop_errmsg():


I just noticed this is in the wrong file... reset_index is tested in pandas/tests/series/test_alter_axes.py, not test_indexing.py.

While you're at it, please replace # https://github.com/pandas-dev/pandas/issues/20925 with the standard # GH 20925.

Thanks didn't know that the right file was test_alter_axes.py

Have moved the function there (now renamed as test_reset_index_drop_errors based on @TomAugspurger 's comment) - also updated comment for # GH 20925

toobaz · 2018-05-17T04:11:10Z

pandas/tests/series/indexing/test_indexing.py

+    with tm.assert_raises_regex(KeyError, 'not found'):
+        s.reset_index('wrong', drop=True)
+
+    # Check that .reset_index([],drop=True) doesn't fail


This is fine but probably not in the right test: could you append it to test_reset_index_level (in pandas/tests/series/test_alter_axes.py)?

Done. Have moved it to function test_reset_index_level outside the for loop

TomAugspurger

Release note can go in 0.23.1 (maybe have to merge in master for it to show up).

TomAugspurger · 2018-05-17T13:09:23Z

pandas/tests/series/indexing/test_indexing.py

@@ -768,3 +768,24 @@ def test_head_tail(test_data):
    assert_series_equal(test_data.series.head(0), test_data.series[0:0])
    assert_series_equal(test_data.series.tail(), test_data.series[-5:])
    assert_series_equal(test_data.series.tail(0), test_data.series[0:0])
+
+
+def test_reset_index_drop_errmsg():


errmsg -> errors or error_message

done. renamed def as test_reset_index_drop_errors

Made requested changes, moved test to pandas/tests/series/test_alter_axes.py

toobaz · 2018-05-17T15:06:39Z

Looks all good to me, ready to merge after the whatsnew note is added.

Updating to 0.23.0

KalyanGokhale · 2018-05-17T18:14:41Z

Updated v0.23.1.txt under Bug Fixes > Indexing as:
Bug in :meth:Series.reset_index where appropriate error was not raised with a non-named level (:issue:20925)

toobaz · 2018-05-17T22:34:20Z

doc/source/whatsnew/v0.23.1.txt

@@ -59,7 +59,7 @@ Conversion
 Indexing
 ^^^^^^^^

-
+- Bug in :meth:`Series.reset_index` where appropriate error was not raised with a non-named level (:issue:`20925`)


I don't get the "a non-named level". Wouldn't it be "an invalid level name"?

Agreed. Done.

toobaz · 2018-05-18T05:54:12Z

@KalyanGokhale merged, thanks!

…das-dev#21016) closes pandas-dev#20925 (cherry picked from commit e033c06)

) closes #20925 (cherry picked from commit e033c06)

…das-dev#21016) closes pandas-dev#20925

toobaz suggested changes May 13, 2018

View reviewed changes

KalyanGokhale added 5 commits May 13, 2018 18:03

GH20925

ef542b1

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and an invalid level is supplied

GH20925

837d4da

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and an invalid level is supplied

GH20925

a10f6ac

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and an invalid level is supplied

GH20925

30bc393

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and an invalid level is supplied

GH20925

cfd70af

Raises appropriate error for Series.reset_index(level_name, drop=True) when index is flat and invalid level is supplied. Made edits as requested in the review.

jreback changed the title ~~GH20925~~ BUG: .reset_index() should raise with a non-named level May 13, 2018

jreback requested changes May 13, 2018

View reviewed changes

jreback added Bug Indexing Related to indexing on series/frames, not to indexes themselves MultiIndex labels May 13, 2018

KalyanGokhale added 4 commits May 13, 2018 19:45

GH20925

6b87a3a

Made edits to test file for labels and introduced blank spaces between test cases.

GH20925

50c553e

Made edits to test file for labels and introduced blank spaces between test cases.

GH20925

1ecf7c3

Made edits to test file for labels and introduced blank spaces between test cases.

GH20925

2945890

Made edits to test file for labels and introduced blank spaces between test cases.

toobaz reviewed May 16, 2018

View reviewed changes

toobaz mentioned this pull request May 16, 2018

Repeated level indexes/names make reset_index() and others misbehave #21091

Open

KalyanGokhale added 2 commits May 17, 2018 07:34

GH20925

284d016

made suggested edits to tests

GH20925

a413bc9

made suggested edits to tests

toobaz suggested changes May 17, 2018

View reviewed changes

TomAugspurger reviewed May 17, 2018

View reviewed changes

TomAugspurger added this to the 0.23.1 milestone May 17, 2018

KalyanGokhale added 2 commits May 17, 2018 18:55

GH20925

f08ea70

Made requested changes, moved test to pandas/tests/series/test_alter_axes.py

GH20925

03226f7

Made requested changes, moved test to pandas/tests/series/test_alter_axes.py

KalyanGokhale added 3 commits May 17, 2018 22:55

Merge pull request #1 from pandas-dev/master

d0c7ebc

Updating to 0.23.0

Merge pull request #2 from KalyanGokhale/master

1df935f

Updating to 0.23.0

Update v0.23.1.txt

71d23a1

toobaz reviewed May 17, 2018

View reviewed changes

Update v0.23.1.txt

c9afed3

KalyanGokhale changed the title ~~BUG: .reset_index() should raise with a non-named level~~ BUG: .reset_index() should raise with an invalid level name May 18, 2018

KalyanGokhale changed the title ~~BUG: .reset_index() should raise with an invalid level name~~ BUG: .reset_index() should raise with an invalid level name (GH20925) May 18, 2018

toobaz approved these changes May 18, 2018

View reviewed changes

toobaz merged commit e033c06 into pandas-dev:master May 18, 2018

toobaz mentioned this pull request May 18, 2018

API: implement droplevels() for flat index #21115

Closed

KalyanGokhale deleted the reset-index-errmsg branch May 19, 2018 05:53

jreback added the Needs Backport label May 19, 2018

jorisvandenbossche removed the Needs Backport label Jun 8, 2018

jorisvandenbossche pushed a commit to jorisvandenbossche/pandas that referenced this pull request Jun 8, 2018

BUG: make .reset_index() raise when passed an invalid level name (pan…

c0da732

…das-dev#21016) closes pandas-dev#20925 (cherry picked from commit e033c06)

jorisvandenbossche pushed a commit that referenced this pull request Jun 9, 2018

BUG: make .reset_index() raise when passed an invalid level name (#21016

14ad199

) closes #20925 (cherry picked from commit e033c06)

david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018

BUG: make .reset_index() raise when passed an invalid level name (pan…

14887b2

…das-dev#21016) closes pandas-dev#20925

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: .reset_index() should raise with an invalid level name (GH20925) #21016

BUG: .reset_index() should raise with an invalid level name (GH20925) #21016

KalyanGokhale commented May 12, 2018 •

edited

Loading

codecov bot commented May 12, 2018 •

edited

Loading

TomAugspurger commented May 12, 2018 •

edited

Loading

pep8speaks commented May 12, 2018 •

edited

Loading

KalyanGokhale commented May 12, 2018 •

edited

Loading

toobaz left a comment

toobaz May 13, 2018

KalyanGokhale May 13, 2018 •

edited

Loading

toobaz May 13, 2018

KalyanGokhale May 13, 2018

toobaz May 13, 2018

KalyanGokhale May 13, 2018 •

edited

Loading

toobaz May 16, 2018

KalyanGokhale May 17, 2018

jreback May 13, 2018

KalyanGokhale May 13, 2018

toobaz May 16, 2018

KalyanGokhale May 17, 2018

toobaz May 16, 2018

KalyanGokhale May 17, 2018

toobaz commented May 16, 2018 •

edited

Loading

toobaz left a comment

toobaz May 17, 2018

KalyanGokhale May 17, 2018 •

edited

Loading

toobaz May 17, 2018

KalyanGokhale May 17, 2018 •

edited

Loading

TomAugspurger left a comment

TomAugspurger May 17, 2018

KalyanGokhale May 17, 2018 •

edited

Loading

toobaz commented May 17, 2018

KalyanGokhale commented May 17, 2018

toobaz May 17, 2018

KalyanGokhale May 18, 2018

toobaz commented May 18, 2018

BUG: .reset_index() should raise with an invalid level name (GH20925) #21016

BUG: .reset_index() should raise with an invalid level name (GH20925) #21016

Conversation

KalyanGokhale commented May 12, 2018 • edited Loading

codecov bot commented May 12, 2018 • edited Loading

Codecov Report

TomAugspurger commented May 12, 2018 • edited Loading

pep8speaks commented May 12, 2018 • edited Loading

Comment last updated on May 18, 2018 at 01:27 Hours UTC

KalyanGokhale commented May 12, 2018 • edited Loading

toobaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalyanGokhale May 13, 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

KalyanGokhale May 13, 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

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

toobaz commented May 16, 2018 • edited Loading

toobaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalyanGokhale May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalyanGokhale May 17, 2018 • edited Loading

Choose a reason for hiding this comment

TomAugspurger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalyanGokhale May 17, 2018 • edited Loading

Choose a reason for hiding this comment

toobaz commented May 17, 2018

KalyanGokhale commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz commented May 18, 2018

KalyanGokhale commented May 12, 2018 •

edited

Loading

codecov bot commented May 12, 2018 •

edited

Loading

TomAugspurger commented May 12, 2018 •

edited

Loading

pep8speaks commented May 12, 2018 •

edited

Loading

KalyanGokhale commented May 12, 2018 •

edited

Loading

KalyanGokhale May 13, 2018 •

edited

Loading

KalyanGokhale May 13, 2018 •

edited

Loading

toobaz commented May 16, 2018 •

edited

Loading

KalyanGokhale May 17, 2018 •

edited

Loading

KalyanGokhale May 17, 2018 •

edited

Loading

KalyanGokhale May 17, 2018 •

edited

Loading