From 04a23761400c10cc7527179e895f0639a6fd18c1 Mon Sep 17 00:00:00 2001 From: jepegit Date: Sun, 4 Oct 2026 13:35:50 +0200 Subject: [PATCH] test: keep Excel nominal capacity as the sheet number An Ah/g unit row is not applied, so the journal stores the bare cell value and treats it as mAh/g. Co-authored-by: Cursor --- .../03-solved-issues/issue1131_original.md | 7 +++ .../03-solved-issues/issue1131_plan.md | 43 ++++++++++++++++++ .../03-solved-issues/issue1131_status.md | 14 ++++++ .../04-designs-and-guides/test-registry.md | 1 + HISTORY.md | 4 ++ tests/test_dbreader.py | 44 +++++++++++++++++++ 6 files changed, 113 insertions(+) create mode 100644 .issueflows/03-solved-issues/issue1131_original.md create mode 100644 .issueflows/03-solved-issues/issue1131_plan.md create mode 100644 .issueflows/03-solved-issues/issue1131_status.md diff --git a/.issueflows/03-solved-issues/issue1131_original.md b/.issueflows/03-solved-issues/issue1131_original.md new file mode 100644 index 00000000..10ddd3a8 --- /dev/null +++ b/.issueflows/03-solved-issues/issue1131_original.md @@ -0,0 +1,7 @@ +# Issue #1131: possible nominal capacity confusion + +Source: https://github.com/jepegit/cellpy/issues/1131 + +## Original issue text + +Several of the batches I have opened with cellpy-simple-gui have the nominal capacity given in Ah/g. Cellpy-simple-gui thinks it is in mAh/g. And I also think it should be that. Did we have a bug in cellpy earlier that created this (it could be the journal loader)? Might we still have this bug? Or is it just something wrong in the cellpy db (Excel sheet)? Do a thorough check at least to rule out this for the current cellpy version. diff --git a/.issueflows/03-solved-issues/issue1131_plan.md b/.issueflows/03-solved-issues/issue1131_plan.md new file mode 100644 index 00000000..23af9477 --- /dev/null +++ b/.issueflows/03-solved-issues/issue1131_plan.md @@ -0,0 +1,43 @@ +# Issue #1131 — Plan: nominal capacity Ah/g vs mAh/g + +## Goal + +Rule out a current-cellpy bug that would store a nominal capacity whose magnitude is in Ah/g while callers (including cellpy-simple-gui) treat that magnitude as mAh/g. If the Excel journal path is doing that, fix it. If it is not, lock the contract with a test and record the finding on the issue. + +## Constraints + +- Stored `nominal_capacity` is a bare float in `cellpy_units.nominal_capacity`. Default is `mAh/g` (`CellpyUnits` in cellpycore, mirrored in `cellpy.config.models`). +- A bare number on the setter is kept as-is. A quantity string is parsed by `_dump_cellpy_unit`, which stores the magnitude and **replaces** `cellpy_units.nominal_capacity` with the parsed unit. It does not convert into the previous unit. +- cellpy-simple-gui labels gravimetric nominal capacity `mAh/g` in `nomCapUnit` and does not read `cellpy_units`. That label is out of this repo. +- Do not rescale existing Excel databases in this issue. A silent ×1000 would rewrite every sheet whose numbers are already mAh/g. + +### Prior art + +- `Reader.get_nom_cap` in `src/cellpy/readers/dbreader.py` — returns the sheet cell. No unit conversion. +- `Reader._find_out_what_rows_to_skip` — `skiprows.union((self.db_unit_row,))` discards the new set, so the unit row is not added by that line. With the defaults (header 0, unit row 1, data start 2) the unit row is still skipped because it sits before the data start. The unit strings are never applied. +- `JsonReader._convert_nominal_capacity_unit` in `src/cellpy/readers/json_dbreader.py` — when the Unit field matches `[unit]`, converts into `cellpy_units["nominal_capacity"]` (mAh/g). Excel does not do this. +- `batch._dbengine._create_pages_dict` — copies `get_nom_cap` onto journal pages unchanged. +- `CellpyCell.nominal_capacity` setter — `_dump_cellpy_unit` in `src/cellpy/readers/cellreader.py`. +- Instrument `raw_units["nominal_capacity"] = "Ah/g"` on Arbin SQL h5 and Neware loaders is the tester charge unit declaration, not a journal value. +- Toolbox: no helper for this. Graph: units live around `nominal_capacity_as_absolute` (cellpycore). No extra journal-unit node to follow. + +## Approach + +1. Confirm with one Excel fixture: header row, unit row `Ah/g` on the nominal-capacity column, data value `3.5`. `get_nom_cap` must return `3.5`, not `3500`. Journal pages must carry `3.5`. That is the current contract: the sheet number is already in mAh/g; the unit row is documentation cellpy does not read. +2. Confirm the setter: `nominal_capacity = 3.5` leaves the value at `3.5` and leaves `cellpy_units.nominal_capacity` at `mAh/g`. A quantity string is recorded as a finding, not changed here. +3. No production change when step 1 matches the code as read. Comment on GitHub #1131 with the three paths (Excel: number kept; JSON: `[Ah/g]` converted to mAh/g; GUI: hardcoded mAh/g label). +4. Do not touch cellpy-simple-gui, BatBase JSON conversion, or instrument `raw_units`. + +## Files to touch + +- `tests/test_dbreader.py` — tiny xlsx (or openpyxl workbook in tmp) whose unit row says `Ah/g` and whose nominal capacity is `3.5`. Assert `get_nom_cap` returns `3.5`. +- No change to `src/cellpy/readers/dbreader.py` unless the test shows the value is rescaled. + +## Test strategy + +`uv run pytest tests/test_dbreader.py -q` from the worktree. No full suite for this lock-in test. + +## Open questions + +- Honor the Excel unit row and convert `Ah/g` → `mAh/g`? Recommended: no. The unit row has never been applied, and converting now would rescale sheets that already store mAh/g. If a real sheet's unit row says `Ah/g` and the numbers are in that unit, the sheet should be rewritten to mAh/g (or a later, explicit converter). +- Fix `_dump_cellpy_unit` so `"3.5 Ah/g"` becomes `3500` mAh/g instead of storing `3.5` and relabeling the unit? Recommended: no in this issue. The reported path is the journal/Excel loader, which passes a bare number. diff --git a/.issueflows/03-solved-issues/issue1131_status.md b/.issueflows/03-solved-issues/issue1131_status.md new file mode 100644 index 00000000..634786e4 --- /dev/null +++ b/.issueflows/03-solved-issues/issue1131_status.md @@ -0,0 +1,14 @@ +# Issue #1131 — status + +- [x] Done + +## What's done + +- Captured on branch `1131-nominal-capacity-units` (worktree `cellpy-1131`). +- Plan accepted: lock the Excel contract (unit row ignored; bare number is mAh/g). No rescale. +- `tests/test_dbreader.py::test_excel_unit_row_does_not_rescale_nominal_capacity` passes (`uv run pytest` that node, 1 passed). +- Finding commented on the issue: https://github.com/jepegit/cellpy/issues/1131#issuecomment-5979452832 + +## Remaining work + +- None. Essential suite: 986 passed, 74 skipped. diff --git a/.issueflows/04-designs-and-guides/test-registry.md b/.issueflows/04-designs-and-guides/test-registry.md index dc2f0cdc..79a99a07 100644 --- a/.issueflows/04-designs-and-guides/test-registry.md +++ b/.issueflows/04-designs-and-guides/test-registry.md @@ -225,6 +225,7 @@ current issue**. `/iflow-doctor` may audit the whole suite against this table. | tests/test_batch_live.py::test_poll_stops_on_cell_error | yes | yes | Batch.poll | #782 | | | tests/test_dbreader.py::test_missing_column_warns_once | yes | yes | readers.dbreader.Reader._pick_info | #1008 | warn-once per missing header | | tests/test_dbreader.py::test_nom_cap_specifics_column_reaches_pages | yes | yes | batch._dbengine._create_pages_dict | #1008 | db value → pages | +| tests/test_dbreader.py::test_excel_unit_row_does_not_rescale_nominal_capacity | yes | yes | readers.dbreader.Reader.get_nom_cap | #1131 | Ah/g unit row ignored; 3.5 stays 3.5 | | tests/test_dbreader.py::test_simple_db_engine_skip_file_search_excel_reader | yes | yes | batch._dbengine.simple_db_engine / find_files | #1017 | skip_file_search frames one row per cell | | tests/test_batch.py::test_find_files_skip_file_search_pads_missing_columns | no | – | batch._dbengine.find_files | #1017 | unit detail; engine test covers the gate | | tests/test_batch_v3_facade.py::test_load_warns_when_journal_autoload_shadows_db | yes | yes | batch.facade.load | #1008 | cached journal + db args | diff --git a/HISTORY.md b/HISTORY.md index 073da173..5ad27796 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -2,6 +2,10 @@ ## [Unreleased] +* Possible nominal capacity confusion. The Excel unit row is not applied, so a + bare nominal-capacity cell stays the number on the sheet and is treated as + mAh/g. (#1131) + ## [2.1.5.post7] - 2026-10-03 * Summary facet rows keep a shared cycle axis when each row has its own diff --git a/tests/test_dbreader.py b/tests/test_dbreader.py index a819ea2d..76381dd9 100644 --- a/tests/test_dbreader.py +++ b/tests/test_dbreader.py @@ -205,6 +205,50 @@ def test_missing_column_warns_once(db_reader): assert db_reader.get_nom_cap_specifics(test_serial_number_two) is None +@pytest.mark.essential +def test_excel_unit_row_does_not_rescale_nominal_capacity(tmp_path): + """An Ah/g unit row is not applied; the sheet number stays as stored (#1131). + + cellpy treats a bare nominal capacity as already in ``mAh/g``. The Excel + unit row is skipped and never used to convert the value. + """ + import warnings + + from openpyxl import Workbook + + from cellpy import config + from cellpy.batch import _dbengine + from cellpy.readers import dbreader + + path = tmp_path / "nom_cap_units.xlsx" + book = Workbook() + sheet = book.active + sheet.title = config.db.db_table_name + rows = [ + [config.db_cols.id, config.db_cols.nom_cap], + ["", "Ah/g"], + [7, 3.5], + ] + # Defaults: header row 0, unit row 1, data start row 2. + assert config.db.db_header_row == 0 + assert config.db.db_unit_row == 1 + assert config.db.db_data_start_row == 2 + for row in rows: + sheet.append(row) + book.save(path) + + reader = dbreader.Reader(db_file=path) + assert reader.get_nom_cap(7) == pytest.approx(3.5) + assert reader.get_nom_cap(7) != pytest.approx(3500) + + # Other journal columns are absent on purpose; their missing-column + # warnings are #1008, not this contract. + with warnings.catch_warnings(): + warnings.simplefilter("ignore", UserWarning) + pages = _dbengine._create_pages_dict(reader, [7]) + assert pages["nom_cap"] == pytest.approx([3.5]) + + @pytest.mark.essential def test_nom_cap_specifics_column_reaches_pages(db_reader): """A present specifics column flows into the journal pages dict (#1008)."""