Make plotly a first-class plotting backend - #701
Conversation
Every comparison plot that used to be matplotlib-only now accepts backend="plotly", and both backends take the same arguments. - plotting/_backend.py: backend names, validation, the optional-plotly import, figsize (inches) -> plotly width/height (px), directional axes, and a modelskill-level error when a matplotlib-only argument reaches plotly's update_layout - plotting/_plotly.py: plotly renderers for timeseries, line, histogram, kde, qq, box, residual_hist and scatter (moved out of _scatter.py) - backend= added to hist, kde, qq, box and residual_hist on Comparer and ComparerCollection, and to the observation/model result timeseries and hist plots - the plotly scatter returns the figure instead of calling fig.show() - TimeSeries plotter classes collapsed into one backend-aware plotter; the plotly plotter class was the only reason for the plugin hook taylor, spatial_overview, temporal_coverage and wind_rose remain matplotlib-only and do not take a backend argument.
taylor, spatial_overview, temporal_coverage and wind_rose now take a backend argument too, so backend= is on every plot method. - taylor: single-quadrant Scatterpolar with r=std, theta=arccos(cc) and dotted centered-RMS-difference contours - spatial_overview: model domain boundary polygons plus labelled point and track observations, equal aspect - temporal_coverage: one categorical row per model/observation - wind_rose: stacked Barpolar with the calm fraction as the polar hole The domain geometry lookup in spatial_overview moved to a _model_geometry helper so both backends share it; the wind rose already kept its histogram computation separate from rendering.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Adding the backend argument dropped the return annotations; mypy accepted it because an unannotated return is Any. _backend.py now defines the two aliases the plots actually return: PlotResult (matplotlib axes or a plotly figure) and FigureResult (a matplotlib figure or a plotly figure, for taylor). Every plot method, standalone plotting function and plotly renderer is annotated again. Note that plotly ships no py.typed marker, so with ignore_missing_imports the plotly half of each union is Any to mypy; the annotation still documents the contract and will start checking if plotly adds stubs.
ANN2 (flake8-annotations) is not in this project's ruff selection, so the four helpers added with the plotly backend kept their implicit Any return.
- the plotly scatter now gets a compass axis for directional quantities; `directional` is an argument to ms.plotting.scatter() handled by both backends, so the post-hoc tick fixup in the two plotter classes is gone. A directional scatter spans 0-360 unless the user passes xlim/ylim, which is what the matplotlib backend already did - _backend.py holds the backend vocabulary only; the plotly layout interop (import_plotly_go, apply_layout, figsize_to_layout, directional_axis) moved to _plotly.py and series_range to _misc.py - RESIDUAL_COLOR moved to _misc.py: it is used by the matplotlib code path, which had no business importing from _plotly - spatial_overview classifies observations once for both backends, so the plotly backend raises on an unsupported observation type instead of silently dropping it - non-uniform bin edges raise on the plotly backend rather than being silently rendered as uniform bins; _hist_bins lost its unused argument - the plotly wind rose colors a magnitude bin by its upper edge normalized to vmax and honors n_dir_labels, as the matplotlib one does - temporal_coverage applies its (7, 0.45*n_lines) figsize default for both backends; marker is documented as matplotlib-only - the leftover Literal["matplotlib", "plotly"] annotations use Backend
The README mentioned it but the plotting user guide did not. Adds a Backends section and renders the observation timeseries and the comparer scatter with both backends, so the interactive version is on the page. plotly is now explicit in the docs dependency group; the docs build got it via the dev group by accident.
The layout helpers were unit tested by importing them from private modules, which is a boundary that should not be crossed regardless of what the import is for. Four of those tests duplicated assertions that already exist against the public plot methods, so they are gone; the two that carried real coverage are rewritten as public calls: - explicit width beating figsize -> cmp.plot.scatter(figsize=..., width=...) - the missing-plotly ImportError -> monkeypatched sys.modules plus cmp.plot.hist(backend="plotly") tests/plot/test_backend.py is removed for the same reason: all four of its tests are covered by the public equivalents in test_plotly_backend.py (invalid backend, ax rejected, compass ticks). The taylor test used the private mtr._std_mod; it now passes a local std_mod function, which is the documented way to add a metric. Also replaces the module-level type-checking imports in _backend.py with symbol imports, so the aliases read as `Axes | go.Figure`.
jpalm3r
left a comment
jpalm3r
left a comment
There was a problem hiding this comment.
I have sent claude to thoroughly review this, and then I have reviewed the comments. They are mostly fixes for edge cases but we might as well address them already.
| apply_layout( | ||
| fig, | ||
| figsize=figsize, | ||
| legend=dict(x=0.01, y=0.99), |
There was a problem hiding this comment.
Hardcoded layout keys collide with a user's kwargs and raise a bare TypeError, bypassing the nice error you built.
apply_layout is called with legend=, yaxis=, title=, yaxis_title= and xaxis_title= set explicitly and **kwargs splatted after, so any of those names arriving from the caller is a duplicate keyword. cmp.plot.qq(backend="plotly", yaxis=dict(type="log")) gives TypeError: apply_layout() got multiple values for keyword argument 'yaxis' — raised at the call site, before the try/except ValueError inside apply_layout can turn it into "Invalid plotly layout argument". The README and the new docs both invite users to pass plotly layout properties this way.
Same pattern in every renderer: lines 193, 221, 264 (barmode), 330, 375, 410 and 448 (showlegend), 769, 840, 968. Note width/height are exempt, because figsize_to_layout merges them as a dict — which is why test_plotly_layout_arguments_are_forwarded passes with width=1234 and the rest of the surface stays untested.
| for j in range(cmp.n_models): | ||
| key = cmp.mod_names[j] | ||
| mod = cmp.raw_mod_data[key]._values_as_series | ||
| mod.plot(ax=ax, color=MOD_COLORS[j], **kwargs) |
There was a problem hiding this comment.
color can no longer be overridden here, and passing it crashes.
mod.plot(ax=ax, color=MOD_COLORS[j], **kwargs) sets color explicitly while forwarding **kwargs, and timeseries() does not declare a color parameter — so cmp.plot.timeseries(color="green") raises TypeError: plot() got multiple values for keyword argument 'color'.
This is not breaking change #3 in your description. width raises because it is meaningless to Series.plot(); color is one of its most ordinary arguments. TimeSeriesPlotter.timeseries in timeseries/_plotter.py gets this right by declaring color explicitly — this method was missed.
| ylabel=ylabel, | ||
| skill_scores=skill_scores, | ||
| skill_score_unit=skill_score_unit, | ||
| directional=self.is_directional, |
There was a problem hiding this comment.
directional= collides the same way, and this PR is what makes a user reach for it.
directional=self.is_directional is injected alongside **kwargs, but directional is not a declared parameter of ComparerPlotter.scatter, so cmp.plot.scatter(directional=True) raises TypeError: scatter() got multiple values for keyword argument 'directional'. The PR newly promotes directional to a documented public parameter of ms.plotting.scatter, so trying it on the plotter is a natural mistake.
13 sites in total: this file at 119, 254, 339, 431, 537, 763, 967, and _collection_plotter.py at 283, 347, 493, 670, 758, 909.
| go = import_plotly_go() | ||
|
|
||
| data = [ | ||
| go.Scatter(x=xlim, y=xlim, name="1:1", mode="lines", line=dict(color="blue")), |
There was a problem hiding this comment.
The plotly scatter ignores options.plot.scatter.*, so the styling settings only work on one backend.
name="1:1" and color="blue" are hardcoded here, and the regression line is hardcoded "red" at line 508. The matplotlib renderer reads options.plot.scatter.oneone_line.label / .color (_scatter.py:325-326) and settings.get_option("plot.scatter.reg_line.kwargs") (line 375). A user who sets those options sees them honoured on matplotlib and silently dropped on plotly.
The file is inconsistent with itself too: the quantiles trace 60 lines down does read options.plot.scatter.quantiles.*. adr/008 documents the options system as deliberate design, so the plotly path should read the same options.
| hoverinfo="skip", | ||
| ) | ||
| ) | ||
| for name, x, y in tracks: |
There was a problem hiding this comment.
The large-track guard does not carry over to plotly.
The matplotlib path still refuses oversized tracks — if len(x) < 10000: ax.scatter(...) else: print(f"{name}: Too many points to plot") in _spatial_overview.py. Here every point of every track goes straight into the figure, so a multi-million-point altimetry track that matplotlib declines to draw is shipped whole to the browser. Satellite altimetry is a headline use case for this package, so this is reachable.
test_spatial_overview_track_observations_are_drawn_as_points asserts len(c2.x) == track.n_points, which locks the behaviour in. Either apply the same guard or downsample for plotly.
| ) | ||
|
|
||
|
|
||
| def directional_ticks( |
There was a problem hiding this comment.
This duplicates _misc._xyticks, and it contradicts the module docstring 70 lines above it.
_xyticks in _misc.py:91 is the same np.linspace(0, 360, n_sectors + 1) with the same clip to lim, and it is what the matplotlib path uses via _xtick_directional/_ytick_directional. Now the compass lives in two places in one package. directional_ticks is also called only from _plotly.py:150, while this module's own docstring says it "holds the backend vocabulary only; the plotly renderers and the plotly layout interop live in _plotly.py".
Have directional_axis call _xyticks and delete this. Which raises the broader question: with the ticks gone, _backend.py is two five-line guards, a Literal and two type aliases — all called from the same two lines at the top of every plot method. Does that earn a module, or does it belong in _misc.py next to the tick helpers it would then be using?
| RESIDUAL_COLOR = "#8B8D8E" | ||
|
|
||
|
|
||
| def series_range(series: Sequence) -> Tuple[float, float]: |
There was a problem hiding this comment.
series_range has no matplotlib callers, so it does not belong in the shared module.
Its only two callers are _plotly.py:312 and _plotly.py:352. Would it make sense moving it into _plotly.py keeps _misc.py genuinely shared?
| fig.show() | ||
| # kept as an alias: the plotter used to be selected by class, it is now | ||
| # selected by the `backend` argument on each plot method | ||
| MatplotlibTimeSeriesPlotter = TimeSeriesPlotter |
There was a problem hiding this comment.
Dead alias, and it makes an existing name mean something new.
Nothing imports MatplotlibTimeSeriesPlotter any more — _timeseries.py switched to TimeSeriesPlotter in this PR. It is a private module, so there is no compatibility argument for keeping it.
To your review note: yes, TimeSeries.plotter should go too. With the Protocol gone and exactly one possible value, it is an injection point nothing injects. Worth flagging that TimeSeriesPlotter now names a concrete class where it used to name the Protocol, so any annotation against it silently changed meaning — another reason not to leave a second name pointing at it.
| reject_matplotlib_axes, | ||
| validate_backend, | ||
| ) | ||
| from ._plotly import scatter as _scatter_plotly |
There was a problem hiding this comment.
Three different import styles for _plotly across the PR.
Top-level here and in _comparer_plotter.py:23, but lazy from . import _plotly inside the function body in _spatial_overview.py:84, _taylor_diagram.py:70, _temporal_coverage.py:91 and _wind_rose.py:272.
The lazy form is not buying anything: _plotly.py imports plotly only inside import_plotly_go, and this very line already pulls the module in at import modelskill time, so the deferred ones are deferring an import that has already happened. Pick one — top-level everywhere reads better and matches what actually occurs.
|
|
||
| @pytest.mark.parametrize("kind", PLOT_KINDS) | ||
| def test_matplotlib_axes_are_rejected_by_the_plotly_backend(cmp, kind): | ||
| _, ax = matplotlib.pyplot.subplots() |
There was a problem hiding this comment.
Leaks a figure per parametrised case.
matplotlib.pyplot.subplots() runs once for each of the seven PLOT_KINDS entries and the figure is never closed. tests/plot/test_plot.py closes figures throughout and tests/conftest.py has no autouse fixture that does it for you — the autouse fixture in this file only sets the Agg backend.
A reviewer raised exactly this on #672. plt.close(fig) at the end, or a plt.subplots() fixture that closes on teardown.
2 participants
Summary
backend="plotly"used to exist on exactly two plots (scatterandComparer.plot.timeseries), silently ignorefigsize, and blow up with plotly's internalBad property path: cmapif you passed a matplotlib argument. The plotly scatter also calledfig.show()and returnedNone.This makes plotly a peer of matplotlib rather than a special case.
Same plots.
backend=is now on every plot method:scatter,hist,kde,qq,box,residual_histandtayloron bothComparerandComparerCollection;Comparer.plot.timeseries;ComparerCollection.plot.spatial_overviewand.temporal_coverage; thetimeseries/histplots on observations and model results; and the standalone functions inms.plottingincludingwind_rose.Same arguments.
figsizeis in inches for both backends and is translated to plotly'swidth/height.axis rejected with a clear message on the plotly backend instead of being silently dropped. A matplotlib-only argument now gets a modelskill error naming the offending keyword and pointing at the plotly layout reference. Directional quantities get a 0-360 compass axis in plotly too.Same contract. Every plot returns a figure/axes; the plotly scatter no longer calls
fig.show().New
plotting/_backend.pyowns the backend vocabulary (names, validation, return types, rejectingax), andplotting/_plotly.pyowns the plotly renderers plus the plotly layout interop (the optional import, figsize translation, directional axes), so there is one place to look for plotly behaviour and_backend.pystays backend-neutral.Deliberate breaking changes
fig.show()and returnNone. Notebooks still render it as the cell result; scripts need.show().figsize(default(8, 8)) is now honored.**kwargson the matplotlibComparer.plot.timeseriesare no longer ignored -- they are forwarded toSeries.plot().cmp.plot.timeseries(width=1000)used to do nothing silently and now raises;widthis a plotly argument.ax=withbackend="plotly"now raises ontimeseriesinstead of being silently ignored (scatteralready raised)."Plotting backend: x not supported"/"backend must be one of [...]"to"Invalid backend 'x'. Valid options are: [...]".xbinscan only express uniformly spaced bins, sohist(bins=[0, 0.5, 1.0, 3.0], backend="plotly")raises rather than silently drawing uniform bins. Note also that an integerbinsis an upper bound to plotly, not an exact bin count.PlotlyTimeSeriesPlotterremoved from the privatemodelskill.timeseries._plotter. Backend is a per-call argument now, so the two plotter classes collapse into oneTimeSeriesPlotter(MatplotlibTimeSeriesPlotterkept as an alias,TimeSeries.plotterstill resolves).1-4 are all cases where the old behaviour was silently wrong. Worth a changelog line.
Docs
The plotting user guide has a new Backends section, and renders the observation timeseries and the comparer scatter with both backends so the interactive version is on the page. plotly is now explicit in the
docsdependency group (the docs build got it viadevby accident).Notes for review
taylorplotly diagram draws dotted centered-RMS-difference contours rather than matplotlib's labelled contour set; the reference point, correlation ticks and radial std axis match.spatial_overview's domain-geometry lookup moved to a_model_geometryhelper shared by both backends._reglabelmoved to_misc.reglabeland the residual grey to_misc.RESIDUAL_COLOR(both used by both backends);_scatter_plotlymoved to_plotly.scatter.directionalis now an argument toms.plotting.scatterhandled by both backends, rather than a tick fixup applied by the callers afterwards. A directional scatter spans 0-360 unlessxlim/ylimsay otherwise, which is what matplotlib already did.spatial_overviewclassifies observations once for both backends, so plotly raises on an unsupported observation type rather than dropping it silently.TimeSeries.plottershould go entirely now that nothing swaps it.testdependency group, so the missing-plotly path is covered by a unit test rather than end to end.🤖 Generated with Claude Code