-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
Deprecate level keyword for dataframe and series aggregations #40869
Conversation
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.
wouldn't be averse to change some of the tests to use the groupby instead of the reduction method to remove / avoid the warnings (some, not all)
doc/source/user_guide/advanced.rst
Outdated
@@ -492,6 +492,7 @@ Using the parameter ``level`` in the :meth:`~DataFrame.reindex` and | |||
values across a level. For instance: | |||
|
|||
.. ipython:: python | |||
:okwarning: |
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.
would just remove this example
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.
may instead show a groupby example of this (but then i would put it in the groupby section), you can leave a pointer if you'd like here
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.
If ok, I would keep it there, there are level examples above and below this snippet
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.
But changed to groupby
@@ -625,6 +625,7 @@ even if some categories are not present in the data: | |||
``DataFrame`` methods like :meth:`DataFrame.sum` also show "unused" categories. | |||
|
|||
.. ipython:: python | |||
:okwarning: |
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 change this so its a regular group op
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
doc/source/user_guide/groupby.rst
Outdated
@@ -325,6 +325,7 @@ directly. Additionally, the resulting index will be named according to the | |||
chosen level: | |||
|
|||
.. ipython:: python |
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.
point the example from advanced.rst here (and make this a proper groupby example)
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.
Removed, since above is the groupby case
@@ -16,17 +16,22 @@ def test_count_multiindex(self, multiindex_dataframe_random_data): | |||
frame = frame.copy() | |||
frame.index.names = ["a", "b"] | |||
|
|||
result = frame.count(level="b") | |||
expected = frame.count(level=1) | |||
with tm.assert_produces_warning(FutureWarning): |
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 move these to a new test_count_with_level_deprecated
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.
yep, done
Most of the tests are pretty basic and already covered in groupby, the more complicated ones are mostly checking if the aggregation with level equals a groupby, so don't know if this makes sense here? |
kk sounds good |
thanks @phofl |
level
parameter for aggregations in DataFrame and Series #39983This would deprecate the level keyword if we want to move forward with this