Fix statistical gallery semantics and defaults [mpl compatibility] - #335
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change aligns pyplot boxplot, violinplot, histogram, and Welch-based spectral behavior with Matplotlib compatibility expectations, updates native spectral processing to non-detrended defaults, and adds rendering and numeric regression coverage. ChangesMatplotlib compatibility corrections
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PyplotSpectralMethod
participant NativeWelchAPI
participant RustSpectralKernel
PyplotSpectralMethod->>NativeWelchAPI: request default Welch or spectrogram output
NativeWelchAPI->>RustSpectralKernel: process windowed samples without mean subtraction
RustSpectralKernel-->>NativeWelchAPI: return spectral arrays
NativeWelchAPI-->>PyplotSpectralMethod: return frequencies and spectra
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Added one more exact-gallery fix to this grouped statistics PR: Root cause: pyplot gave default filled histogram patches black edges. In the 400-bin inset, ~1.39 px strokes completely overpainted ~0.32 px blue bars in PNG; the SVG retained 400 rectangles but incorrectly stroked every one. Matplotlib leaves filled patch edges transparent unless an edge is explicitly requested or
Verification on the combined PR branch:
The image lives only on the review-evidence branch and is not part of this PR diff. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/xy/pyplot/_plot_types.py (1)
3294-3344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: bind
mean_segment/mean_pointunconditionally or compute at the emit site.Both names are conditionally assigned in each orientation branch and read ~15 lines later under a repeated
showmeans and "mean" in itemguard. It is correct today, but it reads as possibly-unbound and will trip type checkers/linters if the guards ever drift.♻️ Suggested shape
- if showmeans and "mean" in item: - mean = float(item["mean"]) - if meanline: - mean_segment = (center - half, mean, center + half, mean) - else: - mean_point = (center, mean) + # ... later, at the single emit site: + # mean = float(item["mean"]) + # mean_segment = ((center - half, mean, center + half, mean) + # if orientation == "vertical" + # else (mean, center - half, mean, center + half))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_plot_types.py` around lines 3294 - 3344, Ensure mean_segment and mean_point are initialized unconditionally before the orientation-specific branches, or compute them directly at their emission site, so both names are definitely bound when later used. Preserve the existing showmeans, "mean" in item, and meanline behavior in both branches.python/xy/pyplot/_axes.py (1)
2040-2092: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDense stacked/density/cumulative transform is well-tested but hard to follow.
The numeric semantics here (stack → normalize top envelope → optionally re-derive cumulative tops) are exercised by
test_stacked_weighted_density_matches_matplotlib_311and match the expected Matplotlib values, so I have no correctness concern. Given the flagged high complexity, consider extracting this pure-NumPy transform (inputs:counts_array,stacked,density,edges,cumulative) into a small helper sohist()itself stays readable and the transform becomes independently unit-testable.♻️ Illustrative extraction sketch
def _stack_and_accumulate( counts_array: np.ndarray, edges: np.ndarray, *, stacked: bool, density: bool, cumulative: bool | int ) -> np.ndarray: if stacked: tops = np.cumsum(counts_array, axis=0) if density: with np.errstate(divide="ignore", invalid="ignore"): tops = (tops / np.diff(edges)) / tops[-1].sum() counts_array = np.diff(np.vstack((np.zeros((1, counts_array.shape[1])), tops)), axis=0) if cumulative: ... # existing cumulative branch, unchanged return counts_array🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/pyplot/_axes.py` around lines 2040 - 2092, Extract the stacked, density-normalization, and cumulative NumPy transformation from hist() into a small pure helper, such as _stack_and_accumulate, accepting counts_array, edges, stacked, density, and cumulative. Preserve the existing operation order and numeric behavior, including weighted stacked normalization and reverse cumulative handling, then have hist() call the helper and convert its result to counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 2190-2199: Update the horizontal stepfilled histogram entry
construction around the bar properties so dashed linestyles do not render as
solid strokes. When the existing dash setting is present, either apply the same
dashed-border fallback used by the general bar path or omit the stroke
properties; preserve the current solid-edge behavior when no dash is configured.
---
Nitpick comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 2040-2092: Extract the stacked, density-normalization, and
cumulative NumPy transformation from hist() into a small pure helper, such as
_stack_and_accumulate, accepting counts_array, edges, stacked, density, and
cumulative. Preserve the existing operation order and numeric behavior,
including weighted stacked normalization and reverse cumulative handling, then
have hist() call the helper and convert its result to counts.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 3294-3344: Ensure mean_segment and mean_point are initialized
unconditionally before the orientation-specific branches, or compute them
directly at their emission site, so both names are definitely bound when later
used. Preserve the existing showmeans, "mean" in item, and meanline behavior in
both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c9c55306-2e63-4486-95fb-5dbdd82f9d6a
📒 Files selected for processing (11)
python/xy/_native.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_plot_types.pyspec/design/rust-engine.mdspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdsrc/kernels.rstests/pyplot/test_axes_charts.pytests/pyplot/test_box_violin_default_compat.pytests/pyplot/test_gallery_hist_errorbar_compat.pytests/pyplot/test_statistics_numeric_regressions.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/pyplot/test_gallery_hist_errorbar_compat.py`:
- Around line 97-125: Update
test_hist_step_outline_connects_to_baseline_in_both_orientations to unpack both
entries from horizontal_ax._entries, matching the connector and stairs entries
produced for horizontal histtype="step". Select the segments connector for the
coordinate assertions and preserve the existing endpoint checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 378ca0b5-9807-4ef8-aeee-03c36ab43ae5
📒 Files selected for processing (3)
python/xy/pyplot/_axes.pyspec/matplotlib/compat.mdtests/pyplot/test_gallery_hist_errorbar_compat.py
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/matplotlib/compat.md
- python/xy/pyplot/_axes.py
3 participants
Summary
This groups the Matplotlib statistical-chart fixes that share one compatibility surface:
psd,csd,cohere, andspecgram;The opinionated native
xy.box()andxy.violin()APIs remain unchanged; these defaults are contained in the Matplotlib shim.Why
The previous histogram path normalized each input series independently before stacking, so the top envelope was not a probability density. The previous box/violin fast paths reused native opinionated marks, which changed box fills, flier placement, returned artist counts, and violin distribution shapes relative to Matplotlib. The spectral kernel also subtracted each segment mean even though Matplotlib's omitted/default
detrendisdetrend_none.Visual comparisons
These are review-only images from immutable evidence commit
a856621; they are not part of this PR's diff.Weighted stacked density with unequal bins
The before plot's density scale reaches ~0.85 because each dataset was normalized independently. The after envelope matches Matplotlib's ~0.35 scale and shape.
Matplotlib gallery: box plot vs. violin plot
Matplotlib gallery: violin customization
Verification
statistics/histogram_multihist.py: 6 figuresstatistics/boxplot_vs_violin.py: 1 figurestatistics/violinplot.py: 1 figuregit diff --checkpassNo browser capture was used for this static-rendering group.
Summary by CodeRabbit
histrendering to match Matplotlib for stackeddensity=Truenormalization (unequal bins, cumulative modes), dash/edge styling, and consistent filled vs outline stroke handling in vertical and horizontal orientations.boxplot/violinplotadapter output to align Matplotlib-like component structure, geometry, tick/median behavior, and joined violin body styling (including updated placeholder opacity).psd/csd/cohere/specgram) to default to non-detrended output and fail loudly for unsupported detrending.