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

ENH: removed median percentile to be always included in describe GH #60550 #60557

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ZenithClown
Copy link

@ZenithClown ZenithClown commented Dec 13, 2024

- fixes pandas-dev#60550
- median percentile is default when a blank list of percentiles is passed
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Thanks could you add a test for this?

@mroeschke mroeschke added the Reduction Operations sum, mean, min, max, etc. label Dec 13, 2024
@ZenithClown
Copy link
Author

@mroeschke I don't think a separate test is needed as the core functionality remains the same.

@rhshadrach
Copy link
Member

@mroeschke I don't think a separate test is needed as the core functionality remains the same.

We should have had a test that when you specify [0.3, 0.6] you get 0.3, 0.5, and 0.6. This was missing in our test coverage. If it was there, then you would have had to modify such a test to just 0.3 and 0.6.

Since we're changing the behavior, I think a test needs to be added.

@ZenithClown
Copy link
Author

@mroeschke @rhshadrach thanks for the feedback, the tests are now added.

@ZenithClown
Copy link
Author

@mroeschke, I hope you were able to look into this and merge the PR?

Co-authored-by: Asish Mahapatra <[email protected]>
@ZenithClown
Copy link
Author

@mroeschke please do the needful.

@@ -74,7 +74,8 @@ def describe_ndframe(
percentiles : list-like of numbers, optional
The percentiles to include in the output. All should fall between 0 and 1.
The default is ``[.25, .5, .75]``, which returns the 25th, 50th, and
75th percentiles.
75th percentiles.If a blank list is passed, then returns only the
Copy link
Member

Choose a reason for hiding this comment

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

Space after .

Comment on lines +39 to +40
- Median percentile is only included in :meth:`~Series.describe` when a blank
list is passed (:issue:`60550`).
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to v3.0.0.rst. We should not do this breaking change in 2.3.

Comment on lines +10798 to +10799
75th percentiles. If a blank list is passed, then returns
only the 50th percentile value.
Copy link
Member

Choose a reason for hiding this comment

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

I do not think this should be the behavior. If I pass an empty list, I should not get any percentiles.

Copy link
Author

Choose a reason for hiding this comment

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

I was trying to preserve the default percentile behavior. I will change this so that percentiles are omitted if a blank list is passed.

Copy link
Contributor

@asishm asishm Jan 11, 2025

Choose a reason for hiding this comment

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

I don't think we should remove the 50th percentile without at least a deprecation warning. Reporting the median by default is incredibly helpful (by default)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reduction Operations sum, mean, min, max, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Passing a single value to .describe(percentiles = [0.25]) returns 25th- and 50th-percentile
4 participants