-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Idx droplevel #21116
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
pandas/tests/indexes/test_base.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
Should this be added to |
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 |
Ready to merge if there are no objections |
will look in a bit |
There was a problem hiding this 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.
|
just add it to both |
There is no "both" as there is no specific |
@@ -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`) |
There was a problem hiding this comment.
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).
thanks! |
@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?) |
Yes, in practice that is what it means. |
OK! So you're taking care of this in your PR? Or should I open another one? |
Yep, will do |
git diff upstream/master -u -- "*.py" | flake8 --diff
Collateral change: if
mi
is aMultiIndex
,mi.droplevel([])
will now returnmi
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.