Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .issueflows/03-solved-issues/issue639_original.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Issue #639: Matplotlib backend; retire SeabornPlotBuilder; unify backend=

Source: https://github.com/jepegit/cellpy/issues/639

## Original issue text

## Context

Part of epic #567 (Stage 1 — Spec pipeline for `summary_plot`). Plan of record: `architecture-plan/cellpy2-plotting-redesign-plan.md`.

## Scope

Add `cellpy/plotting/backends/mpl.py` that renders the same `FigureSpec` (seaborn used only for palette/style helpers, not as a separate backend). Delete `SeabornPlotBuilder`. Public API: `backend="plotly"|"matplotlib"` on `summary_plot`; keep `interactive=` as a `warn_once` alias registered in `DEPRECATIONS.md` (removal 2.1).

## Acceptance

- Matplotlib summary oracle cases green and describe to the same structural shape family-for-family as plotly where the snapshot already compares them.
- No `SeabornPlotBuilder` class remains.
- Calling `interactive=True/False` warns once and maps to the right backend.

## Depends on

#638

Part of epic #567.
92 changes: 92 additions & 0 deletions .issueflows/03-solved-issues/issue639_plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# Issue #639 — Plan: matplotlib backend; retire SeabornPlotBuilder; unify `backend=`

## Goal

Add `cellpy/plotting/backends/mpl.py` that renders the same summary `(frame, FigureSpec)` as plotly, delete `SeabornPlotBuilder`, and switch public `summary_plot` to `backend="plotly"|"matplotlib"` with `interactive=` as a `warn_once` alias (removal 2.1).

## Constraints

- Plan of record: [`architecture-plan/cellpy2-plotting-redesign-plan.md`](../../../architecture-plan/cellpy2-plotting-redesign-plan.md) §3.1 / Phase 2 item 4; epic [`.issueflows/05-epics/epic567_plan.md`](../05-epics/epic567_plan.md) Stage 1 issue 4.
- Depends on #638 (merged): prepare → `FigureSpec` → `PlotlyBackend.render` is the interactive path; static path still uses `SeabornPlotBuilder` on the same prepared frame ([`plotting-prepare.md`](../04-designs-and-guides/plotting-prepare.md), [`plotting-backends.md`](../04-designs-and-guides/plotting-backends.md)).
- Oracle gate: `tests/test_figure_specs.py` matplotlib summary cases stay green **without** intentional `figure_specs.json` regen unless a proven structural change is required.
- Scope is **`summary_plot` only** — do not retarget `raw_plot` / `cycles_plot` / `cycle_info_plot` `interactive=` (Stage 2).
- Not yolo — static-output engine change; seaborn-loyalist surface.

### Prior art

- `SeabornPlotBuilder` — `cellpy/utils/plotutils.py` (~1002–1856, ~850 lines, 14 methods). Owns `sns.relplot` construction, formation facet columns, axis info dicts, legend convert, line hooks. Returns `sns_fig.figure` (already a matplotlib `Figure`).
- `summary_plot` orchestration (~2190–2218) — `interactive` → `PlotlyBackend` vs `SeabornPlotBuilder`.
- `SummaryPlotConfig.interactive` / seaborn styling fields (`seaborn_palette`, `seaborn_style`, `seaborn_line_hooks`) — keep styling knobs; replace the bool switch with `backend`.
- `PlotlyBackend.render(frame, spec)` — contract to mirror; mpl reads the same `spec.extras` (`prepared_data_info`, and any mpl-needed keys we add under `render` / top-level extras).
- `tests/figure_spec_support.py` — menu uses `interactive=True/False` and `needs_seaborn=True` for matplotlib summary cases; `_describe_matplotlib` already structural.
- `cellpy._deprecation.warn_once` + `DEPRECATIONS.md` — pattern used by `summary_plot_legacy` (#596).
- Architecture policy: two backends (`plotly` | `matplotlib`); seaborn = styling inside mpl, not a third backend name.
- Toolbox / graphify: nothing relevant.

## Approach

1. **`cellpy/plotting/backends/mpl.py` — `MatplotlibBackend`**
- Implement `Backend.render(frame, spec) -> matplotlib.figure.Figure`.
- **Mechanical port** of `SeabornPlotBuilder.build_plot` (+ helpers) into this class/module: keep `sns.relplot` / `sns.set_style` / `sns.set_palette` as the faceting+styling engine so oracle structure stays stable. Seaborn is not a public backend name.
- Pull layout inputs from `spec.extras` (`prepared_data_info`, labels, row/col ids) rather than a parallel prepare path. Cell-bound bits still needed for titles/units can come from extras already populated by prepare (#638) or a small `extras['cell_name']` / capacity fields — avoid re-importing preparer logic.
- Soft-fail if seaborn missing (same warn-and-return-data behaviour as today) so optional-deps story does not regress.

2. **`get_backend(name)`**
- Add thin resolver in `cellpy/plotting/backends/__init__.py` (and optional re-export): `"plotly"` → `PlotlyBackend`, `"matplotlib"` → `MatplotlibBackend`; unknown → clear `ValueError`.

3. **Public `summary_plot` API**
- Add `backend: Optional[str] = None`.
- Change `interactive` default to `None` (sentinel). Resolution order:
1. If `interactive is not None`: `warn_once("summary_plot(interactive=...)", 'backend="plotly"|"matplotlib"', removal="2.1")` and map `True→"plotly"`, `False→"matplotlib"` when `backend` is unset; if both set and conflict, prefer `backend` after warning (document in status).
2. If `backend is None`: default `"plotly"`.
3. Validate via `get_backend`.
- Orchestration becomes: prepare → `get_backend(backend).render(frame, spec)`.
- Update docstring examples to prefer `backend=`; note `interactive=` deprecated.
- Mirror fields on `SummaryPlotConfig` (`backend`, `interactive` optional).
- Regenerate / ensure `DEPRECATIONS.md` picks up the new `warn_once` name (run `uv run python -m cellpy._deprecation` or the project’s documented regen path when the call site is live).

4. **Delete `SeabornPlotBuilder`**
- Remove the class from `plotutils.py`. No shim class. Grep tests/docs for the name and update.

5. **Tests / oracle harness**
- Point `figure_spec_support` summary matplotlib cases at `backend="matplotlib"` (and plotly at `backend="plotly"`); keep `needs_seaborn` (or rename lightly) while mpl render still imports seaborn.
- Add focused tests: `get_backend` smoke; `interactive=False/True` emits `DeprecationWarning` once and selects the right backend; `SeabornPlotBuilder` absent; oracle + `tests/test_plotutils_summary_plot.py` green (migrate call sites to `backend=` to avoid warning noise, plus 1–2 explicit interactive-deprecation cases).
- Prefer **no** `figure_specs.json` regen; if matplotlib describe shape drifts unavoidably, regen in the same commit with a status note.

6. **Design notes**
- Update [`plotting-backends.md`](../04-designs-and-guides/plotting-backends.md) and [`plotting-prepare.md`](../04-designs-and-guides/plotting-prepare.md): mpl owns static summary render; seaborn is styling-only; `backend=` is the public switch.

## Files to touch

| Path | Change |
|---|---|
| `cellpy/plotting/backends/mpl.py` | **new** — `MatplotlibBackend` (ported seaborn builder) |
| `cellpy/plotting/backends/__init__.py` | export `MatplotlibBackend`, `get_backend` |
| `cellpy/plotting/__init__.py` | light re-exports / docstring |
| `cellpy/utils/plotutils.py` | `backend=` + interactive sentinel; delete `SeabornPlotBuilder`; thin orchestrate via `get_backend` |
| `DEPRECATIONS.md` | regen row for `summary_plot(interactive=...)` |
| `tests/figure_spec_support.py` | menu kwargs → `backend=` |
| `tests/test_plotutils_summary_plot.py` | migrate to `backend=`; add deprecation cases |
| `tests/test_summary_prepare.py` / new `tests/test_mpl_backend.py` | backend selection + builder-gone |
| `.issueflows/04-designs-and-guides/plotting-*.md` | document flip |

## Test strategy

```bash
uv sync --extra batch
MPLBACKEND=Agg uv run pytest tests/test_mpl_backend.py tests/test_summary_prepare.py tests/test_plotutils_summary_plot.py tests/test_figure_specs.py -q
MPLBACKEND=Agg uv run pytest -m essential --ignore=tests/test_arbin_variants_two_stage.py
```

Parity focus: all summary families × matplotlib oracle entries; formation on/off; `interactive=` warn+map; no `SeabornPlotBuilder`.

## Open questions

1. **How much of seaborn stays inside `MatplotlibBackend`?** — **Recommend:** keep `sns.relplot` + style/palette helpers for this PR (oracle-stable “styling+facet engine”). Alternative: rewrite with plain `matplotlib` Axes immediately — higher blast radius; defer unless Accept insists.
2. **`interactive=` default sentinel** — **Recommend:** `interactive: Optional[bool] = None` so quiet default is `backend="plotly"` with no warning; only explicit `interactive=` warns. Alternative: keep `interactive=True` default and warn always — too noisy.
3. **Both `backend=` and `interactive=` passed** — **Recommend:** warn on `interactive=`, honour `backend=` if set. Alternative: error on conflict — harsher for transitional callers.
4. **Escape hatch to old builder** — **Recommend:** none (issue requires class deleted). Architecture’s `CELLPY_LEGACY_SUMMARY_PLOT` is obsolete once prepare path is the only prepare.

## Scope check

One Stage-1 slice: mpl backend module + API unify + delete seaborn builder. Fits a single PR. Follow-ups already in epic: Stage 2 ports (`cycles_plot`, etc.) reuse `backend=` / backends.
21 changes: 21 additions & 0 deletions .issueflows/03-solved-issues/issue639_status.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Issue #639 — Status

- [x] Done

## What's done

- Plan accepted (2026-07-23).
- Branch `cursor/639-mpl-backend-4efd`; draft PR #645.
- Added `cellpy/plotting/backends/mpl.py` (`MatplotlibBackend.render`).
- Added `get_backend()`; flipped `summary_plot` to `backend=` with
`interactive=` as `warn_once` alias (removal 2.1); seeded in
`_deprecation._seed_known_deprecations` + `DEPRECATIONS.md`.
- Deleted `SeabornPlotBuilder`.
- Migrated oracle/summary tests to `backend=`; added `tests/test_mpl_backend.py`.
- Design notes updated (`plotting-backends.md`, `plotting-prepare.md`).
- Verified green on close: mpl + figure-specs (61); essential (556).
- HISTORY Unreleased bullet added; issue docs archived to `03-solved-issues`.

## Remaining work

- None (post-merge: `/iflow-cleanup`).
27 changes: 11 additions & 16 deletions .issueflows/04-designs-and-guides/plotting-backends.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,23 @@

## Context

`PlotlyPlotBuilder` had four nearly-copied
`_configure_formation_{1,2,3,4}_rows` methods (~350 lines) that set facet
axis domains/matches/ranges for formation figures. Epic #567 Stage 1 needs
one layout engine and a real prepare→spec→render path for `summary_plot`.
Epic #567 Stage 1 needs one layout engine and prepare→spec→render for
`summary_plot`, with two public backends (`plotly` | `matplotlib`).

## Decision

- **Formation layout lives in** `cellpy/plotting/backends/plotly.py`
(`configure_formation_layout`).
- **Per-row-count methods are deleted.** Specials stay as parameters/helpers:
N=1 annotation y; optional `top_row_label` domains;
`configure_fullcell_standard_domains` for fullcell 4-row titles.
- **`Backend` protocol** in `backends/base.py`; public interactive
`summary_plot` calls `PlotlyBackend.render(frame, spec)` (#638).
- **No-formation path** also lives on `PlotlyBackend` (layout knobs come from
`FigureSpec.extras['render']` produced by prepare).
- **`PlotlyPlotBuilder` deleted.** `CELLPY_SUMMARY_PLOTLY_SPEC` provisional
dual-path removed — single interactive path.
- **`SeabornPlotBuilder`** remains until #639; it consumes the same prepared
frame / `prepared_data_info` from prepare.
- **`Backend` protocol** in `backends/base.py`; `get_backend(name)` resolves
`"plotly"` / `"matplotlib"`.
- **Interactive path:** `PlotlyBackend.render(frame, spec)` (#638).
- **Static path:** `MatplotlibBackend.render(frame, spec)` (#639). Seaborn is
used only for palette/style/faceting helpers (`relplot`); it is **not** a
public backend name. `SeabornPlotBuilder` is deleted.
- **Public switch:** `summary_plot(..., backend="plotly"|"matplotlib")`.
`interactive=` is a `warn_once` alias (removal 2.1).

## Links

- Issues #638, #637, #636; epic #567
- Issues #639, #638, #637, #636; epic #567
- Plan of record: `architecture-plan/cellpy2-plotting-redesign-plan.md` §3.1
17 changes: 9 additions & 8 deletions .issueflows/04-designs-and-guides/plotting-prepare.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,25 @@
## Context

`SummaryPlotDataPreparer` lived in `cellpy/utils/plotutils.py` and fed both
`PlotlyPlotBuilder` and `SeabornPlotBuilder`. Epic #567 Stage 1 needs
prepare → `FigureSpec` → backend.render as the only summary path.
plotly and seaborn builders. Epic #567 Stage 1 needs prepare → `FigureSpec` →
backend.render as the only summary path.

## Decision

- **Prepare lives in** `cellpy/plotting/prepare/summary.py`.
`prepare(ctx, family, config) -> (frame, FigureSpec)`.
- **`SummaryPlotDataPreparer`** moved there (implementation detail); deleted
from `plotutils`.
- **`FigureSpec.extras`** carries `prepared_data_info` (seaborn bridge) and
`render` (plotly knobs including precomputed formation / no-formation
layout). First-class panel/axis fields grow as later issues need them.
- **`FigureSpec.extras`** carries `prepared_data_info` and `render` (plotly
knobs including precomputed formation / no-formation layout). For the
matplotlib backend, `summary_plot` also attaches live `config` / `cell` on
`extras` so seaborn styling knobs remain available without a second prepare.
- **`CellContext`** in `cellpy/plotting/context.py` is the thin cell adapter;
BatchContext waits for collectors rebase.
- Public `summary_plot` stays in `plotutils` but only orchestrates
context → registry → prepare → render.
- Public `summary_plot` stays in `plotutils` and orchestrates
context → registry → prepare → `get_backend(backend).render`.

## Links

- Issue #638; epic #567
- Issues #639, #638; epic #567
- Related: `plotting-registry.md`, `plotting-backends.md`
1 change: 1 addition & 0 deletions DEPRECATIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ uv run python -m cellpy._deprecation
| `legacy header attribute access (headers_normal / _summary / _step_table)` | `c.schema.raw / c.schema.steps / c.schema.summary` | 2.0 | 2.1 |
| `make_new_cell` | `CellpyCell.vacant` | 2.0 | 2.1 |
| `plotutils.summary_plot_legacy` | `cellpy.utils.plotutils.summary_plot (same figures, same options)` | 2.0 | 2.1 |
| `summary_plot(interactive=...)` | `backend="plotly"|"matplotlib"` | 2.0 | 2.1 |
| `the 'dq' column of the ica output frame` | `the 'dqdv' column of the same frame` | 2.0 | 2.1 |
2 changes: 2 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## [Unreleased]

* Matplotlib backend; retire SeabornPlotBuilder; unify backend= (#639).

* Port summary prepare path and flip summary_plot to prepare→spec→render (#638).

* Generic plotly panel/formation layout backend (#637).
Expand Down
6 changes: 6 additions & 0 deletions cellpy/_deprecation.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ def _seed_known_deprecations() -> None:
"cellpy.utils.plotutils.summary_plot (same figures, same options)",
removal="2.1",
)
# Stage 1 (#639): interactive= is a warn_once alias for backend=.
_register(
"summary_plot(interactive=...)",
'backend="plotly"|"matplotlib"',
removal="2.1",
)


if __name__ == "__main__":
Expand Down
12 changes: 9 additions & 3 deletions cellpy/plotting/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@

The old locations re-export from here, so nothing that imported them breaks.
Public ``summary_plot`` still lives in ``plotutils`` but runs
prepare → spec → ``PlotlyBackend.render`` for the interactive path (#638);
``SeabornPlotBuilder`` remains until #639.
prepare → spec → ``get_backend(...).render`` (#638 / #639).
"""

from __future__ import annotations

from cellpy.plotting.backends import Backend, PlotlyBackend
from cellpy.plotting.backends import (
Backend,
MatplotlibBackend,
PlotlyBackend,
get_backend,
)
from cellpy.plotting.context import CellContext, from_source
from cellpy.plotting.figures import (
load_figure,
Expand All @@ -50,12 +54,14 @@
"Backend",
"CellContext",
"FigureSpec",
"MatplotlibBackend",
"PanelSpec",
"PlotFamily",
"PlotlyBackend",
"_register_family",
"families",
"from_source",
"get_backend",
"get_family",
"legend_replacer",
"load_figure",
Expand Down
19 changes: 18 additions & 1 deletion cellpy/plotting/backends/__init__.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
"""Plotting backends (prepare → spec → render) — epic #567 / #637–#638."""
"""Plotting backends (prepare → spec → render) — epic #567 / #637–#639."""

from __future__ import annotations

from typing import Any

from cellpy.plotting.backends.base import Backend
from cellpy.plotting.backends.mpl import MatplotlibBackend
from cellpy.plotting.backends.plotly import (
PlotlyBackend,
configure_formation_layout,
Expand All @@ -11,7 +14,21 @@

__all__ = [
"Backend",
"MatplotlibBackend",
"PlotlyBackend",
"configure_formation_layout",
"configure_fullcell_standard_domains",
"get_backend",
]


def get_backend(name: str) -> Any:
"""Return a backend instance for *name* (``plotly`` or ``matplotlib``)."""
key = (name or "").strip().lower()
if key == "plotly":
return PlotlyBackend()
if key == "matplotlib":
return MatplotlibBackend()
raise ValueError(
f"unknown plotting backend {name!r} (known: plotly, matplotlib)"
)
Loading
Loading