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/issue638_original.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Issue #638: Port summary prepare path and flip summary_plot to prepare→spec→render

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

## 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/prepare/summary.py` by extracting/reusing `SummaryPlotDataPreparer` (filters, rate rescaling, normalization, formation marking, CV partitioning → tidy long frame + `FigureSpec`). Point public `summary_plot` at context adapter → registry → prepare → backend.render. Preserve the user-facing signature and defaults (including named `y=` strings and `return_data=True` frame shape).

## Acceptance

- Figure-spec oracle green for all summary families × both backends currently covered.
- `return_data` frame columns/dtypes match today's builder output on the oracle cell.
- `SummaryPlotDataPreparer` / `PlotlyPlotBuilder` live code either becomes the prepare/backend implementation or is deleted in this PR — no third parallel summary path left.

## Depends on

#637

Part of epic #567.
104 changes: 104 additions & 0 deletions .issueflows/03-solved-issues/issue638_plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Issue #638 — Plan: port summary prepare path; flip `summary_plot` to prepare→spec→render

## Goal

Make public `summary_plot` run **context → registry → prepare → backend.render** for the interactive (plotly) path: extract prepare into `cellpy/plotting/prepare/summary.py` (emitting a tidy frame + real `FigureSpec`), absorb `PlotlyPlotBuilder` into `PlotlyBackend.render`, and leave no third parallel summary prepare/render path. Preserve signature, defaults, and `return_data` frame shape.

## Constraints

- Plan of record: [`architecture-plan/cellpy2-plotting-redesign-plan.md`](../../../architecture-plan/cellpy2-plotting-redesign-plan.md) §3.1 / Phase 2 items 2–3; epic [`.issueflows/05-epics/epic567_plan.md`](../05-epics/epic567_plan.md) Stage 1 issue 3.
- Depends on #636/#637 (merged): registry + `FigureSpec` exist; formation layout lives in `backends/plotly.py`; `PlotlyBackend.render` is provisional/incomplete; public path still uses `SummaryPlotDataPreparer` + `PlotlyPlotBuilder` / `SeabornPlotBuilder`.
- Oracle gate: `tests/test_figure_specs.py` green for all summary families × both backends currently covered (**no** intentional `figure_specs.json` regen unless a proven intentional visual change).
- Do **not** add `backends/mpl.py` or delete `SeabornPlotBuilder` (that is #639). Do **not** port collectors / batch faceting.
- User-facing `summary_plot` signature and defaults stay verbatim (including named `y=` and `return_data=True`).
- Not yolo — flag-day for the main plot entry; needs careful parity.

### Prior art

- `SummaryPlotDataPreparer` — `cellpy/utils/plotutils.py` (~1008–1420). Filters, rate rescale, normalization, formation marking, CV / fullcell / standard melts → tidy frame + metadata dict (`data`, `number_of_rows`, labels, cycle bounds, `formation_cycle_selector`).
- `PlotlyPlotBuilder.build_plot` — same module (~1429–1853). Owns `px.line` construction, hover/facet kwargs, formation adapter → `configure_formation_layout`, **no-formation** layout (`_configure_no_formation_axes`), legend prettify, rangeslider / share_y. This is what must land inside `PlotlyBackend.render` (or helpers it calls).
- `SeabornPlotBuilder` — same module (~1856+). Stays until #639; must consume the **same** prepared frame from the new prepare module.
- `PlotlyBackend.render` — `cellpy/plotting/backends/plotly.py`. Minimal `px.line` + formation via `extras`; missing no-formation, legend convert, height defaults, hover, fullcell post-steps wiring from config, etc. `CELLPY_SUMMARY_PLOTLY_SPEC` / `use_spec_render()` exist but are **not** wired into `summary_plot` today.
- `SummaryPlotInfo` + registry — column sets / labels already registry-backed (#636); prepare keeps using them (or equivalent) rather than reinventing column tables.
- Architecture sketch: `context.from_source` → `registry.get` → `prepare.summary.prepare` → `backends.get(backend).render` — no `context.py` yet.
- Toolbox / graphify: nothing relevant.

## Approach

1. **Minimal context adapter** — add `cellpy/plotting/context.py` with a thin `CellContext` (or `from_source(c)`) that exposes what prepare/render need from a `CellpyCell` (`data.summary`, headers/schema, `make_summary`, `cell_name`, units helpers). BatchContext is out of scope. `summary_plot` builds context once and passes it into prepare (and into plotly render only where cell-bound labels still need it, e.g. capacity units / title).

2. **`cellpy/plotting/prepare/summary.py`**
- Move `SummaryPlotDataPreparer` here ( mechanistically; keep private helpers). Public function shape aligned with the plan of record:

```python
def prepare(ctx, family, config) -> tuple[pd.DataFrame, FigureSpec]:
...
```

- Still produce today’s tidy long frame (same columns / dtypes / row marks for formation and panel `row`).
- Build a **real** `FigureSpec`: panels from family/row count; `x_axis` / per-panel `y_axis.range` from config (`y_range`, `ce_range`, `norm_range`, `cv_share_range`); `supports_formation` from family + `config.show_formation`; put render knobs that are not yet first-class fields into `spec.extras` (x column name, `y_header`, plotly facet keys, formation domains/ranges once computed, fullcell domain kwargs, height/markers/template flags, etc.). Prefer computing formation domain/range numbers in prepare (or a small shared helper) so `render` stays declarative.
- Delete the class from `plotutils` (or leave a thin re-export alias only if something external imports it — prefer delete; nothing in tests should import the class name).

3. **Complete `PlotlyBackend.render(frame, spec)`**
- Port the body of `PlotlyPlotBuilder.build_plot` (including **no-formation** path and legend conversion) to operate from `(frame, FigureSpec)` + whatever tiny cell-bound bits remain on context/extras.
- Formation continues to call `configure_formation_layout` / `configure_fullcell_standard_domains`.
- Delete `PlotlyPlotBuilder` from the live path (class removed or reduced to a one-liner deprecated shim that calls the backend — prefer remove).
- Remove the provisional `CELLPY_SUMMARY_PLOTLY_SPEC` dual-path concept (flag unused today): interactive path **always** uses `PlotlyBackend.render`. No third path.

4. **Flip `summary_plot`**
```text
config → resolve columns → CellContext
→ registry.get(y) / SummaryPlotInfo as needed
→ prepare.summary.prepare(ctx, family, config) → frame, spec
→ if interactive: PlotlyBackend().render(frame, spec)
else: SeabornPlotBuilder().build_plot(frame, prepared_meta_or_spec, …)
→ return_data ? (fig, frame) : fig
```
- `interactive=False` keeps `SeabornPlotBuilder` until #639, but it must use the **same** `frame` from `prepare/summary.py` (adapt its `prepared_data_info` needs from `FigureSpec` / a small metadata side-channel if required — do not re-run preparer logic).
- Keep the public function in `cellpy.utils.plotutils` for this PR (permanent re-export home already decided); no API move to `cellpy.plotting.summary_plot` required here.

5. **Package exports**
- `cellpy/plotting/prepare/__init__.py` (+ optional `summary` re-export).
- Light updates to `cellpy/plotting/__init__.py` / `backends` docs; update [`.issueflows/04-designs-and-guides/plotting-backends.md`](../04-designs-and-guides/plotting-backends.md) and add a short prepare note (or extend registry/backends docs) recording the flip.

6. **Parity / tests**
- Oracle: all summary families × plotly + seaborn paths green.
- Add focused tests: prepare returns `(frame, FigureSpec)` with expected panel count / extras keys for 1–2 representative families; `return_data` columns match pre-flip shape on the oracle cell; asserting `PlotlyPlotBuilder` is gone (or not imported by `summary_plot`).
- Existing `tests/test_plotutils_summary_plot.py` stays green.

## Files to touch

| Path | Change |
|---|---|
| `cellpy/plotting/context.py` | **new** — minimal `CellContext` / `from_source` |
| `cellpy/plotting/prepare/__init__.py` | **new** |
| `cellpy/plotting/prepare/summary.py` | **new** — port of `SummaryPlotDataPreparer` + `FigureSpec` emission |
| `cellpy/plotting/backends/plotly.py` | complete `render`; drop unused spec-render env gate; own no-formation layout |
| `cellpy/plotting/backends/__init__.py` | drop/adjust `use_spec_render` exports if removed |
| `cellpy/plotting/spec.py` | only if documented `extras` keys or small field tweaks needed |
| `cellpy/utils/plotutils.py` | thin `summary_plot` orchestration; delete preparer + `PlotlyPlotBuilder`; keep `SeabornPlotBuilder` + helpers still shared |
| `cellpy/plotting/__init__.py` | optional exports |
| `tests/test_summary_prepare.py` (name flexible) | **new** — prepare + return_data / spec shape |
| `tests/test_figure_specs.py` / `test_plotutils_summary_plot.py` / `test_plotly_backend_layout.py` | run; adjust imports if builders move |
| `.issueflows/04-designs-and-guides/plotting-backends.md` (+ prepare note) | document the flip |

## Test strategy

```bash
uv sync --extra batch
MPLBACKEND=Agg uv run pytest tests/test_summary_prepare.py tests/test_plotly_backend_layout.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: formation on/off, CE / with_rate / CV-split / fullcell families, `return_data` columns/dtypes on the oracle cell, both `interactive=True/False`.

## Open questions

1. **How thin is `context.py`?** — **Recommend:** minimal `CellContext` wrapping one `CellpyCell` (attributes/methods prepare already needs). No BatchContext. Alternative: skip new module and pass `c` into prepare until collectors stage — weaker vs issue text (“context adapter”).
2. **`SeabornPlotBuilder` this PR?** — **Recommend:** keep as the only `interactive=False` renderer, fed by the new prepare output; delete only `PlotlyPlotBuilder` + old preparer. Meets “no third path” (one prepare, plotly via backend, seaborn temporary). Alternative: also stub mpl backend early — rejected (belongs to #639).
3. **Escape-hatch env flag?** — **Recommend:** no dual path after flip (remove `CELLPY_SUMMARY_PLOTLY_SPEC`). Oracle + existing unit tests are the safety net; a legacy flag would reintroduce the third path the issue forbids. Alternative: keep `CELLPY_LEGACY_SUMMARY_PLOTLY=1` for one release — only if Accept asks for it.
4. **Where formation domain/range math lives** — **Recommend:** compute in prepare (or a shared helper used by prepare) and stash on `FigureSpec.extras`, so `render` applies layout without re-reading the frame for cycle bounds. Alternative: keep that math inside `PlotlyBackend` (closer to today’s builder) — acceptable if it unblocks parity faster; still one path.

## Scope check

Single Stage-1 flag-day PR: prepare module + context stub + complete plotly render + `summary_plot` flip + delete preparer/`PlotlyPlotBuilder`. Fits one PR. Follow-up already published: #639 (mpl backend; retire seaborn builder).
22 changes: 22 additions & 0 deletions .issueflows/03-solved-issues/issue638_status.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Issue #638 — Status

- [x] Done

## What's done

- Plan accepted (2026-07-23).
- Branch `cursor/638-summary-prepare-render-4efd`; draft PR #644.
- Added `cellpy/plotting/context.py` (`CellContext` / `from_source`).
- Ported preparer to `cellpy/plotting/prepare/summary.py`; `prepare()` returns `(frame, FigureSpec)` with formation/no-formation knobs in `extras['render']`.
- Completed `PlotlyBackend.render`; deleted `PlotlyPlotBuilder` + `CELLPY_SUMMARY_PLOTLY_SPEC` dual-path.
- Flipped public `summary_plot` to context → registry → prepare → render; `SeabornPlotBuilder` kept on the same frame until #639.
- Design notes: `plotting-prepare.md`; updated backends/registry docs.
- Tests: `tests/test_summary_prepare.py`; layout test asserts builders gone.
- Verified green on close:
- prepare + layout + figure-specs (67)
- `pytest -m essential --ignore=tests/test_arbin_variants_two_stage.py` (551)
- HISTORY Unreleased bullet added; issue docs archived to `03-solved-issues`.

## Remaining work

- None (post-merge: `/iflow-cleanup`).
24 changes: 13 additions & 11 deletions .issueflows/04-designs-and-guides/plotting-backends.md
Original file line number Diff line number Diff line change
@@ -1,27 +1,29 @@
# Plotting backends (formation layout)
# Plotting backends (formation layout + render)

## 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 before prepare→spec→render (#638).
one layout engine and a real prepare→spec→render path for `summary_plot`.

## Decision

- **Formation layout lives in** `cellpy/plotting/backends/plotly.py`
(`configure_formation_layout`). `PlotlyPlotBuilder._configure_formation_axes`
is a thin adapter that computes domains/ranges and calls it.
(`configure_formation_layout`).
- **Per-row-count methods are deleted.** Specials stay as parameters/helpers:
N=1 annotation y; optional `top_row_label` domains;
N=1 annotation y; optional `top_row_label` domains;
`configure_fullcell_standard_domains` for fullcell 4-row titles.
- **`Backend` protocol** in `backends/base.py`; `PlotlyBackend.render` exists
for #638. Public `summary_plot` does **not** flip yet.
- **Provisional full-render flag:** env `CELLPY_SUMMARY_PLOTLY_SPEC=1`
(default off). Layout engine itself is always on.
- **No-formation path** stays on `PlotlyPlotBuilder` until #638.
- **`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.

## Links

- Issues #637, #636; epic #567
- Issues #638, #637, #636; epic #567
- Plan of record: `architecture-plan/cellpy2-plotting-redesign-plan.md` §3.1
26 changes: 26 additions & 0 deletions .issueflows/04-designs-and-guides/plotting-prepare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Plotting prepare (summary)

## 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.

## 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.
- **`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.

## Links

- Issue #638; epic #567
- Related: `plotting-registry.md`, `plotting-backends.md`
5 changes: 3 additions & 2 deletions .issueflows/04-designs-and-guides/plotting-registry.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ moves selection into `cellpy.plotting.registry`.
- **Unknown `y` fails loudly** (`ValueError` listing known names). Undocumented
raw-column fallthrough (`y_cols.get(y, y)`) is gone; extension is
`_register_family` (provisional in 2.0).
- **`FigureSpec` / `PanelSpec` / `AxisSpec`** land in `cellpy/plotting/spec.py`
but are not consumed by builders until Stage-1 prepare→spec→render (#637–#639).
- **`FigureSpec` / `PanelSpec` / `AxisSpec`** live in `cellpy/plotting/spec.py`.
Prepare emits a real `FigureSpec` for `summary_plot` (#638); matplotlib
backend consumption is #639.
- **Oracle menu** (`tests/figure_spec_support.SUMMARY_FAMILIES`) derives from
`families()` so the snapshot menu cannot drift from the runtime menu.

Expand Down
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]

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

* Generic plotly panel/formation layout backend (#637).

* Add FigureSpec dataclasses and a PlotFamily registry (#636).
Expand Down
13 changes: 10 additions & 3 deletions cellpy/plotting/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,19 @@
| [`spec`][cellpy.plotting.spec] | ``FigureSpec`` / ``PanelSpec`` / ``AxisSpec`` |
| [`registry`][cellpy.plotting.registry] | named ``PlotFamily`` records for ``summary_plot`` |
| [`backends`][cellpy.plotting.backends] | render protocol + plotly formation layout (#637) |
| [`prepare`][cellpy.plotting.prepare] | summary tidy-frame + ``FigureSpec`` (#638) |
| [`context`][cellpy.plotting.context] | ``CellContext`` adapter (#638) |

The old locations re-export from here, so nothing that imported them breaks.
The drawing code itself — ``summary_plot`` and friends — still lives in
``plotutils``; Stage 1 of epic #567 moves selection behind the registry and
lands the spec/backends scaffolding for prepare→spec→render (#638).
Public ``summary_plot`` still lives in ``plotutils`` but runs
prepare → spec → ``PlotlyBackend.render`` for the interactive path (#638);
``SeabornPlotBuilder`` remains until #639.
"""

from __future__ import annotations

from cellpy.plotting.backends import Backend, PlotlyBackend
from cellpy.plotting.context import CellContext, from_source
from cellpy.plotting.figures import (
load_figure,
load_matplotlib_figure,
Expand All @@ -32,6 +35,7 @@
save_matplotlib_figure,
)
from cellpy.plotting.labels import legend_replacer, remove_markers
from cellpy.plotting.prepare import prepare_summary
from cellpy.plotting.registry import (
PlotFamily,
_register_family,
Expand All @@ -44,19 +48,22 @@
__all__ = [
"AxisSpec",
"Backend",
"CellContext",
"FigureSpec",
"PanelSpec",
"PlotFamily",
"PlotlyBackend",
"_register_family",
"families",
"from_source",
"get_family",
"legend_replacer",
"load_figure",
"load_matplotlib_figure",
"load_plotly_figure",
"make_matplotlib_manager",
"make_plotly_template",
"prepare_summary",
"remove_markers",
"save_matplotlib_figure",
]
4 changes: 1 addition & 3 deletions cellpy/plotting/backends/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Plotting backends (prepare → spec → render) — epic #567 / #637."""
"""Plotting backends (prepare → spec → render) — epic #567 / #637–#638."""

from __future__ import annotations

Expand All @@ -7,13 +7,11 @@
PlotlyBackend,
configure_formation_layout,
configure_fullcell_standard_domains,
use_spec_render,
)

__all__ = [
"Backend",
"PlotlyBackend",
"configure_formation_layout",
"configure_fullcell_standard_domains",
"use_spec_render",
]
Loading
Loading