fix: legend, style persistence, and axis ranges in combined figures#53
Merged
fix: legend, style persistence, and axis ranges in combined figures#53
Conversation
…d figures Combined figures (overlay, add_secondary_y) had three issues: 1. Legends disappeared because Plotly Express sets showlegend=False on single-trace figures. Now unnamed traces get names derived from the source figure's y-axis title, and showlegend is fixed per legendgroup. 2. Colors and styles were lost during animation because frame traces carried PX defaults. Now marker, line, opacity and legend properties are propagated from fig.data into all animation frame traces. 3. Axis ranges were computed from fig.data only, so frames with different data ranges went off-screen during animation. Now global min/max is computed across all frames and set explicitly on the layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a public Changes
Sequence DiagramsequenceDiagram
participant User
participant API as Subplots/Overlay/AddSecondaryY
participant Remapper as AxisRemapper
participant Legend as LegendManager
participant Frames as FrameMerger
participant AxisFix as AxisRangeFixer
participant Output as ResultFigure
User->>API: call with one or more figures
API->>Remapper: remap axes, place traces/frames into grid cells
Remapper->>Frames: provide frame traces for merging/remapping
Remapper->>Legend: provide source metadata for legend handling
Legend->>Frames: propagate legend visibility into frames
Frames->>AxisFix: compute global ranges across frames/traces
AxisFix->>Output: set final axis ranges and layout
Output->>User: return combined figure
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xarray_plotly/figures.py (1)
103-111: Consider deep-copying complex style attributes to prevent unintended mutations.
setattr(frame_trace, attr, src_val)assignsmarkerandlineobjects by reference. If frame traces are later mutated, they could inadvertently modify the shared object fromcombined.data. While unlikely in typical usage, defensive deep-copying would be safer.♻️ Optional: Deep-copy complex attributes
_STYLE_ATTRS = ("name", "legendgroup", "showlegend", "marker", "line", "opacity") + _COMPLEX_ATTRS = {"marker", "line"} for frame in combined.frames or []: for i, frame_trace in enumerate(frame.data): if i < len(combined.data): src = combined.data[i] for attr in _STYLE_ATTRS: src_val = getattr(src, attr, None) if src_val is not None: - setattr(frame_trace, attr, src_val) + if attr in _COMPLEX_ATTRS: + setattr(frame_trace, attr, copy.deepcopy(src_val)) + else: + setattr(frame_trace, attr, src_val)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xarray_plotly/figures.py` around lines 103 - 111, The loop that copies style attributes from combined.data into frame.data (variables: combined, frame, frame_trace, _STYLE_ATTRS) assigns complex objects like marker and line by reference, risking shared-mutation; modify the setattr logic in that loop to deep-copy values for complex attributes (e.g., marker and line or any non-primitive) before assigning (use Python's copy.deepcopy on src_val when its type is mutable/complex) while leaving primitives assigned directly to avoid unnecessary copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@xarray_plotly/figures.py`:
- Around line 103-111: The loop that copies style attributes from combined.data
into frame.data (variables: combined, frame, frame_trace, _STYLE_ATTRS) assigns
complex objects like marker and line by reference, risking shared-mutation;
modify the setattr logic in that loop to deep-copy values for complex attributes
(e.g., marker and line or any non-primitive) before assigning (use Python's
copy.deepcopy on src_val when its type is mutable/complex) while leaving
primitives assigned directly to avoid unnecessary copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 220d956f-bb12-483d-a2bc-6df5ad750bf8
📒 Files selected for processing (2)
tests/test_figures.pyxarray_plotly/figures.py
New function to arrange independent figures in a subplot grid:
grid = subplots(fig1, fig2, fig3, cols=2)
- Subplot titles auto-derived from figure title or y-axis label
- Axis config (titles, tick format, type) copied from source figures
- Validates: rejects faceted or animated figures (not supported)
- Empty cells via go.Figure()
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/examples/combining.ipynb`:
- Around line 10-15: The notebook advertises slider_to_dropdown but never
imports or demonstrates it; either add slider_to_dropdown to the import cell and
to the xarray_plotly export so it’s available (alongside add_secondary_y,
config, overlay, subplots, xpx) and include a short example cell that converts a
figure’s animation slider to a dropdown, or remove the slider_to_dropdown bullet
from the intro so the notebook and package exports stay consistent. Ensure you
update the import statement that references
add_secondary_y/config/overlay/subplots/xpx and the package export
(xarray_plotly.__init__ or equivalent) to match whichever option you choose.
In `@xarray_plotly/figures.py`:
- Around line 614-620: The truthiness check on fig.layout.title.text (assigned
to title) doesn't narrow its Any type for mypy; update the logic in the function
containing this snippet to ensure you only return when title is a str (use
isinstance(title, str) in addition to the truthiness check). Specifically,
inside the try block where title = fig.layout.title.text, change the condition
to verify isinstance(title, str) (and truthiness if desired) before returning
title, otherwise fall back to calling _get_yaxis_title(fig).
- Around line 164-178: When computing axis ranges in figures.py (the loops over
y_by_axis and x_by_axis), ensure zero is included for axes that host bar traces
before applying padding: inspect fig.data for traces with type == "bar" and for
y-axes include bars with orientation != "h" (vertical bars), and for x-axes
include bars with orientation == "h" (horizontal bars); if any such bar trace is
present on the current axis_ref, clamp lo = min(lo, 0) and hi = max(hi, 0)
before computing pad and setting fig.layout[layout_prop].range so animated bars
grow from the zero baseline rather than floating.
- Around line 143-160: The code coerces trace.x/trace.y to float with
np.asarray(..., dtype=float) which converts datetime64/timedelta64 to numeric
epochs and corrupts date autorange; before casting, create arr = np.asarray(x or
y) and if np.issubdtype(arr.dtype, np.datetime64) or np.issubdtype(arr.dtype,
np.timedelta64) skip adding to x_by_axis/x_by_axis (i.e., do not call
np.asarray(..., dtype=float) or extend x_by_axis/y_by_axis) so date axes remain
on autorange; update the branches around the np.asarray(..., dtype=float) calls
and ensure you add a regression test covering datetime64 coordinates with
animation frames to prevent regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 521ba76b-9ead-4918-8f45-83e72aaad9f3
📒 Files selected for processing (4)
docs/examples/combining.ipynbtests/test_figures.pyxarray_plotly/__init__.pyxarray_plotly/figures.py
Rewrote subplots() to use manual axis domain management instead of make_subplots. Each figure's internal axes are remapped with scaled domains to fit within the grid cell, so faceted figures now work. Updated notebook with faceted subplots example. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
xarray_plotly/figures.py (1)
615-617:⚠️ Potential issue | 🟠 MajorLine 617 still returns
Any, which keeps mypy failing.
titleis not narrowed tostrbefore return; this aligns with the current CI failure (no-any-return).Minimal fix
try: title = fig.layout.title.text - if title: + if isinstance(title, str) and title: return title except AttributeError: pass#!/bin/bash uv run mypy xarray_plotly/figures.py --show-error-codes | rg -n "no-any-return|_get_figure_title|figures.py"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xarray_plotly/figures.py` around lines 615 - 617, The variable title (from fig.layout.title.text) is typed as Any so returning it causes mypy failure; in the _get_figure_title code path ensure you return a concrete str by either checking its type (e.g., if isinstance(title, str): return title) or converting/casting it to str (e.g., return str(title) or return cast(str, title) after importing typing.cast), so the function no longer returns Any—update the return to a guaranteed str using the title variable and the fig.layout.title.text expression.docs/examples/combining.ipynb (1)
13-15:⚠️ Potential issue | 🟡 Minor
slider_to_dropdownis listed but still not imported or demonstrated.The notebook advertises it in the intro, but the import cell and examples do not expose usage, so readers hit a dead end.
Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/examples/combining.ipynb` around lines 13 - 15, The README/intro mentions slider_to_dropdown but the notebook never imports or demonstrates it: add an import for the slider_to_dropdown utility (the symbol slider_to_dropdown) in the notebook's import cell and add a short example cell showing its usage with a sample animated plot (e.g., convert an existing animation slider to a dropdown using slider_to_dropdown and display the updated figure); ensure the example references the same variable names used elsewhere in the notebook so readers can run it end-to-end.
🧹 Nitpick comments (1)
tests/test_figures.py (1)
836-836: Update stale comment referencingmake_subplots.The implementation now uses custom remapping, so this test comment is outdated and may mislead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_figures.py` at line 836, Replace the stale comment that mentions make_subplots with a concise, accurate note about the new custom remapping behavior: update the comment near the test that currently reads "make_subplots omits annotations for empty titles" to something like "custom remapping omits annotations for empty titles" or remove it entirely so it no longer references make_subplots; locate the comment by searching for the string "make_subplots omits annotations for empty titles" in the test file and update the text adjacent to the relevant test/assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xarray_plotly/figures.py`:
- Around line 59-63: The disambiguation currently uses unique_labels = {lb for
lb in labels if lb} and runs when len(unique_labels) == 1, which incorrectly
triggers for mixed lists containing an empty label; change the condition so
renaming only happens when every label is non-empty and they are all identical.
Update the check around labels/unique_labels in the function (use the variables
labels and unique_labels) to require that all(lb for lb in labels) (i.e., no
empty strings) and len(unique_labels) == 1 before constructing the disambiguated
labels list.
---
Duplicate comments:
In `@docs/examples/combining.ipynb`:
- Around line 13-15: The README/intro mentions slider_to_dropdown but the
notebook never imports or demonstrates it: add an import for the
slider_to_dropdown utility (the symbol slider_to_dropdown) in the notebook's
import cell and add a short example cell showing its usage with a sample
animated plot (e.g., convert an existing animation slider to a dropdown using
slider_to_dropdown and display the updated figure); ensure the example
references the same variable names used elsewhere in the notebook so readers can
run it end-to-end.
In `@xarray_plotly/figures.py`:
- Around line 615-617: The variable title (from fig.layout.title.text) is typed
as Any so returning it causes mypy failure; in the _get_figure_title code path
ensure you return a concrete str by either checking its type (e.g., if
isinstance(title, str): return title) or converting/casting it to str (e.g.,
return str(title) or return cast(str, title) after importing typing.cast), so
the function no longer returns Any—update the return to a guaranteed str using
the title variable and the fig.layout.title.text expression.
---
Nitpick comments:
In `@tests/test_figures.py`:
- Line 836: Replace the stale comment that mentions make_subplots with a
concise, accurate note about the new custom remapping behavior: update the
comment near the test that currently reads "make_subplots omits annotations for
empty titles" to something like "custom remapping omits annotations for empty
titles" or remove it entirely so it no longer references make_subplots; locate
the comment by searching for the string "make_subplots omits annotations for
empty titles" in the test file and update the text adjacent to the relevant
test/assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b8f7ccb-5eff-45fb-a299-17d7c245b27a
📒 Files selected for processing (3)
docs/examples/combining.ipynbtests/test_figures.pyxarray_plotly/figures.py
…tale bullet - Skip datetime64/timedelta64 axes in _fix_animation_axis_ranges to prevent float epoch corruption; leave them on autorange - Include zero in axis range for bar traces so bars grow from baseline - Use isinstance(str) check in _get_figure_title for mypy type narrowing - Remove stale slider_to_dropdown bullet from notebook intro Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
xarray_plotly/figures.py (1)
59-63:⚠️ Potential issue | 🟡 MinorDisambiguation may incorrectly trigger with mixed empty/non-empty labels.
When
labels = ["", "Pressure"], the set comprehension produces{"Pressure"}with length 1, triggering disambiguation. This results in"Pressure (1)"and"Pressure (2)"even though only one figure has a label. The condition should require all labels to be non-empty before disambiguating.,
Suggested fix
- unique_labels = {lb for lb in labels if lb} - if len(unique_labels) == 1: - labels = [f"{labels[0]} ({i + 1})" for i in range(len(labels))] + non_empty = [lb for lb in labels if lb] + if len(non_empty) == len(labels) and len(set(non_empty)) == 1: + labels = [f"{non_empty[0]} ({i + 1})" for i in range(len(labels))]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xarray_plotly/figures.py` around lines 59 - 63, The disambiguation block currently uses unique_labels = {lb for lb in labels if lb} and triggers when len(unique_labels) == 1, which misfires if some labels are empty; change the condition to only run when every label is non-empty and they are all identical (e.g., check all(labels) and len(unique_labels) == 1) so that labels like ["", "Pressure"] won't be renamed; update the block around the labels variable/unique_labels calculation and the disambiguation list comprehension to use that combined condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_figures.py`:
- Around line 790-791: The unpacking of combined.layout.yaxis.range assigns lo
and hi but hi is unused, triggering a linter error; change the unpack to discard
the second value (e.g., replace the assignment "lo, hi =
combined.layout.yaxis.range" with "lo, _ = combined.layout.yaxis.range") so lo
is kept and the unused variable is explicitly ignored in the test that asserts
lo <= 0.
---
Duplicate comments:
In `@xarray_plotly/figures.py`:
- Around line 59-63: The disambiguation block currently uses unique_labels = {lb
for lb in labels if lb} and triggers when len(unique_labels) == 1, which
misfires if some labels are empty; change the condition to only run when every
label is non-empty and they are all identical (e.g., check all(labels) and
len(unique_labels) == 1) so that labels like ["", "Pressure"] won't be renamed;
update the block around the labels variable/unique_labels calculation and the
disambiguation list comprehension to use that combined condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f17f82d-4ecd-4eaa-871a-8ec1fc5b4bfe
📒 Files selected for processing (3)
docs/examples/combining.ipynbtests/test_figures.pyxarray_plotly/figures.py
- Only disambiguate labels when all are non-empty and identical, preventing spurious suffixes when mixing named and unnamed figures - Update stale make_subplots comment in test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_figures.py (1)
790-791:⚠️ Potential issue | 🟡 MinorFix pipeline failure: unused variable
hi.The linter reports RUF059 because
hiis unpacked but never used. Use_to indicate intentional discard.Suggested fix
- lo, hi = combined.layout.yaxis.range + lo, _ = combined.layout.yaxis.range assert lo <= 0, f"Bar y-axis range should include 0, got lo={lo}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_figures.py` around lines 790 - 791, The test unpacks combined.layout.yaxis.range into lo, hi but never uses hi, causing an unused-variable linter error; change the unpack to lo, _ = combined.layout.yaxis.range (or otherwise discard the second value) so that lo is used and the unused-variable warning is avoided—update the unpack in the test function that references combined.layout.yaxis.range in tests/test_figures.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_figures.py`:
- Around line 790-791: The test unpacks combined.layout.yaxis.range into lo, hi
but never uses hi, causing an unused-variable linter error; change the unpack to
lo, _ = combined.layout.yaxis.range (or otherwise discard the second value) so
that lo is used and the unused-variable warning is avoided—update the unpack in
the test function that references combined.layout.yaxis.range in
tests/test_figures.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6be066f8-2e22-4192-b74b-02f10fb05f19
📒 Files selected for processing (2)
tests/test_figures.pyxarray_plotly/figures.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
showlegendis deduplicated per legendgroup.fig.datainto all animation frame traces, so styles survive frame switches.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes