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
26 changes: 26 additions & 0 deletions .issueflows/03-solved-issues/issue657_original.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Issue #657: Re-base collectors' drawing half onto `cellpy.plotting`

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

## Original issue text

## Context

Part of epic #567 (Stage 3 — Collectors drawing half, Batch.plot, retire batch_plotters). Plan of record: `architecture-plan/cellpy2-plotting-redesign-plan.md` (plus batch redesign §4.7 and collectors redesign §3.3 for the hand-offs). Collection, caching, autonaming, and frame persistence stay in collectors (out of scope to redesign).

## Scope

Delete collectors' plotter implementations (`sequence_plotter`, `summary_plotter`, `cycles_plotter`, `spread_plot` drawing bodies) and local templates; `BatchCollector.plot` / render path calls `cellpy.plotting.*` with the already-collected tidy frame (`cell`/`group`/`sub_group` columns). Fold fig-per-cell / fig-per-cycle / film / spread capabilities into plotting options (`layout=`, `kind=`) as needed for parity.

## Acceptance

- Collector-driven figures that the maintainers' docs/tutorials exercise match the oracle (add collector/batch-input cases to `figure_specs.json` as needed).
- Collectors no longer define figure IO, legend helpers, or plotly templates locally.
- `import cellpy.utils.collectors` stays safe without the `batch` extra for non-plot imports.

## Depends on

#647

Part of epic #567.

192 changes: 192 additions & 0 deletions .issueflows/03-solved-issues/issue657_plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
# Issue #657 — Plan

## Goal

Re-base collectors' drawing half onto `cellpy.plotting`: already-collected tidy
frames (`cell` / `group` / `sub_group`) render through prepare→spec→render with
`layout=` / `kind=` options; delete local plotter bodies; keep collection,
caching, autonaming, and frame persistence in collectors.

## Constraints

- Plan of record: `architecture-plan/cellpy2-plotting-redesign-plan.md` §3.3 /
Phase 4; collectors hand-off in
`architecture-plan/cellpy2-collectors-redesign-plan.md` §3.3.
- Collection redesign (options dataclasses, elevated-args collapse, recipes) is
**out of scope** — leave `data_collector` / `plotter_arguments` wiring; only
swap the draw callee.
- `#658` (`Batch.plot` + delete `batch_plotters`) depends on this landing; do
not touch `batch_plotters.py` here.
- Filters stay in `cellpy/filters/`
(`.issueflows/04-designs-and-guides/filters-and-plot-filtering.md`).
- Parity gate: keep `tests/test_figure_specs.py` green; extend
`figure_specs.json` with collector/batch-input cases in the same commit as
intentional visual changes. Plotting tests under `MPLBACKEND=Agg`.
- `import cellpy.utils.collectors` must stay safe without the `batch` extra for
**non-plot** imports (no import-time plotly attribute use; defer heavy plot
deps).
- Public collector UX (`Batch*Collector`, `render` / `show` / `update`,
`plot_type` → `method`) stays working; notebooks must not break.

### Prior art

- `BatchCollector.render` → `self.plotter(self.data, …)` —
[`cellpy/utils/collectors.py`](cellpy/utils/collectors.py) (~L421); plotters
`sequence_plotter` / `summary_plotter` / `cycles_plotter` / `ica_plotter` /
`spread_plot` (~L1765–2912). **Migrate** draw bodies out; keep collect path.
- `CellContext` / `from_source` —
[`cellpy/plotting/context.py`](cellpy/plotting/context.py); docstring already
reserves Batch/frame adapters for this issue. **Extend**.
- Prepare → `FigureSpec` → `get_backend().render` —
[`cellpy/plotting/prepare/`](cellpy/plotting/prepare/),
[`backends/`](cellpy/plotting/backends/); design notes in
`.issueflows/04-designs-and-guides/plotting-prepare.md` +
`plotting-backends.md`. **Mirror** for collected frames.
- Collector plotly templates already in
[`cellpy/plotting/theme.py`](cellpy/plotting/theme.py)
(`make_collector_templates`, names `fig_pr_cell` / `fig_pr_cycle` / `film` /
`summary`). **Reuse**; stop treating collectors as template owner.
- Figure IO / legend helpers already in `plotting.figures` / `plotting.labels`
(Phase 1 / #595); collectors re-export. **Keep** re-exports or thin imports;
delete any remaining local exporter duplication that is purely drawing.
- Oracle:
[`tests/test_figure_specs.py`](tests/test_figure_specs.py) +
[`tests/figure_spec_support.py`](tests/figure_spec_support.py) — single-cell
only today. **Extend** with collector/frame cases.
- Smoke:
[`tests/test_collectors.py`](tests/test_collectors.py) — E2E autorun for
summary / cycles / ICA (+ film). **Keep green**.
- Toolbox (`00-tools/`): nothing that builds collector figures. Graph communities
35/58/163 (collectors) and plotting prepare/backends hubs.

## Approach

**Recommended: frame-in adapter + multi-cell render in `cellpy.plotting`, then
delete collectors plotters.** Do not leave `sequence_plotter` as a permanent
public API under a new path; fold behaviour into backends/`layout`/`kind`.

### 1. Frame context

- Add `FrameContext` (name flexible) in `context.py`: holds the tidy
`DataFrame` plus optional `units` / `journal` / collector `kind`
(`summary` | `cycles` | `ica`).
- Extend `from_source` (or add `from_frame`) so a collected frame is a first-class
plotting input without requiring a live `CellpyCell` per subplot.
- Do **not** implement full `Batch` → collect inside plotting; collectors keep
owning collection.

### 2. Collected-frame plot API in plotting

- Add one orchestrator used by collectors, e.g.
`cellpy.plotting.collected_plot(frame, *, family_kind, layout=…, kind=…,
backend=…, **opts)` (exact name bikeshed OK; keep it package-private or
public-but-narrow).
- Map today's collector knobs:
- `method` / `plot_type`: `fig_pr_cell` → `layout="per_cell"`;
`fig_pr_cycle` → `layout="per_cycle"`; `film` → `kind="film"` (+ layout);
`summary` → summary collected path.
- `spread=True` → `kind="spread"`.
- Default line plots → `kind="line"`.
- Build a `FigureSpec` (panels / extras carrying layout+kind+labels) from the
already-tidy frame; skip single-cell prepare that expects `CellContext`.
- Render via existing `get_backend(backend)` with new multi-cell branches in
plotly (primary). Port the behavioural core of `sequence_plotter` /
`spread_plot` / summary melt path into those branches (mechanical move +
rename to `layout`/`kind`, not a redesign of collection).

### 3. Wire collectors

- `BatchCollector.render` (and optionally `plot` as a thin alias for redesign
§3.3 naming) calls the plotting orchestrator with `self.data` + mapped
options; drop `self.plotter` callable requirement **or** replace subclass
plotter defaults with a sentinel that means “use plotting”.
- Subclasses keep elevated `plot_type` / collector args; only the draw target
changes.
- Figure save / PNG / SVG paths use `plotting.figures` (delete duplicate local
plotly exporters if still present as bodies).
- Templates: call `theme.make_collector_templates()` from plotting/orchestrator
(or once on first collected render); collectors stop owning template setup
beyond a no-op or re-export.

### 4. Delete drawing bodies

Remove from `collectors.py` (or reduce to deprecated shims that forward one
release if something external imported them — prefer delete if only used
internally):

- `sequence_plotter`, `_cycles_plotter`, `summary_plotter`, `cycles_plotter`,
`ica_plotter`, `spread_plot`, film helpers that exist only for drawing
(`histogram_equalization` / `_hist_eq` as needed).
- Keep: collectors, `pick_named_cell`, `*_collector` frame builders,
persistence, autoname, units parsing.

### 5. Import hygiene

- Keep plotly import guarded; ensure no import-time `go`/`px`/`pio` use outside
functions (existing `test_plotting_package` AST guard).
- Soften hard `matplotlib` import at collectors module top if it blocks
non-plot import paths; match the plotly pattern where practical.

### 6. Docs / deprecations

- If any public plotter name was importable from `cellpy.utils.collectors`,
register a `warn_once` shim in `DEPRECATIONS.md` pointing at the plotting
API — only if grep shows external/doc usage; otherwise delete cleanly.
- Short note in
`.issueflows/04-designs-and-guides/plotting-prepare.md` (or new
`plotting-collected.md`): FrameContext + `layout`/`kind` contract.

### Ordering inside the PR

1. FrameContext + collected orchestrator + plotly multi-cell render (dual-path
behind collectors still calling old plotters until green).
2. Flip `BatchCollector.render` to the new path; run collector + oracle tests.
3. Delete old plotter bodies; regenerate `figure_specs.json` for new cases.
4. Import hygiene + design-doc touch-up.

## Files to touch

| Path | Change |
|------|--------|
| `cellpy/plotting/context.py` | `FrameContext` (+ `from_frame` / extend `from_source`) |
| `cellpy/plotting/` (new module, e.g. `collected.py` or `prepare/collected.py`) | Orchestrator: frame → FigureSpec → backend |
| `cellpy/plotting/backends/plotly.py` | Multi-cell `layout` / `kind` render branches |
| `cellpy/plotting/backends/mpl.py` | Parity only where collectors already support seaborn/mpl today; else document plotly-primary for collected layouts |
| `cellpy/plotting/theme.py` | Ensure collector templates registered from plotting side |
| `cellpy/plotting/__init__.py` | Export new public/narrow API as needed |
| `cellpy/utils/collectors.py` | `render`/`plot` → plotting; delete plotter bodies; import hygiene |
| `tests/figure_spec_support.py` + `tests/data/figure_specs.json` | Collector/batch-input cases + snapshot |
| `tests/test_figure_specs.py` / `dev/snapshot_figure_specs.py` | Wire new cases if needed |
| `tests/test_collectors.py` | Stay green; adjust imports if plotters vanish |
| `tests/test_plotting_package.py` | Re-exports / no import-time plotly |
| `DEPRECATIONS.md` | Only if public shims needed |
| `.issueflows/04-designs-and-guides/plotting-*.md` | Record FrameContext / layout/kind decision |

## Test strategy

```bash
MPLBACKEND=Agg uv run pytest tests/test_collectors.py tests/test_plotting_package.py tests/test_figure_specs.py -q
MPLBACKEND=Agg uv run pytest -m essential -q
```

- Extend oracle with a **minimum** collector column (recommend): summary default
(plotly), cycles `per_cell` (plotly), ICA `film` (plotly), and one `spread`
summary if `standard_gravimetric_collector` / spread path is still public.
- Keep existing `test_collectors.py` E2E autorun assertions.
- Regenerate snapshot via `uv run python dev/snapshot_figure_specs.py` when
structure intentionally changes.

## Decisions (confirmed)

1. **Backend parity:** plotly-complete for all `layout`/`kind` modes;
`backend="seaborn"` → matplotlib backend with best-effort parity (or
`warn_once` + plotly fallback if parity is incomplete).
2. **Frame-in API:** narrow public `cellpy.plotting.collected_plot` (name OK to
adjust slightly) so #658 can call it without `BatchCollector`.
3. **`plot` alias:** add `BatchCollector.plot` as a thin alias of `render`.
4. **Scope:** one PR, full delete of collectors plotter bodies (phased commits
inside the PR).

## Status

- [x] Plan accepted — ready for `/iflow-build`
15 changes: 15 additions & 0 deletions .issueflows/03-solved-issues/issue657_status.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Issue #657 — Status

- [x] Done

## What's done

- Re-based collectors drawing onto `cellpy.plotting.collected_plot` (`layout=` / `kind=`).
- Moved plotter bodies out of `collectors.py`; `BatchCollector.render` / `plot` delegate.
- `FrameContext` + design note `plotting-collected.md`.
- Collector figure oracle (`tests/data/collector_figure_specs.json`) + tests.
- Focused suite green (collectors / plotting_package / figure_specs).

## Remaining work

- None.
33 changes: 33 additions & 0 deletions .issueflows/04-designs-and-guides/plotting-collected.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Collected-frame plotting (`layout=` / `kind=`)

## Context

Epic #567 Stage 3 / issue #657 re-bases collectors' drawing half onto
`cellpy.plotting`. Collection, caching, and autonaming stay in
`cellpy.utils.collectors`; drawing uses an already-tidy multi-cell frame.

## Decision

- **`FrameContext`** / **`from_frame`** in `cellpy.plotting.context` wrap a
collected frame plus light metadata (`family_kind`, `units`, `journal`).
- **Public orchestrator:** `cellpy.plotting.collected_plot(frame, *,
family_kind, layout=…, kind=…, backend=…)`.
- Legacy collector knobs map as:
- `method`/`plot_type` `fig_pr_cell` → `layout="per_cell"`
- `fig_pr_cycle` → `layout="per_cycle"`
- `film` → `kind="film"`
- `spread=True` → `kind="spread"`
- default → `kind="line"`
- Flow: resolve layout/kind → `FigureSpec(extras["kind"]="collected", …)` →
`get_backend(...).render` → collected layout engines (plotly primary;
`seaborn` / `matplotlib` → historical seaborn collector path, best-effort).
- **`BatchCollector.render`** (and thin **`plot`** alias) call
`collected_plot`; custom `plotter=` callables still work if provided.
- Collector plotly templates remain in `cellpy.plotting.theme`
(`make_collector_templates`).

## Links

- Issue #657; epic #567; plan
`architecture-plan/cellpy2-plotting-redesign-plan.md` §3.3 / Phase 4
- Related: `plotting-prepare.md`, `plotting-backends.md`
5 changes: 3 additions & 2 deletions .issueflows/04-designs-and-guides/plotting-prepare.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ backend.render as the only summary path. Stage 2 extends the same contract to
style (plotly hover carries `direction`).
- **`LiveHeaders`** lives in `cellpy/plotting/headers.py` (re-exported as
`plotutils._LiveHeaders`).
- **`CellContext`** in `cellpy/plotting/context.py` is the thin cell adapter;
BatchContext waits for collectors rebase.
- **`CellContext`** in `cellpy/plotting/context.py` is the thin cell adapter.
Collected multi-cell frames use **`FrameContext`** / **`collected_plot`**
(`layout=` / `kind=`) — see `plotting-collected.md` (#657).
- Public `summary_plot` / `cycles_plot` / `raw_plot` / `cycle_info_plot` /
`ica_plot` / `dva_plot` stay in `plotutils` and orchestrate context →
registry → prepare → `get_backend(backend).render`.
Expand Down
2 changes: 2 additions & 0 deletions .issueflows/05-epics/epic567_plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ Closes the epic acceptance criteria: one drawing home, batch facade unchanged,
stays safe without the `batch` extra for non-plot imports.
- Depends on: stage 2 issue 2
- yolo: no — large behavioural surface; collection API must not break.
- Published: #657

### Issue: Wire `Batch.plot()` to plotting and delete `batch_plotters.py`

Expand All @@ -214,6 +215,7 @@ Closes the epic acceptance criteria: one drawing home, batch facade unchanged,
docs mention the backend triage.
- Depends on: stage 3 issue 1
- yolo: no — deletes a 1.5k-line module; user-facing batch entry.
- Published: #658

## Later (unstaged)

Expand Down
2 changes: 2 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
(#599). `harmonize()` raises if a schema cast empties a column; new
`LoaderDeclarations.duration_columns` for string durations (loader authors).

* Re-base collectors' drawing half onto `cellpy.plotting` (#657).

## [2.0.0a6] - 2026-07-22

* **Breaking:** `CellpyCell.get_cap` now returns native `cellpycore` curve
Expand Down
9 changes: 7 additions & 2 deletions cellpy/plotting/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
| [`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) |
| [`context`][cellpy.plotting.context] | ``CellContext`` / ``FrameContext`` (#638 / #657) |
| [`collected`][cellpy.plotting.collected] | multi-cell ``collected_plot`` (``layout=`` / ``kind=``) (#657) |

The old locations re-export from here, so nothing that imported them breaks.
Public ``summary_plot`` still lives in ``plotutils`` but runs
Expand All @@ -31,7 +32,8 @@
PlotlyBackend,
get_backend,
)
from cellpy.plotting.context import CellContext, from_source
from cellpy.plotting.collected import collected_plot
from cellpy.plotting.context import CellContext, FrameContext, from_frame, from_source
from cellpy.plotting.cycle_legend import (
DEFAULT_LEGEND_CYCLE_LIMIT,
resolve_cycle_legend_mode,
Expand Down Expand Up @@ -65,12 +67,15 @@
"CellContext",
"DEFAULT_LEGEND_CYCLE_LIMIT",
"FigureSpec",
"FrameContext",
"MatplotlibBackend",
"PanelSpec",
"PlotFamily",
"PlotlyBackend",
"_register_family",
"collected_plot",
"families",
"from_frame",
"from_source",
"get_backend",
"get_family",
Expand Down
5 changes: 5 additions & 0 deletions cellpy/plotting/backends/mpl.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ def render(self, frame: Any, spec: FigureSpec) -> Any:
"""Render a tidy frame according to *spec* (matplotlib Figure)."""
extras = dict(spec.extras or {})
kind = extras.get("kind")
if kind == "collected":
# Collectors historically used seaborn for non-plotly layouts (#657).
from cellpy.plotting.collected import render_collected

return render_collected(frame, spec, backend_override="seaborn")
if kind == "cycles":
return self._render_cycles(frame, spec)
if kind == "raw":
Expand Down
4 changes: 4 additions & 0 deletions cellpy/plotting/backends/plotly.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,10 @@ def __init__(self) -> None:
def render(self, frame: Any, spec: FigureSpec) -> Any:
extras = dict(spec.extras or {})
kind = extras.get("kind")
if kind == "collected":
from cellpy.plotting.collected import render_collected

return render_collected(frame, spec, backend_override="plotly")
if kind == "cycles":
return self._render_cycles(frame, spec)
if kind == "raw":
Expand Down
Loading
Loading