Fix bug in Series.describe where the median is included any time the percentiles argument is not None
#61158
Conversation
…n the `percentiles` argument is passed
| if len(percentiles) == 0: | ||
| return [] | ||
|
|
There was a problem hiding this comment.
This is backward-compatible as it is only extending the range of values that the input parameter can take.
pandas/core/generic.py
Outdated
| fall between 0 and 1. The default is | ||
| ``[.25, .5, .75]``, which returns the 25th, 50th, and | ||
| 75th percentiles. | ||
| fall between 0 and 1. Here are the options: | ||
| - A list-like of numbers : To include the percentiles listed. If | ||
| that list is empty, no percentiles will be returned. | ||
| - None (default) : To include the default percentiles, which are the | ||
| 25th, 50th, and 75th ones. |
There was a problem hiding this comment.
I think you can just keep the old description but clarify The default, ``None``, will automatically return the 25th, 50th, and 75th percentiles.
| **{f"{p:.0%}": df.a.quantile(p) for p in percentiles}, | ||
| "max": df.a.max(), | ||
| }, | ||
| ).to_frame(name="a") |
There was a problem hiding this comment.
Can you just create expected = DataFrame(...) instead of using to_frame?
There was a problem hiding this comment.
Is the new formulation appropriate? I passed index=['a'] and had to transpose. Maybe there is a cleaner way such as to avoid the transpose?
There was a problem hiding this comment.
You can do:
DataFrame(
[len(df.a), df.a.mean(), ..., **[df.a.quantile(p) for p in percentiles], df.a.max()],
index=pd.Index(["count", ..., **[f"{p:.0%}" for p in percentiles], ...]),
column=["a"]
)There was a problem hiding this comment.
Good point, although not sure it's really cleaner, as the list exhaustion is duplicated and this removes the clarity of the mapping from key to value. Is it fine as currently is, or would you rather prefer to have it as you mentioned just above?
There was a problem hiding this comment.
I would prefer the suggested method to not exercise other extraneous pandas APIs in this test like transpose or to_frame
There was a problem hiding this comment.
That makes total sense; will update it right now.
Co-authored-by: Matthew Roeschke <[email protected]>
|
@mroeschke Thanks for the feedback! I applied your comments. Lmk if there's anything else to update. |
|
Thanks @MartinBraquet |
.describe(percentiles = [0.25])returns 25th- and 50th-percentile #60550 (Replace xxxx with the GitHub issue number)doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.This PR aims at fixing the bug mentioned in the issue referenced above. Multiple PRs (#61024, #61023 , #60986 , #60557) have already attempted to resolve it, yet all of them got closed or stale for more than 2 weeks. So I allowed myself to apply here all the comments in the issue and previous PRs.
One last thing left hanging pertains to backward incompatibility. The only case in which the result will change is when
percentilesis a list that does not contain 0.5. Before the PR, it adds 0.5 to the result; after the PR, it does not include it.If we add a warning message in such case, it would be considered as:
So, what is pandas' policy regarding warning messages and its potentially large number of false alerts?