Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idx droplevel #21116

Merged
merged 5 commits into from
May 21, 2018
Merged

Idx droplevel #21116

merged 5 commits into from
May 21, 2018

Conversation

toobaz
Copy link
Member

@toobaz toobaz commented May 18, 2018

Collateral change: if mi is a MultiIndex, mi.droplevel([]) will now return mi itself, not a copy of it. Given that these are immutable objects, seems OK to me - but if a copy is preferred, the change is trivial.

@codecov
Copy link

codecov bot commented May 18, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21116      +/-   ##
==========================================
+ Coverage   91.83%   91.84%   +<.01%     
==========================================
  Files         153      153              
  Lines       49498    49505       +7     
==========================================
+ Hits        45458    45466       +8     
+ Misses       4040     4039       -1
Flag Coverage Δ
#multiple 90.23% <100%> (ø) ⬆️
#single 41.88% <76.66%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/multi.py 94.99% <ø> (-0.09%) ⬇️
pandas/core/series.py 94.12% <100%> (ø) ⬆️
pandas/core/frame.py 97.22% <100%> (-0.01%) ⬇️
pandas/core/indexes/base.py 96.68% <100%> (+0.04%) ⬆️
pandas/core/reshape/reshape.py 99.78% <0%> (-0.22%) ⬇️
pandas/core/accessor.py 98.71% <0%> (ø) ⬆️
pandas/core/common.py 92.09% <0%> (+0.08%) ⬆️
pandas/util/testing.py 84.81% <0%> (+0.21%) ⬆️

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 e033c06...57a5332. Read the comment docs.

@@ -245,6 +245,20 @@ def test_constructor_int_dtype_nan(self):
result = Index(data, dtype='float')
tm.assert_index_equal(result, expected)

def test_droplevel(self):
# GH 21115
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can u test for all types here; see other test for how to iterate

If a string is given, must be the name of a level
If list-like, elements must be names or indexes of levels.

Returns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can u add a versionadded tag

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(done)

@@ -1199,9 +1199,8 @@ def reset_index(self, level=None, drop=False, name=None, inplace=False):
if not isinstance(level, (tuple, list)):
level = [level]
level = [self.index._get_level_number(lev) for lev in level]
if isinstance(self.index, MultiIndex):
if len(level) < self.index.nlevels:
new_index = self.index.droplevel(level)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there more places u can simplify? maybe in core/indexing.py ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found only another one. The other uses of droplevel do need to treat differently flat indexes.

@jschendel
Copy link
Member

Should this be added to api.rst under Index?

@toobaz
Copy link
Member Author

toobaz commented May 19, 2018

Should this be added to api.rst under Index?

Not sure, we should find a general decision in #3268 (my understanding is that subdivisions in api.rst are thematic, and droplevel certainly has to do with MultiIndex)

@toobaz
Copy link
Member Author

toobaz commented May 19, 2018

Ready to merge if there are no objections

@jreback
Copy link
Contributor

jreback commented May 19, 2018

will look in a bit

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add to api.rst as well (as the one for MI is already there), merge when good.

@jreback jreback added this to the 0.23.1 milestone May 19, 2018
@toobaz
Copy link
Member Author

toobaz commented May 19, 2018

can you add to api.rst as well (as the one for MI is already there), merge when good.

MultiIndex is an Index, so if I add it in the Index section I will remove it from the MultiIndex section. And I argue this would not help users. We can probably reorganize api.rst better when we work on #3268, but only moving droplevel (and not, say, swaplevel) is not good.

@jreback
Copy link
Contributor

jreback commented May 21, 2018

MultiIndex is an Index, so if I add it in the Index section I will remove it from the MultiIndex

just add it to both

@toobaz
Copy link
Member Author

toobaz commented May 21, 2018

just add it to both

There is no "both" as there is no specific Index section: I created a new section but it's probably going to be removed/changed when #3268 evolves.

@@ -65,6 +65,7 @@ Indexing
^^^^^^^^

- Bug in :meth:`Series.reset_index` where appropriate error was not raised with an invalid level name (:issue:`20925`)
- :meth:`Index.droplevel` is now implemented also for flat indexes, for compatibility with MultiIndex (:issue:`21115`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should move to enhancements (later PR is fine).

@jreback jreback merged commit 172ab7a into pandas-dev:master May 21, 2018
@jreback
Copy link
Contributor

jreback commented May 21, 2018

thanks!

@jorisvandenbossche
Copy link
Member

@toobaz Let's keep this for 0.24.0 ? (it's a new method/feature)
Or does any of the other bug fixes depend on it?

@jorisvandenbossche jorisvandenbossche modified the milestones: 0.23.1, 0.24.0 Jun 7, 2018
@toobaz
Copy link
Member Author

toobaz commented Jun 7, 2018

@toobaz Let's keep this for 0.24.0 ? (it's a new method/feature)

I don't see any harm (but it's not entirely obvious what this means to me: just move to the other whatsnew?)

@jorisvandenbossche
Copy link
Member

Yes, in practice that is what it means.
(I was planning to do a PR to clean up the whatsnew file for and just saw this was the only new feature)

@toobaz
Copy link
Member Author

toobaz commented Jun 7, 2018

(I was planning to do a PR to clean up the whatsnew file for and just saw this was the only new feature)

OK! So you're taking care of this in your PR? Or should I open another one?

@jorisvandenbossche
Copy link
Member

Yep, will do

jorisvandenbossche added a commit to jorisvandenbossche/pandas that referenced this pull request Jun 7, 2018
daminisatya pushed a commit to daminisatya/pandas that referenced this pull request Jun 8, 2018
david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018
david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: implement droplevels() for flat index
4 participants