fix: keep secondary traces visible in legend when combining figures#76
fix: keep secondary traces visible in legend when combining figures#76
Conversation
add_secondary_y was deduplicating legendgroups across both source figures, hiding the secondary's traces from the legend whenever the two figures shared legendgroup names (e.g. PX color= producing the same categories on both axes). Cross-source dedup is correct for overlay (same data, two display styles) but wrong for add_secondary_y, where each side plots different data on its own axis. Add a cross_source_dedup flag to _ensure_legend_visibility. overlay keeps the existing behavior; add_secondary_y opts out and instead namespaces colliding legendgroups with the source label and dedupes within each slice independently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors legend handling: introduce LegendMode type, add a within-trace dedup helper, make _ensure_legend_visibility mode-driven (merge/suffix/separate), extend add_secondary_y to accept legend modes and set default secondary-y layout, and add tests and notebook examples for multi-trace legend behaviors. ChangesLegend visibility and add_secondary_y
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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)
45-52: 💤 Low valueOptional: make the dedup loop exhaustive by explicitly silencing nameless grouped traces.
Currently, a trace with a
legendgroupbut a falsynamethat appears before the first named trace in the group hits neither branch — its originalshowlegendvalue is preserved. If Plotly Express had left it asTrue(e.g., first trace in a faceted legendgroup before Step 1 relabelling), that nameless trace and the subsequent named trace would both haveshowlegend=True, producing a blank legend entry. Standardxpxusage doesn't trigger this because PX always setsname == legendgroup, but the function's docstring contract is stricter than the implementation.🛡️ Proposed fix
for group_traces in grouped.values(): has_visible = False for t in group_traces: if has_visible: t.showlegend = False elif getattr(t, "name", None): t.showlegend = True has_visible = True + else: + t.showlegend = False🤖 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 45 - 52, The deduplication loop over grouped traces can leave a leading nameless trace's showlegend unchanged; update the loop in the function that iterates over grouped.values() (the variables group_traces and t) to explicitly set t.showlegend = False in the case where the trace has no truthy name, i.e. add an else branch after the existing elif getattr(t, "name", None) that sets t.showlegend = False so any nameless trace before the first named trace is silenced.
🤖 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 45-52: The deduplication loop over grouped traces can leave a
leading nameless trace's showlegend unchanged; update the loop in the function
that iterates over grouped.values() (the variables group_traces and t) to
explicitly set t.showlegend = False in the case where the trace has no truthy
name, i.e. add an else branch after the existing elif getattr(t, "name", None)
that sets t.showlegend = False so any nameless trace before the first named
trace is silenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a241855b-cd91-4a19-80f7-179aac59cdc4
📒 Files selected for processing (2)
tests/test_figures.pyxarray_plotly/figures.py
Demonstrates the legend fix: two figures sharing categorical legendgroups (e.g. country dimension) now produce 8 distinct legend entries instead of hiding the secondary's traces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…axis Anchor the legend to the figure container's right edge (xref="container", xanchor="right") and keep its vertical position in paper coords (top of plot area). Combined with automargin=True on each secondary y-axis, Plotly reserves space for the axis title between the plot and the legend, so they no longer overlap. Existing user-set legend.x/y are preserved. Also expand docs/examples/combining.ipynb with two edge-case examples to visually verify features compose: - Multi-trace within facets on both axes - overlay -> add_secondary_y composition Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI mypy 1.x flagged the set comprehension as Set[Any | None] (the truthy filter wasn't narrowing). Rewrite as an explicit loop that adds only truthy strings, dropping the now-stale type: ignore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `legend: Literal["suffix", "merge", "separate"] = "suffix"` so the
caller picks how same-named traces from the two source figures are
presented:
- "suffix" (default, current behavior): each trace gets its own legend
entry with the source y-axis title appended ("Brazil (Population)",
"Brazil (GDP per Capita)"). Each toggles independently.
- "merge": same-named traces share a legendgroup, collapsing to a single
legend entry that toggles both axes together (Plotly's default
groupclick="togglegroup").
- "separate": PX legend output is left untouched; duplicate names across
the two figures are accepted, each toggles alone.
`_ensure_legend_visibility` was refactored from a `cross_source_dedup`
bool to a `mode` parameter using the new `LegendMode` Literal in
`xarray_plotly.common`. Tests cover all three modes plus an invalid
mode error path. Notebook adds an example of `legend="merge"`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
xarray_plotly/figures.py (2)
33-58: 💤 Low value
_dedup_legend_within_tracesmay override an explicitshowlegend=Falseon ungrouped traces.Lines 56–58 unconditionally force
showlegend=Truefor any named, ungrouped trace, overriding a caller's deliberateupdate_traces(showlegend=False). The grouped branch is similarly forceful (line 51) but only after the first visible entry, which is mostly desired. The ungrouped branch has no such gate.If preserving caller intent matters here, consider only flipping
showlegendwhen it is currentlyNone(unset), e.g.:♻️ Optional: respect explicit user overrides
for trace in ungrouped: - if getattr(trace, "name", None): + if getattr(trace, "name", None) and getattr(trace, "showlegend", None) is None: trace.showlegend = TrueSame idea could apply on Line 53 if preserving an explicitly
False-set first trace per group is desired.This mirrors prior behavior, so it's not a regression — just worth confirming intent.
🤖 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 33 - 58, _dedup_legend_within_traces currently forces showlegend=True on named traces and can override an explicit showlegend=False; change the logic so you only set t.showlegend = True when the trace's showlegend is unset (None) to preserve caller intent; specifically, in the grouped loop (function _dedup_legend_within_traces, variable group_traces and t) only mark the first named trace visible if getattr(t, "showlegend", None) is None, and in the ungrouped loop (variable ungrouped and trace) only set trace.showlegend = True when getattr(trace, "showlegend", None) is None.
113-152: 💤 Low valueMode validation runs after step-1 mutations.
The
else: raise ValueError(...)arm fires only after step 1 has already relabeled unnamed traces and setlegendgroup. Becausecombinedis a freshly built local figure, the leak isn't observable today, but a defensive up-front guard is cheap and makes the contract clearer:♻️ Optional: validate mode up front
from collections import defaultdict + + if mode not in ("merge", "suffix", "separate"): + raise ValueError(f"legend mode must be 'suffix', 'merge', or 'separate', got {mode!r}") # --- Step 1: label unnamed traces from source y-axis titles -----------Then drop the trailing
else:arm at lines 151–152.🤖 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 113 - 152, Validate the incoming mode value before any mutations to the local Figure (before you relabel unnamed traces or set legendgroup) by checking that mode is one of "merge", "suffix", or "separate" and raising ValueError early if not; this should be added near the start of the function that contains variables mode, combined, trace_slices, and labels, and then remove the trailing else: raise ValueError(...) branch at the end of the shown conditional so that mode validation is performed up-front and no mutations occur for invalid modes.
🤖 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 `@xarray_plotly/figures.py`:
- Around line 637-663: The change uses legend.xref="container" which requires
plotly>=5.15.0; update _set_default_secondary_y_layout to avoid breaking older
plotly by either (A) requiring plotly>=5.15.0 in packaging or (B) guarding the
assignment: detect plotly version (plotly.__version__) and only set
legend_defaults["xref"]="container" (and x/xanchor) when version >= "5.15.0", or
attempt to set it inside a try/except (catch plotly schema/ValueError) and skip
xref if it fails; keep the rest of the legend defaults logic intact and
reference the function name _set_default_secondary_y_layout and the
legend.xref/legend_defaults keys when making the change.
---
Nitpick comments:
In `@xarray_plotly/figures.py`:
- Around line 33-58: _dedup_legend_within_traces currently forces
showlegend=True on named traces and can override an explicit showlegend=False;
change the logic so you only set t.showlegend = True when the trace's showlegend
is unset (None) to preserve caller intent; specifically, in the grouped loop
(function _dedup_legend_within_traces, variable group_traces and t) only mark
the first named trace visible if getattr(t, "showlegend", None) is None, and in
the ungrouped loop (variable ungrouped and trace) only set trace.showlegend =
True when getattr(trace, "showlegend", None) is None.
- Around line 113-152: Validate the incoming mode value before any mutations to
the local Figure (before you relabel unnamed traces or set legendgroup) by
checking that mode is one of "merge", "suffix", or "separate" and raising
ValueError early if not; this should be added near the start of the function
that contains variables mode, combined, trace_slices, and labels, and then
remove the trailing else: raise ValueError(...) branch at the end of the shown
conditional so that mode validation is performed up-front and no mutations occur
for invalid modes.
🪄 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
Run ID: 7f3cf0b1-6fa2-4614-be7f-2051633c775d
📒 Files selected for processing (4)
docs/examples/combining.ipynbtests/test_figures.pyxarray_plotly/common.pyxarray_plotly/figures.py
✅ Files skipped from review due to trivial changes (1)
- xarray_plotly/common.py
| def _set_default_secondary_y_layout(fig: go.Figure) -> None: | ||
| """Anchor the legend to the figure container so it doesn't fight the | ||
| secondary y-axis for paper-coordinate space. | ||
|
|
||
| With ``xref="container"`` the legend's right edge sits at the figure's | ||
| right edge regardless of plot width. Combined with ``automargin=True`` | ||
| on the secondary y-axes (set in ``add_secondary_y``), Plotly reserves | ||
| space for the axis title between the plot and the legend, so the two | ||
| do not overlap. Only fields the user has not already set are touched, | ||
| so explicit ``update_layout(legend=...)`` on the source figures wins. | ||
| """ | ||
| legend_defaults: dict[str, Any] = {} | ||
| legend = fig.layout.legend | ||
| if legend.x is None: | ||
| # Container-relative x so the legend sits at the figure's right edge | ||
| # rather than fighting the secondary y-axis title for paper-coord space. | ||
| legend_defaults["x"] = 1.0 | ||
| legend_defaults["xanchor"] = "right" | ||
| legend_defaults["xref"] = "container" | ||
| if legend.y is None: | ||
| # Paper-relative y so the legend top aligns with the plot top (below | ||
| # the figure title) — same vertical position Plotly uses by default. | ||
| legend_defaults["y"] = 1.0 | ||
| legend_defaults["yanchor"] = "top" | ||
| if legend_defaults: | ||
| fig.update_layout(legend=legend_defaults) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate plotly version constraint and confirm it covers >= 5.23.0.
fd -t f -e toml -e cfg -e txt | xargs rg -nP --no-messages '\bplotly\b' 2>/dev/null
echo "---"
# Also list any setup.py or requirements files
fd -t f 'pyproject\.toml|setup\.(py|cfg)|requirements.*\.txt'Repository: FBumann/xarray_plotly
Length of output: 244
🌐 Web query:
What plotly.py version first added layout.legend.xref / layout.legend.yref support?
💡 Result:
layout.legend.xref / layout.legend.yref support was first added in Plotly.py 5.15.0.[1][2] Explanation: Plotly.py’s changelog for 5.15.0 explicitly notes “Add legend.xref and legend.yref to enable container-referenced positioning of legends”.[1] The Plotly.py 5.15.0 GitHub release tag confirms the release version.[2]
Citations:
- 1: https://github.com/plotly/plotly.py/blob/main/CHANGELOG.md
- 2: https://github.com/plotly/plotly.py/releases/tag/v5.15.0
Verify the package's minimum plotly version supports legend.xref="container".
legend.xref / legend.yref were added in plotly.py 5.15.0 (CHANGELOG entry "Add legend.xref and legend.yref to enable container-referenced positioning of legends"). The project's pyproject.toml lists plotly>=5.0.0, which permits versions below 5.15.0. On those older versions, fig.update_layout(legend={"xref": "container"}) will raise a schema validation error from plotly.graph_objects, breaking every add_secondary_y(...) call.
Please bump the plotly minimum requirement to >=5.15.0 or guard the xref="container" assignment behind a version check / try-except.
🤖 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 637 - 663, The change uses
legend.xref="container" which requires plotly>=5.15.0; update
_set_default_secondary_y_layout to avoid breaking older plotly by either (A)
requiring plotly>=5.15.0 in packaging or (B) guarding the assignment: detect
plotly version (plotly.__version__) and only set
legend_defaults["xref"]="container" (and x/xanchor) when version >= "5.15.0", or
attempt to set it inside a try/except (catch plotly schema/ValueError) and skip
xref if it fails; keep the rest of the legend defaults logic intact and
reference the function name _set_default_secondary_y_layout and the
legend.xref/legend_defaults keys when making the change.
1 participant
Summary
add_secondary_ywas deduplicating legend entries across both source figures, hiding the secondary's traces whenever the two figures sharedlegendgroupnames (e.g. PXcolor=producing the same categories on both axes). Cross-source dedup is correct foroverlay(same data, two display styles) but wrong foradd_secondary_ywhere each side plots different data on its own axis.cross_source_dedupflag to_ensure_legend_visibility.overlaykeeps current behavior;add_secondary_yopts out, namespacing colliding legendgroups with the source label and deduping per-slice instead of globally.add_secondary_y, andoverlay→add_secondary_y).Test plan
pytest tests/— 147 passedadd_secondary_y(fig1, fig2)with sharedcolor=cattracesoverlaystill dedupes its own multi-trace inputs (existingtest_overlay_multi_trace_deduplicates_legendstill passes)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests