fix(pandas_postprocessing): avoid FutureWarning for mean/median in boxplot#42004
fix(pandas_postprocessing): avoid FutureWarning for mean/median in boxplot#42004eschutho wants to merge 2 commits into
Conversation
… avoid FutureWarning boxplot() built its own operators map with raw np.mean/np.median callables, bypassing the string-based fix from apache#41025 and still triggering pandas' GroupBy.agg FutureWarning about ambiguous callables. Passing the string names routes through the same code path that already special-cases them, with no change in output.
…ian FutureWarning Guards against the callable-vs-string aggfunc regression by asserting no FutureWarning is raised and that mean/median values match a plain pandas groupby.
Code Review Agent Run #1de445Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| assert len(df) == 4 | ||
|
|
||
|
|
||
| def test_boxplot_mean_median_no_future_warning(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function signature (for example, -> None) to satisfy the type-hint requirement for newly introduced Python functions. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new test function is missing an explicit return type annotation. Under the Python type-hint rule, newly introduced functions should be annotated, so -> None should be added here.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/pandas_postprocessing/test_boxplot.py
**Line:** 50:50
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function signature (for example, `-> None`) to satisfy the type-hint requirement for newly introduced Python functions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The suggestion to add an explicit return type annotation to the new test function is correct and follows standard Python type-hinting practices. You can resolve this by updating the function signature to include Here is the corrected function signature: def test_boxplot_mean_median_no_future_warning() -> None:
"""mean/median must be passed as strings (not np.mean/np.median) to
GroupBy.agg, else pandas raises a FutureWarning. Also verify the values
match a plain pandas groupby, since the string and callable forms could
silently diverge on a future pandas version."""There are no other review comments available in the provided context to check. Would you like me to proceed with any other tasks? tests/unit_tests/pandas_postprocessing/test_boxplot.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42004 +/- ##
=======================================
Coverage 65.01% 65.01%
=======================================
Files 2744 2744
Lines 153571 153571
Branches 35212 35212
=======================================
Hits 99842 99842
Misses 51822 51822
Partials 1907 1907
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
What
Production logs showed a high-volume pandas
FutureWarning:This exact class of warning was previously addressed in #41025, which taught
_get_aggregate_funcs(inpandas_postprocessing/utils.py) to pass the string operator name (e.g."mean") instead of a raw numpy callable when possible, avoiding the ambiguity pandas warns about.However,
pandas_postprocessing/boxplot.pybuilds its ownoperatorsdict independently and was passingnp.mean/np.mediandirectly as theoperatorvalue. Since_get_aggregate_funcsspecial-cases callables first (if callable(operator): aggfunc = operator), this bypassed the #41025 fix entirely — so every boxplot chart post-processing call kept emitting the warning. Boxplot charts are common, which is why this was showing up frequently in prod logs.Change
In
boxplot.py, pass"mean"/"median"as string literals instead ofnp.mean/np.median, so they route through the same string-based path_get_aggregate_funcsalready uses.np.ma.countis left untouched, since it intentionally differs from pandas'"count"(it includes NaNs).No behavior change
Verified empirically that the string and callable forms produce identical output for
mean/medianon the currently pinned pandas range (>=2.1.4,<2.4). Added a regression test that also guards against a real (not just cosmetic) divergence: on pandas ≥3.0,np.medianon a group containing NaNs returnsNaN(noskipna), while the"median"string aggregator still skips NaNs — so this also prevents a silent numeric regression whenever the pandas pin is eventually bumped.Test plan
test_boxplot_mean_median_no_future_warningintests/unit_tests/pandas_postprocessing/test_boxplot.py: asserts noFutureWarningis raised and thatmean/medianvalues match a plaindf.groupby(...).agg(["mean", "median"])call.tests/unit_tests/pandas_postprocessing/suite locally (108 tests, all passing).ruff check/ruff formatandmypyon the changed file — no new issues introduced (two pre-existing, unrelated mypy errors on lines 103-104 remain untouched).