diff --git a/.issueflows/01-current-issues/issue359_original.md b/.issueflows/01-current-issues/issue359_original.md new file mode 100644 index 0000000..a303f18 --- /dev/null +++ b/.issueflows/01-current-issues/issue359_original.md @@ -0,0 +1,25 @@ +# Issue #359: handle full cells that starts with a discharge step + +Source: https://github.com/jepegit/cellpy/issues/359 + +> **Provisional capture.** Upstream issue lives in `jepegit/cellpy` (Stage 5, S3, +> labelled `to core`). The cloud-agent token cannot create issues, so the +> `cellpy/cellpy-core` mirror issue has not been created yet. Once it exists, +> rename this group (`issue359_*` -> `issue_*`) and update `Source:`. +> Draft mirror body: see `issue359_plan.md` -> "Mirror issue (to create)". + +## Original issue text + +We typically define the coulombic efficiency as charge/discharge. One exception is half-cells studying an anode material (where coulombic efficiency makes more sense if it is defined as discharge/charge). Therefore, `cellpy`'s `cycle_mode` parameter was implemented to control how coulombic efficiency is calculated. It is also used (at least for default settings) in defining how charge and discharge curves are extracted and visualized. + +A common pitfall is when testing commercial cells, where one might start with a discharge (cycle 1) and then continue with charge-discharge cycles (cycles 2, 3, 4, ...). It is likely that some files end up with an "erroneous" cycle counter (could be that the first cycle is discharge-charge-discharge, or that all cycles end up as discharge-charge). + +Is there a method in `cellpy` to handle these cases? If not, could it be implemented (e.g. either as another alternative for the `cycle_mode` parameter, or some post-processing steps like `update_cycle_counter(...))? + +## Comments (curated summary) + +- **Clarifications / constraints**: + - Owner (@jepegit) narrows to two candidate solutions: **(1)** more cycle modes, with the handling of each mode implemented in cellpycore; **(2)** more keyword arguments on the cellpycore cycle-summary creator for granular control of how cycle summaries are built. + - The work "has to be done in cellpycore" (core-first; cellpy gets a thin surface). + +_Note: this section is an interpretive summary of the comment thread, not a verbatim dump. Source comments: 1, last comment by @jepegit on 2026-07-23._ diff --git a/.issueflows/01-current-issues/issue359_plan.md b/.issueflows/01-current-issues/issue359_plan.md new file mode 100644 index 0000000..8fe0628 --- /dev/null +++ b/.issueflows/01-current-issues/issue359_plan.md @@ -0,0 +1,189 @@ +# Plan — Issue #359: cycle counter for discharge-first full cells + +Upstream: [jepegit/cellpy#359](https://github.com/jepegit/cellpy/issues/359) (Stage 5 S3, +`to core`). Provisional local number `359`; rename the group when the `cellpy-core` +mirror issue exists (see "Mirror issue (to create)" at the end). + +## Goal + +Give cellpy-core an explicit, opt-in way to repair a cycler's cycle counter so that every +cycle opens with a chosen direction (charge for full/cathode cells, discharge for anode +half-cells), with a lone leading step of the other direction kept as its own first cycle. +Raw cycle-cumulative capacities/energies, the step table and the summary must all follow +the new cycle boundaries, so coulombic efficiency and curve extraction pair the right +half-cycles. + +## Constraints + +- Core-first, additive, default behaviour unchanged: nothing is renumbered unless the + caller asks. Golden parquet fixtures and e2e tests stay byte-identical. +- `TestMode` stays binary (polarity only; `config.py` docstring). Cycle *ordering* is a + separate concern and must not be folded into `TestMode`. +- Harmonized raw mandates cycle-cumulative capacity (`docs/specifications/harmonized-raw.md` + §Capacity convention); the raw spec already lists "do we need an updated cycle number?" + as an open question for `cycle_num` — this issue answers it with a post-processing step, + not a new raw column. +- Metadata boundary: the engine must not require populated metadata (`cellpy-core-migration.md`). + `cycle_mode` on `meta_test_dependent` is read, never required. +- Polars-native; accept pandas in / pandas out like `normalize_capacity_granularity`. +- Per-step stat column names are a fixed contract — no new step columns needed here. +- KISS: one public function, one shared private helper, one test module, one design note. + +### Prior art + +- `summarizers.normalize_capacity_granularity` — reconstructs per-row increments inside a + source reset group and re-accumulates over `(test_id, cycle_num)`. Exactly the math a + renumbering needs (source = *old* cycle keys, target = *new* cycle keys). **Migrate** the + two-pass body into a shared private helper and call it from both. +- `summarizers._ensure_test_id`, `_group_keys`, `_CUMULATIVE_ATTRS` — reuse as-is. +- `make_step_table` — classification depends on current/potential stats only, never on + `cycle_num`, so rebuilding steps after renumbering re-derives identical step types. +- `make_summary(test_mode=...)` — CE direction already keyed on `TestMode`; untouched. +- `merge.update_data` — incremental append keyed on cycler cycle numbers. Coexist: renumber + is a post-processing step a consumer applies after the raw is complete (documented + limitation; no change to merge). +- Toolbox `.issueflows/00-tools/` — empty. Graph report stale (2026-07); grep used. + +## Approach + +### Public API + +```python +def renumber_cycles( + data: Data, + schema: Optional[Schema] = None, + opening: StepDirection = StepDirection.CHARGE, # or test_mode -> derive + **step_table_kwargs, +) -> Data +``` + +- Lives in `summarizers.py` next to `normalize_capacity_granularity`; exported from + `cellpycore/__init__.py`. +- `opening` says which direction starts a cycle. Default charge (matches `TestMode.NORMAL`). + Anode half-cells pass `discharge`. Implementation: a tiny `StrEnum` in `config.py` + (`StepDirection.CHARGE / DISCHARGE`) so the value is validated, mirroring the existing + enums. Convenience: `opening=None` -> derive from `TestMode` passed as keyword + (`test_mode=TestMode.INVERTED` -> discharge). Keep only one of these two knobs if it + feels like two ways to say the same thing — see Open questions. +- Requires `data.raw` and a classified `data.steps` (`NoDataFound` otherwise). Raises + `ValueError` if the step table carries `ustep` rows (ambiguous raw mapping) or if a + `(test_id, cycle, step)` key is not unique. + +### Algorithm (polars, per `test_id`) + +1. Sort steps by `test_id`, `test_time_first` (fallback `datapoint_num_first`). +2. Direction per step from `step_type`: charge-ish = `charge, cv_charge, taper_charge, + charge_cv`; discharge-ish = `discharge, cv_discharge, taper_discharge, discharge_cv`; + everything else (`rest, ir, ocvrlx_*, not_known, ""`) is neutral and attaches to the + current cycle. +3. `prev` = forward-filled last non-neutral direction *before* this step (`shift(1)` over + `test_id` on the forward-filled series). New cycle boundary iff + `dir == opening and prev == closing`. First step of a test is never a boundary, so a + lone leading discharge becomes cycle 1 on its own. +4. `new_cycle = base + cum_sum(boundary)` over `test_id`, with `base` = the test's original + first `cycle_num` (keeps 0- vs 1-based numbering of the source). +5. Build the mapping `(test_id, old_cycle, step_num) -> new_cycle`; join onto raw. Raw rows + with no step row (e.g. `skip_steps`) forward-fill the new cycle in datapoint order, + remaining nulls keep the old value. +6. Re-accumulate `_CUMULATIVE_ATTRS` columns via the shared helper: increments over + `(test_id, old_cycle)`, cum_sum over `(test_id, new_cycle)`. Identity wherever the + boundaries did not move, so an already-correct file is a no-op (test for it). +7. Replace raw `cycle_num`, drop helper columns, restore frame type (pandas/polars). +8. Rebuild steps with `make_step_table(data, schema=schema, **step_table_kwargs)` so the + per-step cumulative-capacity stats follow the new boundaries (deltas are invariant, but + `*_first/last/avg/...` of cumulative columns are not). Classification is unchanged by + construction. +9. `data.summary = None` and log at info that the summary must be rebuilt + (`make_summary(...)`); the function does not know the caller's `test_mode` / + `exclude_step_types`. + +Expected outcomes (charge opening): + +- cycler `[D][C D][C D]` -> unchanged (no-op). +- cycler `[D C D][C D]` -> `[D][C D][C D]`. +- cycler `[D C][D C][D C]` -> `[D][C D][C D][C]`. + +Cycle 1 (lone discharge) gets `CE = discharge/0 = inf` under `NORMAL`; left as-is and +documented — CE math is out of scope. + +### cellpy wiring (not in this PR, recorded for the design note) + +cellpy adds a `cycle_mode` variant (e.g. `"full_cell_discharge_first"`) that maps to +`TestMode.NORMAL` + `renumber_cycles(opening="charge")` after the step table is built. This +is owner "Solution 1" at the cellpy layer backed by owner "Solution 2" (explicit control) +at the core layer. Convention delta -> release note + comparator exception on the cellpy +side (stage5 §S3). + +## Files to touch + +- `src/cellpycore/config.py` — add `StepDirection` StrEnum (CHARGE / DISCHARGE) with + Google-style docstring. +- `src/cellpycore/summarizers.py` — extract `_reaccumulate_cumulative(raw, cols, + source_keys, target_keys)` from `normalize_capacity_granularity`; add + `_step_direction_expr(shdr)` and public `renumber_cycles(...)`. +- `src/cellpycore/__init__.py` — export `renumber_cycles` (and `StepDirection`), extend + `__all__` and module docstring. +- `tests/test_renumber_cycles.py` — new (see Test strategy). +- `docs/specifications/harmonized-raw.md` — resolve the `cycle_num` open question with a + one-line pointer to `renumber_cycles`. +- `docs/user-guide/` (or `docs/api/public.md`) — short "repairing the cycle counter" + section with the three patterns above. +- `.issueflows/04-designs-and-guides/cycle-renumbering.md` — new design note (context, + decision, alternatives: new `TestMode` member / kwarg on `make_summary` / raw column; + cellpy wiring; limitations: usteps, incremental `update_data`). +- `docs/changelog.md` — entry at close (feature -> `uv version --bump minor`, `0.2.6` -> + `0.3.0`, decided at `/iflow-close`). + +## Test strategy + +Command: `uv run pytest` plus `uv run ruff check && uv run ruff format --check`. + +New `tests/test_renumber_cycles.py` with a small synthetic-raw builder (same style as +`tests/test_exclude_types.py::_build_cv_raw`; override step types via +`override_step_types` so classification is deterministic): + +- no-op on an already charge-first file (raw, steps, summary unchanged). +- `[D C D][C D]` -> `[D][C D][C D]`; per-cycle `charge_capacity` / `discharge_capacity` + in `make_summary` equal the hand-computed per-half-cycle totals. +- `[D C][D C][D C]` -> four cycles, trailing lone charge kept. +- rest / ir steps attach to the current cycle; CC + CV charge stay in one cycle. +- `opening=DISCHARGE` (anode) mirrors the above with roles swapped. +- two `test_id`s in one frame are renumbered independently (base per test preserved). +- pandas in -> pandas out. +- `ustep` step table -> `ValueError`; missing steps -> `NoDataFound`. +- regression: `renumber_cycles` on the harmonized/golden fixture (if present) leaves raw + byte-identical and `normalize_capacity_granularity` still passes its existing tests + after the helper extraction. + +## Open questions + +1. **Mirror issue number** — I cannot create issues with this token. Please create the + `cellpy/cellpy-core` issue (body below) and tell me the number; I rename the + `.issueflows` group and reference it in commits/PR. Until then everything is tracked + as `359`. +2. **Name**: `renumber_cycles` (recommended; verb is unambiguous) vs the issue's + `update_cycle_counter`. +3. **Knob**: `opening: StepDirection` only (recommended; one knob) vs also accepting + `test_mode: TestMode` and deriving it. cellpy can do the derivation in its thin surface. +4. **Numbering base**: keep the source's first cycle number per test (recommended) vs + always restart at 1. +5. **`CellpyCellCore.renumber_core_cycles(...)` wrapper**: skip for now (recommended, + YAGNI; cellpy calls the function) vs add a 10-line passthrough like `make_core_summary`. + +## Mirror issue (to create) + +Title: `Cycle counter for discharge-first full cells (cellpy#359 / Stage 5 S3)` — label +`enhancement`. Body: + +> Mirrors [jepegit/cellpy#359](https://github.com/jepegit/cellpy/issues/359) (Stage 5, S3 — labelled `to core`). +> +> **Problem.** Commercial / full cells are often tested starting with a discharge (cycle 1 = discharge only), followed by charge–discharge cycles. Cyclers then assign an "erroneous" cycle counter: either cycle 1 becomes discharge–charge–discharge, or every cycle ends up discharge–charge. Downstream, coulombic efficiency (charge/discharge for full cells) and charge/discharge curve extraction pair the wrong half-cycles. +> +> **Current state in core.** `cycle_index` is taken as-is from the harmonized raw frame; core never renumbers cycles. Polarity is handled only via `TestMode` (`NORMAL` / `INVERTED`, from legacy `cycle_mode`) in `cell_core.py`, `curves.py` and `summarizers.py`. There is no notion of "discharge-first" ordering within a cycle. +> +> **Scope (core-first; cellpy gets a thin surface later).** +> - Add an explicit `renumber_cycles(...)` post-processing step that renumbers `cycle_num` so each cycle opens with a chosen direction (charge for full/cathode cells, discharge for anode half-cells), keeping a leading lone step of the other direction as its own cycle. Raw cycle-cumulative capacities/energies, the step table and the summary follow the new boundaries. +> - Additive only; default behaviour unchanged (cycler counter kept). Golden/e2e fixtures stay green. +> - Tests: synthetic raw frames with discharge-first patterns (lone first discharge; all-cycles discharge–charge), plus a design note under `.issueflows/04-designs-and-guides/` since this is a user-visible convention change (release note needed in cellpy). +> +> **Out of scope.** cellpy-side `cycle_mode` plumbing / CLI; S1 (#313) and S2 (#312). diff --git a/.issueflows/01-current-issues/issue359_status.md b/.issueflows/01-current-issues/issue359_status.md new file mode 100644 index 0000000..fb459c2 --- /dev/null +++ b/.issueflows/01-current-issues/issue359_status.md @@ -0,0 +1,29 @@ +# Status — Issue #359: cycle counter for discharge-first full cells + +Branch: `cursor/359-discharge-first-cycles-061c` (cloud-agent naming; local number +provisional, mirrors jepegit/cellpy#359 until the cellpy-core mirror issue exists). +PR: https://github.com/cellpy/cellpy-core/pull/153 (#153, draft) + +- [ ] Done + +## What's done + +- 2026-10-01: issue captured, plan drafted and accepted (recommendations taken: + `renumber_cycles`, single `opening: StepDirection` knob, keep source numbering base, + no `CellpyCellCore` wrapper). +- `config.StepDirection` enum (CHARGE / DISCHARGE). +- `summarizers.renumber_cycles` + shared `_reaccumulate_cumulative` / + `_present_cumulative_cols` helpers (extracted from `normalize_capacity_granularity`); + exported from `cellpycore`. +- `tests/test_renumber_cycles.py` (14 tests: three patterns, rest/CV attachment, anode + mirror, 0-based numbering, energy columns, multi `test_id`, pandas round-trip, summary + drop, error paths, harmonized fixture no-op + recount). Full suite 303 passed; ruff clean. +- Docs: `standalone-use.md` section, `harmonized-raw.md` open question resolved, design + note `04-designs-and-guides/cycle-renumbering.md`. + +## Remaining work + +- Create the `cellpy/cellpy-core` mirror issue (token here is read-only) and rename the + `issue359_*` group to the real number. +- `/iflow-close`: changelog entry, `uv version --bump minor` (0.2.6 -> 0.3.0), final push. +- cellpy side (separate repo, later): `cycle_mode` variant wiring + release note. diff --git a/.issueflows/04-designs-and-guides/cycle-renumbering.md b/.issueflows/04-designs-and-guides/cycle-renumbering.md new file mode 100644 index 0000000..73bdabd --- /dev/null +++ b/.issueflows/04-designs-and-guides/cycle-renumbering.md @@ -0,0 +1,52 @@ +# Cycle renumbering for discharge-first cells + +Issue: [jepegit/cellpy#359](https://github.com/jepegit/cellpy/issues/359) (Stage 5 S3, +`to core`); core PR [#153](https://github.com/cellpy/cellpy-core/pull/153). + +## Context + +Full / commercial cells are often started with a lone discharge. The cycler's cycle counter +then disagrees with the "charge opens a cycle" convention (`[D C D][C D]…` or +`[D C][D C]…`), so coulombic efficiency and curve extraction pair the wrong half-cycles. +Core took `cycle_num` as-is and only knew about polarity (`TestMode`). + +## Decision + +- **Explicit opt-in post-processing step** `summarizers.renumber_cycles(data, schema=None, + opening=StepDirection.CHARGE, **step_table_kwargs)`, exported top-level. Default engine + behaviour unchanged; golden fixtures untouched. +- **New enum `config.StepDirection`** (`CHARGE` / `DISCHARGE`) for the opening direction. + `TestMode` stays binary (polarity only); half-cycle *ordering* is a separate axis. +- **Algorithm** (per `test_id`, steps sorted by `datapoint_num_first`): direction from + `step_type` (charge-ish / discharge-ish / neutral); a boundary opens where an `opening` + step follows the last non-neutral step of the other direction; the first step is never a + boundary (a leading run of closing steps becomes cycle 1). Numbering restarts from the + test's original first cycle number. +- **Raw capacities follow**: the cycle-cumulative capacity / energy columns are + re-accumulated from old to new cycle keys with `_reaccumulate_cumulative` (the body + extracted from `normalize_capacity_granularity`). Exact no-op (early return) when the + mapping is the identity. +- **Steps are rebuilt** with `make_step_table(**step_table_kwargs)` (per-step cumulative + `*_first/last` stats shift; classification is cycle-independent). **Summary is dropped** + (`data.summary = None`) — caller reruns `make_summary` with its own `test_mode`. +- A lone first discharge yields `CE = inf` under `NORMAL`; left as-is (honest value). + +## Alternatives considered + +- New `TestMode` member (e.g. `FULL_DISCHARGE_FIRST`) — conflates polarity with ordering; + `TestMode` docstring explicitly wants it binary. Rejected. +- Keyword on `make_summary` only — cannot fix raw cumulative capacities or the step table, + so curves / steps would still be mis-paired. Rejected. +- New raw column (`corrected_cycle_num`) — spec answer: no; keep one `cycle_num`, repair it. +- Mapping-only update of `steps.cycle_num` instead of rebuild — leaves cumulative-capacity + step stats stale. Rejected (rebuild is one extra group_by). + +## Limitations / follow-ups + +- `usteps=True` step tables are rejected (ustep rows cannot be mapped back onto raw). +- Apply after the raw is complete — `merge.update_data` keys on the cycler counter. +- Owner's two solutions ([comment](https://github.com/jepegit/cellpy/issues/359)): core + delivers "Solution 2" (explicit control); cellpy wires "Solution 1" on top — a + `cycle_mode` variant (e.g. `"full_cell_discharge_first"`) that maps to `TestMode.NORMAL` + + `renumber_cycles(opening=CHARGE)` after the step table. User-visible convention change + → release note + comparator exception on the cellpy side (stage5 §S3). diff --git a/docs/specifications/harmonized-raw.md b/docs/specifications/harmonized-raw.md index 3a42496..b86a386 100644 --- a/docs/specifications/harmonized-raw.md +++ b/docs/specifications/harmonized-raw.md @@ -180,7 +180,10 @@ The `CellMeta` dataclass additionally carries two core-only fields legacy never native datetime with the helpers in `cellpycore.timestamps`. - **cycle_num** - usually provided by the tester - - enough with keeping it, or do we need an updated cycle number? + - ~~enough with keeping it, or do we need an updated cycle number?~~ **Resolved (issue #359):** + the raw keeps the tester's counter; a corrected counter is an opt-in post-processing + step (`summarizers.renumber_cycles`), which also re-accumulates the cycle-cumulative + capacity / energy columns to the new boundaries. No extra raw column. - **cycle_type** - details to be discussed - *Reference vocabulary now defined as `cellpycore.config.CycleType` (`Standard`/`GITT`/`ICI`/`Characterization`); may later migrate to test metadata as `test_type` (issue #24).* diff --git a/docs/user-guide/standalone-use.md b/docs/user-guide/standalone-use.md index 2771acf..51538f2 100644 --- a/docs/user-guide/standalone-use.md +++ b/docs/user-guide/standalone-use.md @@ -150,6 +150,39 @@ that when loading real cells. Do not rely on implicit defaults across layers. cycles) plus `{column}_{gravimetric,areal,absolute}` variants of the capacity-like columns (`CycleCols().specific_columns`). +### Repairing the cycle counter (discharge-first cells) + +Commercial / full cells are often started with a lone discharge. Cyclers then +tend to report a counter that does not match the convention — cycle 1 as +`discharge-charge-discharge`, or every cycle as `discharge-charge` — so the +coulombic efficiency and the charge/discharge curves pair the wrong +half-cycles. `summarizers.renumber_cycles` is the opt-in repair: it renumbers +`cycle_num` so every cycle opens with a chosen direction and keeps a leading +run of the other direction as the first cycle on its own. + +```python +from cellpycore import summarizers, config + +data = summarizers.make_step_table(data) +data = summarizers.renumber_cycles(data, opening=config.StepDirection.CHARGE) +data = summarizers.make_summary(data) # the stale summary is dropped by renumber_cycles +``` + +| cycler counter | after `opening=CHARGE` | +|-----------------------|------------------------| +| `[D] [C D] [C D]` | unchanged (no-op) | +| `[D C D] [C D]` | `[D] [C D] [C D]` | +| `[D C] [D C] [D C]` | `[D] [C D] [C D] [C]` | + +Use `StepDirection.DISCHARGE` for anode half-cells (`TestMode.INVERTED`). The +raw cumulative capacity / energy columns are re-accumulated to the new +boundaries, the step table is rebuilt (pass the same `make_step_table` kwargs, +e.g. `raw_limits=...`), and rest / IR / OCV steps attach to the cycle they occur +in. A lone first discharge gives `coulombic_efficiency = inf` for that cycle +under the normal convention — that is the honest value. Run it once the raw +data is complete; incremental appends (`update_data`) key on the cycler's own +counter. + ## The class-free alternative The engine functions in `cellpycore.summarizers` work directly on a `Data` diff --git a/src/cellpycore/__init__.py b/src/cellpycore/__init__.py index 8679043..cc18173 100644 --- a/src/cellpycore/__init__.py +++ b/src/cellpycore/__init__.py @@ -3,8 +3,8 @@ The public, stable surface is the curated set of names re-exported here: the cell classes (``CellpyCellCore``, its legacy bridge ``OldCellpyCellCore``), the ``Data`` container, the engine entry points (``make_step_table``, -``add_step_c_rate``, ``make_summary``), the column-header schema types, and the package -exceptions. Everything else (``units``, ``timestamps``, ``legacy``, ``metadata``, +``add_step_c_rate``, ``make_summary``, ``renumber_cycles``), the column-header +schema types, and the package exceptions. Everything else (``units``, ``timestamps``, ``legacy``, ``metadata``, ``testing`` ...) remains importable as submodules but is not part of the guaranteed top-level API. """ @@ -23,11 +23,17 @@ RawCols, Schema, StepCols, + StepDirection, default_schema, ) from cellpycore.exceptions import CellpyError, NoDataFound from cellpycore.merge import merge_data, update_data -from cellpycore.summarizers import add_step_c_rate, make_step_table, make_summary +from cellpycore.summarizers import ( + add_step_c_rate, + make_step_table, + make_summary, + renumber_cycles, +) try: __version__ = version("cellpycore") @@ -44,6 +50,7 @@ "RawCols", "Schema", "StepCols", + "StepDirection", "__version__", "add_step_c_rate", "cast_raw_frame", @@ -51,6 +58,7 @@ "make_step_table", "make_summary", "merge_data", + "renumber_cycles", "update_data", "validate_raw_frame", ] diff --git a/src/cellpycore/config.py b/src/cellpycore/config.py index d95271a..e3e8af2 100644 --- a/src/cellpycore/config.py +++ b/src/cellpycore/config.py @@ -106,6 +106,29 @@ class ResetGranularity(StrEnum): TEST = "test" +class StepDirection(StrEnum): + """Current direction of a charge / discharge step. + + Used by :func:`cellpycore.summarizers.renumber_cycles` to say which + direction *opens* a cycle when a cycler's cycle counter has to be repaired + (e.g. a full cell whose test starts with a lone discharge). Deliberately + separate from :class:`TestMode`: ``TestMode`` is about sign / coulombic- + efficiency polarity, this enum is about the ordering of half-cycles within + a cycle. + + Attributes: + CHARGE (``"charge"``): Charge steps (``charge``, ``cv_charge``, + ``taper_charge``, ``charge_cv``). The usual opening direction for + full cells and cathode half-cells (``TestMode.NORMAL``). + DISCHARGE (``"discharge"``): Discharge steps (``discharge``, + ``cv_discharge``, ``taper_discharge``, ``discharge_cv``). The usual + opening direction for anode half-cells (``TestMode.INVERTED``). + """ + + CHARGE = "charge" + DISCHARGE = "discharge" + + class StepType(StrEnum): """Canonical step-type labels for the ``step_type`` column of the step table. diff --git a/src/cellpycore/summarizers.py b/src/cellpycore/summarizers.py index feac2dd..add9996 100644 --- a/src/cellpycore/summarizers.py +++ b/src/cellpycore/summarizers.py @@ -11,6 +11,8 @@ ResetGranularity, Schema, StepCols, + StepDirection, + StepType, TestMode, default_schema, ) @@ -179,29 +181,243 @@ def normalize_capacity_granularity( source_keys = [nhdr.test_id] cycle_keys = [nhdr.test_id, nhdr.cycle_num] - cols = [ + raw = _reaccumulate_cumulative( + raw, _present_cumulative_cols(raw, nhdr), source_keys, cycle_keys + ) + + data.raw = raw.to_pandas() if was_pandas else raw + return data + + +def _present_cumulative_cols(raw: "pl.DataFrame", nhdr) -> list: + """Return the cumulative capacity / energy columns present in ``raw``.""" + return [ getattr(nhdr, attr) for attr in _CUMULATIVE_ATTRS if getattr(nhdr, attr) in raw.columns ] - if cols: - # Two passes (polars disallows a window expr nested inside another - # window's aggregation): (1) per-row increment within the source reset - # group (first row of the group keeps its own value), then (2) - # re-accumulate over the cycle. - increments = [] - for col in cols: - diff = pl.col(col) - pl.col(col).shift(1).over(source_keys) - increments.append( - pl.when(diff.is_null()).then(pl.col(col)).otherwise(diff).alias(col) - ) - raw = raw.with_columns(increments) - raw = raw.with_columns( - [pl.col(col).cum_sum().over(cycle_keys).alias(col) for col in cols] + + +def _reaccumulate_cumulative( + raw: "pl.DataFrame", cols: list, source_keys: list, target_keys: list +) -> "pl.DataFrame": + """Re-accumulate cumulative columns from ``source_keys`` groups to ``target_keys``. + + For each column, the per-row increment within the *source* reset group is + reconstructed (the first row of a group keeps its own value) and then + re-accumulated over the *target* group. ``raw`` must already be sorted by + datapoint. Shared by :func:`normalize_capacity_granularity` (step/test -> + cycle) and :func:`renumber_cycles` (old cycle -> new cycle). + """ + if not cols: + return raw + # Two passes (polars disallows a window expr nested inside another + # window's aggregation). + increments = [] + for col in cols: + diff = pl.col(col) - pl.col(col).shift(1).over(source_keys) + increments.append( + pl.when(diff.is_null()).then(pl.col(col)).otherwise(diff).alias(col) ) + raw = raw.with_columns(increments) + return raw.with_columns( + [pl.col(col).cum_sum().over(target_keys).alias(col) for col in cols] + ) + + +# Step types counted as charge / discharge when deciding where a cycle opens +# (see ``renumber_cycles``). Everything else (rest, ir, ocvrlx_*, "") is +# neutral and attaches to the current cycle. +_CHARGE_STEP_TYPES = ( + StepType.CHARGE, + StepType.CV_CHARGE, + StepType.TAPER_CHARGE, + StepType.CHARGE_CV, +) +_DISCHARGE_STEP_TYPES = ( + StepType.DISCHARGE, + StepType.CV_DISCHARGE, + StepType.TAPER_DISCHARGE, + StepType.DISCHARGE_CV, +) + + +def renumber_cycles( + data: Data, + schema: Optional[Schema] = None, + opening: StepDirection = StepDirection.CHARGE, + **step_table_kwargs, +) -> Data: + """Renumber cycles so that every cycle opens with the given step direction. + + Repairs a cycler's cycle counter for tests whose half-cycle ordering does + not match the cell convention — typically a full cell that starts with a + lone discharge, where the cycler then reports ``discharge-charge-discharge`` + for cycle 1 or ``discharge-charge`` for every cycle. After renumbering, + each cycle contains at most one run of ``opening`` steps followed by at most + one run of the other direction; neutral steps (rest, ir, ocv relaxation) + attach to the cycle they occur in. A leading run of the *closing* + direction is kept as the first cycle on its own, so coulombic efficiency + and curve extraction pair the right half-cycles from the second cycle on. + + The raw cumulative capacity / energy columns are re-accumulated to the new + cycle boundaries (they stay cycle-cumulative, see + ``docs/specifications/harmonized-raw.md``), the step table is rebuilt with + :func:`make_step_table` so its per-step cumulative statistics follow, and + any existing summary is dropped (call :func:`make_summary` again). + + Args: + data (Data): The data object (needs ``raw`` and a classified ``steps``). + schema: The column-header schema to use. Defaults to the native + cellpy-core schema when not provided. + opening (StepDirection): The direction that starts a cycle. + ``StepDirection.CHARGE`` (default) for full cells / cathode + half-cells, ``StepDirection.DISCHARGE`` for anode half-cells. + **step_table_kwargs: Forwarded to :func:`make_step_table` when the + step table is rebuilt (e.g. ``override_step_types``, + ``raw_limits``). Pass the same arguments the original step table + was built with. + + Returns: + Data: The same object with ``raw`` and ``steps`` renumbered (frame types + preserved — pandas in, pandas out) and ``summary`` set to ``None``. + Returned untouched when the counter already matches the convention. + + Raises: + NoDataFound: If ``data.raw`` or ``data.steps`` is missing. + ValueError: If required columns are missing, if the step table carries + ``ustep`` rows, or if ``(test_id, cycle_num, step_num)`` does not + identify a single step row. + + Note: + Cycle numbering restarts from each test's original first cycle number + (``0``- or ``1``-based input stays that way). A lone leading discharge + under ``TestMode.NORMAL`` yields ``coulombic_efficiency = inf`` for that + first cycle (``discharge / 0``); that is the honest value and is left to + the consumer. Apply this after the raw data is complete — incremental + appends (:func:`cellpycore.merge.update_data`) key on the cycler's own + counter. + """ + if schema is None: + schema = default_schema() + nhdr, shdr = schema.raw, schema.step + opening = StepDirection(opening) + open_sign = 1 if opening == StepDirection.CHARGE else -1 + + _require_frame(data.raw, "raw") + _require_frame(data.steps, "steps") + raw = data.raw + was_pandas = not isinstance(raw, pl.DataFrame) + if was_pandas: + raw = pl.from_pandas(raw) + steps = data.steps + if not isinstance(steps, pl.DataFrame): + steps = pl.from_pandas(steps) + _require_columns( + raw, + { + "datapoint_num": nhdr.datapoint_num, + "cycle_num": nhdr.cycle_num, + "step_num": nhdr.step_num, + }, + "raw", + ) + _require_columns( + steps, + { + "cycle_num": shdr.cycle_num, + "step_num": shdr.step_num, + "step_type": shdr.step_type, + "datapoint_num_first": shdr.datapoint_num_first, + }, + "steps", + ) + if "ustep" in steps.columns: + raise ValueError( + "renumber_cycles needs a step table built without usteps " + "(ustep rows cannot be mapped back onto the raw frame)" + ) + + raw = _ensure_test_id(raw, nhdr.test_id).sort(nhdr.datapoint_num) + steps = _ensure_test_id(steps, shdr.test_id).sort( + [shdr.test_id, shdr.datapoint_num_first] + ) + tid = shdr.test_id + + # Direction per step: +1 charge, -1 discharge, 0 neutral. A new cycle opens + # where an ``opening`` step follows the last non-neutral step of the other + # direction. The first step of a test is never a boundary, so a leading run + # of closing steps becomes a cycle of its own. + direction = ( + pl.when(pl.col(shdr.step_type).is_in([str(t) for t in _CHARGE_STEP_TYPES])) + .then(1) + .when(pl.col(shdr.step_type).is_in([str(t) for t in _DISCHARGE_STEP_TYPES])) + .then(-1) + .otherwise(0) + ) + steps = steps.with_columns(direction.alias("__dir")) + steps = steps.with_columns( + pl.when(pl.col("__dir") != 0) + .then(pl.col("__dir")) + .otherwise(None) + .forward_fill() + .over(tid) + .alias("__prev") + ) + steps = steps.with_columns(pl.col("__prev").shift(1).over(tid)) + boundary = (pl.col("__dir") == open_sign) & (pl.col("__prev") == -open_sign) + steps = steps.with_columns( + ( + pl.col(shdr.cycle_num).min().over(tid) + + boundary.cast(pl.Int64).cum_sum().over(tid) + ).alias("__new_cycle") + ) + + mapping_keys = [tid, shdr.cycle_num, shdr.step_num] + mapping = steps.select([*mapping_keys, "__new_cycle"]) + if mapping.n_unique(subset=mapping_keys) != mapping.height: + raise ValueError( + "renumber_cycles needs (test_id, cycle_num, step_num) to identify a " + "single step row; the step table has duplicate keys" + ) + if (mapping["__new_cycle"] == mapping[shdr.cycle_num]).all(): + logger.debug("renumber_cycles: cycle counter already matches; no-op") + return data + + # Map the new cycle number onto the raw rows. Rows without a step row + # (e.g. ``skip_steps``) inherit the preceding row's cycle, else keep theirs. + raw = raw.join( + mapping, + left_on=[nhdr.test_id, nhdr.cycle_num, nhdr.step_num], + right_on=mapping_keys, + how="left", + ) + raw = raw.with_columns( + pl.col("__new_cycle") + .forward_fill() + .over(nhdr.test_id) + .fill_null(pl.col(nhdr.cycle_num)) + .cast(raw.schema[nhdr.cycle_num]) + ) + raw = _reaccumulate_cumulative( + raw, + _present_cumulative_cols(raw, nhdr), + [nhdr.test_id, nhdr.cycle_num], + [nhdr.test_id, "__new_cycle"], + ) + raw = raw.with_columns(pl.col("__new_cycle").alias(nhdr.cycle_num)).drop( + "__new_cycle" + ) + + n_old = mapping[shdr.cycle_num].n_unique() + n_new = mapping["__new_cycle"].n_unique() + logger.info(f"renumber_cycles: {n_old} -> {n_new} cycles (opening={opening.value})") data.raw = raw.to_pandas() if was_pandas else raw - return data + if data.summary is not None: + logger.info("renumber_cycles: dropping stale summary; rerun make_summary") + data.summary = None + return make_step_table(data, schema=schema, **step_table_kwargs) def _delta_expr(base: str) -> pl.Expr: diff --git a/tests/test_renumber_cycles.py b/tests/test_renumber_cycles.py new file mode 100644 index 0000000..6952b45 --- /dev/null +++ b/tests/test_renumber_cycles.py @@ -0,0 +1,331 @@ +"""Tests for ``summarizers.renumber_cycles`` (issue #359 / Stage 5 S3). + +A cycler's cycle counter is repaired so every cycle opens with a chosen step +direction. Synthetic raw frames are built from a per-cycler-cycle pattern of +half-cycle tokens (``"C"`` charge, ``"D"`` discharge, ``"R"`` rest, ``"V"`` +cv_charge); the expected result is derived with a plain Python oracle over the +same tokens. +""" + +from pathlib import Path + +import pandas as pd +import polars as pl +import pytest + +from cellpycore import config, summarizers +from cellpycore.cell_core import Data +from cellpycore.config import StepDirection, default_schema +from cellpycore.exceptions import NoDataFound + +POINTS_PER_STEP = 4 +CAP_PER_POINT = 0.1 +HARMONIZED_RAW = Path(__file__).parent / "data" / "cycler_cc_harmonized_raw.parquet" + +_TOKEN_DIRECTION = {"C": 1, "V": 1, "D": -1, "R": 0} +_TOKEN_STEP_TYPE = {"C": "charge", "V": "cv_charge", "D": "discharge", "R": "rest"} + + +def _build_raw(pattern, nhdr, first_cycle=1, test_id=None, dp0=0, with_energy=False): + """Raw frame from a list of cycler cycles, each a list of half-cycle tokens. + + Step numbers are unique over the whole test (so the step table maps back + unambiguously). Capacities are cycle-cumulative per the cycler's own + counter; current/potential are shaped so the built-in classifier labels the + steps without overrides. + """ + records = [] + dp = dp0 + step = 0 + for i, tokens in enumerate(pattern): + cyc = first_cycle + i + cc = dc = 0.0 + volt = 3.7 + for tok in tokens: + step += 1 + for k in range(POINTS_PER_STEP): + if tok == "C": + cur, volt, cc = 1.0, volt + 0.1, cc + CAP_PER_POINT + elif tok == "V": + cur, cc = 0.5 - 0.1 * k, cc + CAP_PER_POINT / 2 + elif tok == "D": + cur, volt, dc = -1.0, volt - 0.1, dc + CAP_PER_POINT + else: # rest: a tiny drift keeps the classifier off the ``ir`` rule + cur, volt = 0.0, volt + 0.001 + row = { + nhdr.datapoint_num: dp, + nhdr.test_time: float(dp), + nhdr.step_time: float(k), + nhdr.cycle_num: cyc, + nhdr.step_num: step, + nhdr.current: cur, + nhdr.potential: volt, + nhdr.cumulative_charge_capacity: cc, + nhdr.cumulative_discharge_capacity: dc, + } + if with_energy: + row[nhdr.cumulative_charge_energy] = cc * 3.7 + row[nhdr.cumulative_discharge_energy] = dc * 3.7 + if test_id is not None: + row[nhdr.test_id] = test_id + records.append(row) + dp += 1 + return pl.DataFrame(records) + + +def _oracle(pattern, opening=StepDirection.CHARGE, first_cycle=1): + """Expected (new cycle per step, per-cycle charge/discharge totals).""" + open_sign = 1 if opening == StepDirection.CHARGE else -1 + cycles, totals = [], {} + prev = None + cyc = first_cycle + first = True + for tokens in pattern: + for tok in tokens: + d = _TOKEN_DIRECTION[tok] + if not first and d == open_sign and prev == -open_sign: + cyc += 1 + first = False + if d != 0: + prev = d + cycles.append(cyc) + ch, dch = totals.setdefault(cyc, [0.0, 0.0]) + if tok == "C": + ch += CAP_PER_POINT * POINTS_PER_STEP + elif tok == "V": + ch += CAP_PER_POINT / 2 * POINTS_PER_STEP + elif tok == "D": + dch += CAP_PER_POINT * POINTS_PER_STEP + totals[cyc] = [ch, dch] + return cycles, totals + + +def _data(raw, schema): + data = Data() + data.raw = raw + summarizers.make_step_table(data, schema) + return data + + +def _step_types(data, schema): + shdr = schema.step + steps = data.steps.sort(shdr.datapoint_num_first) + return steps[shdr.step_type].to_list() + + +def _check_pattern(pattern, opening=StepDirection.CHARGE, first_cycle=1): + schema = default_schema() + nhdr, shdr, chdr = schema.raw, schema.step, schema.cycle + raw = _build_raw(pattern, nhdr, first_cycle=first_cycle) + data = _data(raw, schema) + tokens = [tok for tokens in pattern for tok in tokens] + assert _step_types(data, schema) == [_TOKEN_STEP_TYPE[t] for t in tokens] + + summarizers.renumber_cycles(data, schema, opening=opening) + + exp_cycles, exp_totals = _oracle(pattern, opening, first_cycle) + steps = data.steps.sort(shdr.datapoint_num_first) + assert steps[shdr.cycle_num].to_list() == exp_cycles + assert _step_types(data, schema) == [_TOKEN_STEP_TYPE[t] for t in tokens] + # Raw rows carry the same new cycle as their step. + per_row = [c for c in exp_cycles for _ in range(POINTS_PER_STEP)] + assert data.raw.sort(nhdr.datapoint_num)[nhdr.cycle_num].to_list() == per_row + + summarizers.make_summary(data, schema) + summary = data.summary.sort(chdr.cycle_num) + assert summary[chdr.cycle_num].to_list() == sorted(exp_totals) + for cyc, ch, dch in zip( + summary[chdr.cycle_num], + summary[chdr.charge_capacity], + summary[chdr.discharge_capacity], + ): + assert ch == pytest.approx(exp_totals[cyc][0]) + assert dch == pytest.approx(exp_totals[cyc][1]) + return data + + +def test_lone_first_discharge_already_correct_is_noop(): + schema = default_schema() + raw = _build_raw([["D"], ["C", "D"], ["C", "D"]], schema.raw) + data = _data(raw, schema) + raw_before, steps_before = data.raw.clone(), data.steps.clone() + summarizers.make_summary(data, schema) + + summarizers.renumber_cycles(data, schema) + + assert data.raw.equals(raw_before) + assert data.steps.equals(steps_before) + assert data.summary is not None # untouched on a no-op + + +def test_first_cycle_discharge_charge_discharge_is_split(): + data = _check_pattern([["D", "C", "D"], ["C", "D"], ["C", "D"]]) + chdr = default_schema().cycle + assert data.summary[chdr.cycle_num].to_list() == [1, 2, 3, 4] + + +def test_all_cycles_discharge_charge_are_shifted(): + data = _check_pattern([["D", "C"], ["D", "C"], ["D", "C"]]) + chdr = default_schema().cycle + # [D][C D][C D][C]: the trailing lone charge is kept as its own cycle. + assert data.summary[chdr.cycle_num].to_list() == [1, 2, 3, 4] + assert data.summary[chdr.charge_capacity].to_list()[0] == 0.0 + assert data.summary[chdr.discharge_capacity].to_list()[-1] == 0.0 + + +def test_rest_and_cv_attach_to_current_cycle(): + # Rests never open a cycle; CC + CV charge stay in one cycle. + _check_pattern([["R", "D", "R", "C", "V", "R", "D"], ["R", "C", "V", "D", "R"]]) + + +def test_opening_discharge_mirrors_for_anode_cells(): + # Anode half-cell: a test starting with a lone charge, cycler reports C-D-C. + data = _check_pattern( + [["C", "D", "C"], ["D", "C"]], opening=StepDirection.DISCHARGE + ) + chdr = default_schema().cycle + assert data.summary[chdr.cycle_num].to_list() == [1, 2, 3] + + +def test_opening_accepts_plain_string_and_keeps_zero_based_numbering(): + schema = default_schema() + pattern = [["D", "C"], ["D", "C"]] + raw = _build_raw(pattern, schema.raw, first_cycle=0) + data = _data(raw, schema) + summarizers.renumber_cycles(data, schema, opening="charge") + exp_cycles, _ = _oracle(pattern, first_cycle=0) + steps = data.steps.sort(schema.step.datapoint_num_first) + assert steps[schema.step.cycle_num].to_list() == exp_cycles + assert exp_cycles[0] == 0 + + +def test_energy_columns_are_reaccumulated_too(): + schema = default_schema() + nhdr, chdr = schema.raw, schema.cycle + pattern = [["D", "C"], ["D", "C"]] + raw = _build_raw(pattern, nhdr, with_energy=True) + data = _data(raw, schema) + summarizers.renumber_cycles(data, schema) + _, exp_totals = _oracle(pattern) + # Energy is capacity * 3.7 V in the builder; it must follow the new cycles. + last_rows = ( + data.raw.sort(nhdr.datapoint_num) + .group_by(nhdr.cycle_num, maintain_order=True) + .agg(pl.col(nhdr.cumulative_charge_energy).last()) + ) + for cyc, energy in zip( + last_rows[nhdr.cycle_num], last_rows[nhdr.cumulative_charge_energy] + ): + assert energy == pytest.approx(exp_totals[cyc][0] * 3.7) + summarizers.make_summary(data, schema) + assert chdr.charge_capacity in data.summary.columns + + +def test_multiple_tests_are_renumbered_independently(): + schema = default_schema() + nhdr, shdr = schema.raw, schema.step + p0 = [["D", "C"], ["D", "C"]] + p1 = [["C", "D"], ["C", "D"]] # already correct, 5-based numbering + raw = pl.concat( + [ + _build_raw(p0, nhdr, test_id=0), + _build_raw(p1, nhdr, first_cycle=5, test_id=1, dp0=1000), + ] + ) + data = _data(raw, schema) + summarizers.renumber_cycles(data, schema) + steps = data.steps.sort([shdr.test_id, shdr.datapoint_num_first]) + exp0, _ = _oracle(p0) + exp1, _ = _oracle(p1, first_cycle=5) + assert steps.filter(pl.col(shdr.test_id) == 0)[shdr.cycle_num].to_list() == exp0 + assert steps.filter(pl.col(shdr.test_id) == 1)[shdr.cycle_num].to_list() == exp1 + raw1 = data.raw.filter(pl.col(nhdr.test_id) == 1) + assert raw1[nhdr.cycle_num].unique().sort().to_list() == [5, 6] + + +def test_pandas_in_pandas_out(): + schema = default_schema() + raw = _build_raw([["D", "C"], ["D", "C"]], schema.raw).to_pandas() + data = _data(raw, schema) + summarizers.renumber_cycles(data, schema) + assert isinstance(data.raw, pd.DataFrame) + assert data.raw[schema.raw.cycle_num].max() == 3 + summarizers.make_summary(data, schema, test_mode=config.TestMode.NORMAL) + assert data.summary.height == 3 + + +def test_summary_is_dropped_when_cycles_change(): + schema = default_schema() + raw = _build_raw([["D", "C"], ["D", "C"]], schema.raw) + data = _data(raw, schema) + summarizers.make_summary(data, schema) + summarizers.renumber_cycles(data, schema) + assert data.summary is None + + +def test_requires_raw_and_steps(): + schema = default_schema() + data = Data() + with pytest.raises(NoDataFound): + summarizers.renumber_cycles(data, schema) + data.raw = _build_raw([["C", "D"]], schema.raw) + with pytest.raises(NoDataFound): + summarizers.renumber_cycles(data, schema) + + +def test_rejects_ustep_step_table(): + schema = default_schema() + data = Data() + data.raw = _build_raw([["D", "C"], ["D", "C"]], schema.raw) + summarizers.make_step_table(data, schema, usteps=True) + with pytest.raises(ValueError, match="usteps"): + summarizers.renumber_cycles(data, schema) + + +def test_rejects_unknown_opening(): + schema = default_schema() + data = _data(_build_raw([["C", "D"]], schema.raw), schema) + with pytest.raises(ValueError): + summarizers.renumber_cycles(data, schema, opening="sideways") + + +@pytest.mark.skipif(not HARMONIZED_RAW.exists(), reason="harmonized fixture missing") +def test_harmonized_fixture_noop_and_recount(): + """The anode half-cell fixture is [D C] x 17 + [D] on the cycler counter. + + Opening on discharge matches that counter (byte-identical no-op); opening + on charge regroups it as [D] + [C D] x 17 with capacity conserved. + """ + schema = default_schema() + shdr, chdr = schema.step, schema.cycle + data = Data() + data.raw = pl.read_parquet(HARMONIZED_RAW) + summarizers.make_step_table(data, schema) + raw_before = data.raw.clone() + summarizers.make_summary(data, schema, test_mode=config.TestMode.INVERTED) + summary_before = data.summary + + summarizers.renumber_cycles(data, schema, opening=StepDirection.DISCHARGE) + assert data.raw.equals(raw_before) + + types_before = sorted(data.steps[shdr.step_type].to_list()) + summarizers.renumber_cycles(data, schema, opening=StepDirection.CHARGE) + summarizers.make_summary(data, schema) + summary = data.summary.sort(chdr.cycle_num) + assert summary.height == summary_before.height == 18 + assert summary[chdr.charge_capacity][0] == pytest.approx(0.0, abs=1e-9) + assert sorted(data.steps[shdr.step_type].to_list()) == types_before + for col in (chdr.charge_capacity, chdr.discharge_capacity): + assert summary[col].sum() == pytest.approx(summary_before[col].sum()) + # Every cycle after the first pairs one charge with the following discharge. + first = data.steps.sort(shdr.datapoint_num_first).filter( + pl.col(shdr.step_type).is_in(["charge", "discharge"]) + ) + per_cycle = first.group_by(shdr.cycle_num, maintain_order=True).agg( + pl.col(shdr.step_type) + ) + assert per_cycle[shdr.step_type].to_list()[0] == ["discharge"] + assert all( + seq == ["charge", "discharge"] + for seq in per_cycle[shdr.step_type].to_list()[1:] + )