fix(plotting): only pin animation axis ranges when frames exceed the initial view#90
fix(plotting): only pin animation axis ranges when frames exceed the initial view#90FBumann wants to merge 3 commits into
Conversation
…initial view overlay() and add_secondary_y() unconditionally pinned explicit numeric ranges on every axis of an animated figure. This broke two cases: - Switching an axis to type='category' after combining reinterpreted the stale numeric range as category indices, showing an empty plot until the user pressed the autoscale button. - User-set explicit ranges and log axes (whose range coordinates are exponents) were overwritten with data-coordinate ranges. Ranges are now pinned only when a later frame's data exceeds the extent of fig.data — the one case plotly.js autorange cannot handle, since it computes the range from the first frame only and never recalculates during animation. Axes already covered by the initial view keep live autorange, pre-set ranges are respected, and log/date/category axes are never touched. Additionally, with barmode='stack'/'relative' the pinned range is now computed from per-category stacked sums instead of individual segment values, so tall stacks are no longer clipped at the top. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOverlay animation-axis range fixing now evaluates frame extents, stacked bars, axis types, and explicit ranges. Tests cover unchanged, expanded, stacked, logarithmic, categorical, and user-defined axis ranges. ChangesAnimation axis pinning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Overlay
participant _fix_animation_axis_ranges
participant FigureLayout
Overlay->>_fix_animation_axis_ranges: process base data and animation frames
_fix_animation_axis_ranges->>FigureLayout: inspect axis types and existing ranges
_fix_animation_axis_ranges->>FigureLayout: set ranges only for expanded numeric extents
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Fold the bar zero-baseline into extent collection (a bar's value axis simply contributes 0.0), removing the separate zero-clamp pass and list-copying. - Reuse the existing _axis_layout_key() helper instead of reimplementing the axis-ref-to-layout-key conversion. - Drop the float tolerance: frame values are bit-identical to fig.data values and sums are computed identically, so exact comparison is safe. - Inline the two single-use module constants. - Use deterministic data in the shared-extent test; random values made the frame-exceeds-initial condition a coin flip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xarray_plotly/figures.py (1)
147-202: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTreat mixed-sign
barmode='stack'as a separate corner case. The shared positive/negative split matchesrelative, butstackcan diverge with mixed signs, so the computed axis range may be looser than Plotly.js there.🤖 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 `@xarray_plotly/figures.py` around lines 147 - 202, Update _collect_axis_extents so barmode="stack" with mixed-sign bar values is handled separately from the shared positive/negative accumulation used for relative stacking. Preserve the existing relative behavior, and ensure stack-mode candidate extents match Plotly.js for mixed-sign categories rather than unconditionally adding both positive and negative sums.
🤖 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.
Nitpick comments:
In `@xarray_plotly/figures.py`:
- Around line 147-202: Update _collect_axis_extents so barmode="stack" with
mixed-sign bar values is handled separately from the shared positive/negative
accumulation used for relative stacking. Preserve the existing relative
behavior, and ensure stack-mode candidate extents match Plotly.js for mixed-sign
categories rather than unconditionally adding both positive and negative sums.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7855d423-11eb-4a7d-bca1-527bf268bc8c
📒 Files selected for processing (2)
tests/test_figures.pyxarray_plotly/figures.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
overlay()/add_secondary_y()unconditionally pin explicit numeric ranges on every axis of an animated figure (via_fix_animation_axis_ranges). This caused an empty plot when the user switched an axis totype='category'after combining: the stale numeric range (e.g.[2024, 2041]from year values) gets reinterpreted as category indices, putting the viewport far past the 4 real categories until the autoscale button is pressed.It also overwrote user-set explicit ranges, and would corrupt log axes (log ranges are exponents, not data values).
Fix
Pin a range only when actually necessary — i.e. when a later frame's data exceeds the extent of
fig.data. That is the one case plotly.js autorange can't handle, since it computes the range from the first frame only and never recalculates during animation (the gapminder scenario). Everything else stays on live autorange, which:type='category'switch working (frames sharing the same categories never get an x-range pinned),log/date/categoryaxes.Bonus fix: stacked bars were clipped
The pinned y-range was computed from individual segment values, ignoring that
barmode='stack'/'relative'stacks them — so tall stacks were clipped at the top (also only visible until pressing autoscale). The range is now computed from per-category positive/negative sums, per frame.Tests
8 new tests covering: unchanged-extent axes staying on autorange, pinning when a later frame exceeds the initial view, stacked positive/negative sums, explicit-range preservation, log-axis skip, and an end-to-end regression test for the
type='category'bug. Existing guarantees (datetime axes untouched, bar zero baseline) still pass. Full suite: 181 passed.🤖 Generated with Claude Code
Summary by CodeRabbit
stack/relative, including negative stacked totals and keeping the zero baseline.