arbin_res was the tier-1 loader flagged as awkward - Access .res via ODBC on (#608)

Author: jepegit

cellpy/readers/instruments/arbin_res.py

@@ -43,7 +43,7 @@
 import sqlalchemy as sa
 
 from cellpy import prms
-from cellpy.exceptions import NullData
+from cellpy.exceptions import LoaderError, NullData
 from cellpy.parameters.internal_settings import HeaderDict, get_headers_normal
 from cellpy.readers.data_structures import (
     Data,
@@ -817,6 +817,79 @@ def _check_size(self):
             return False
         return True
 
+    def parse(self, source, **kwargs):
+        """Vendor stage (#560): read the .res into a frame with **Arbin** names.
+
+        The two-stage counterpart to :meth:`loader`, and the point where the
+        vendor part of arbin ends: reading the Access database (ODBC on Windows,
+        mdbtools on posix) and assembling the normal data table. Everything
+        after — the rename to native columns, the datetime conversion — is
+        declared and handled by ``harmonize()``.
+
+        This is exactly what ``loader()`` produces before ``_post_process``, so
+        the two share one read path and cannot drift. Scoped to a single test:
+        multi-test ``.res`` files are the switchover's concern, not the vendor
+        stage's (``loader()`` still splits and merges them).
+
+        Returns:
+            The normal table with Arbin column names — ``DateTime`` still an
+            Excel serial, capacities as the file stores them.
+        """
+        import polars as pl
+
+        self.name = source
+        self.copy_to_temporary()
+
+        use_mdbtools = _use_subprocess or _is_posix
+        if use_mdbtools:
+            data = self._loader_posix(
+                self.name,
+                self.temp_file_path,
+                self.temp_file_path.parent,
+                **kwargs,
+            )
+        else:
+            data = self._loader_win(self.name, self.temp_file_path, **kwargs)
+
+        self._parsed = True
+        return pl.from_pandas(data.raw.reset_index(drop=True))
+
+    def declarations(self):
+        """Declarations for arbin's normal table (#560).
+
+        Derived, not hand-written: ``get_headers_normal()`` is already a
+        ``{cellpy attr → Arbin column}`` map — the same shape the configuration
+        loaders carry as ``normal_headers_renaming_dict`` — so inverting it and
+        composing with cellpy-core's legacy→native map (via
+        ``derive_column_maps``) gives Arbin → native and the provenance rule for
+        free (``Test_ID`` is not mapped onto the framework ``test_id``).
+        """
+        from cellpycore.units import CellpyUnits
+
+        from cellpy.readers.instruments.config_declarations import derive_column_maps
+        from cellpy.readers.instruments.declarations import LoaderDeclarations
+
+        if not getattr(self, "_parsed", False):
+            raise LoaderError(
+                "arbin_res.declarations() was called before parse()."
+            )
+
+        # cellpy attr -> Arbin column is what get_headers_normal() returns;
+        # derive_column_maps wants attr -> vendor, which is the same thing.
+        renaming = dict(self.arbin_headers_normal)
+        column_map, passthrough, _ = derive_column_maps(renaming)
+
+        raw_units = {
+            key: value
+            for key, value in self.get_raw_units().items()
+            if hasattr(CellpyUnits(), key)
+        }
+        return LoaderDeclarations(
+            column_map=column_map,
+            raw_units=CellpyUnits(**raw_units),
+            passthrough=passthrough,
+        )
+
     def loader(
         self,
         name,

tests/test_arbin_two_stage.py

@@ -0,0 +1,109 @@
+"""arbin_res's two-stage entry points (#560).
+
+arbin_res reads an Access ``.res`` database — ODBC on Windows, mdbtools on
+posix — so these tests skip where no backend is available, exactly as the
+loader golden does. Where a backend exists (CI has mdbtools), they run.
+
+The vendor stage is unusually thin here: ``get_headers_normal()`` is already a
+``{cellpy attr → Arbin column}`` map, so ``declarations()`` reuses the same
+derivation the configuration loaders use. What ``parse()`` adds is only the
+database read, stopping before the rename and the datetime conversion that
+``harmonize()`` now owns.
+"""
+
+from __future__ import annotations
+
+import warnings
+
+import polars as pl
+import pytest
+
+from cellpy.exceptions import LoaderError
+from cellpy.readers import data_structures as ds
+
+SOURCE = "testdata/data/20160805_test001_45_cc_01.res"
+
+
+def _loader():
+    return ds.generate_default_factory().create("arbin_res")
+
+
+def _parsed_or_skip():
+    loader = _loader()
+    try:
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore")
+            vendor = loader.parse(SOURCE)
+    except Exception as exc:  # noqa: BLE001 — unavailable backend, not a bug
+        pytest.skip(f"arbin_res backend unavailable here: {exc}")
+    return loader, vendor
+
+
+@pytest.mark.essential
+def test_parse_returns_arbin_vendor_names():
+    _, vendor = _parsed_or_skip()
+
+    # Arbin's own spellings, not native — renaming is harmonize's job.
+    for arbin in ("Voltage", "Current", "Charge_Capacity", "Cycle_Index"):
+        assert arbin in vendor.columns, f"{arbin} missing from the parsed frame"
+    assert "potential" not in vendor.columns
+
+
+@pytest.mark.essential
+def test_declarations_derive_from_get_headers_normal():
+    loader, _ = _parsed_or_skip()
+    declarations = loader.declarations()
+
+    assert declarations.column_map["Voltage"] == "potential"
+    assert declarations.column_map["Current"] == "current"
+    assert (
+        declarations.column_map["Charge_Capacity"] == "cumulative_charge_capacity"
+    )
+
+
+@pytest.mark.essential
+def test_the_vendor_test_id_is_not_mapped_onto_the_framework_test_id():
+    """``Test_ID`` is provenance; mapping it onto native ``test_id`` (the
+    grouping key) would let the vendor value win. The shared derivation drops
+    it, same as for the configuration loaders."""
+    loader, _ = _parsed_or_skip()
+    declarations = loader.declarations()
+
+    assert "Test_ID" not in declarations.column_map
+    assert "test_id" not in declarations.column_map.values()
+
+
+@pytest.mark.essential
+def test_declarations_before_parse_raises():
+    loader = _loader()
+    with pytest.raises(LoaderError, match="before parse"):
+        loader.declarations()
+
+
+@pytest.mark.essential
+def test_two_stage_capacities_match_the_legacy_frame():
+    """The assertion the switchover rests on, through the public entry points."""
+    import cellpy
+
+    loader, vendor = _parsed_or_skip()
+    from cellpy.readers.instruments.harmonize import harmonize
+
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        raw = harmonize(vendor, loader.declarations(), strict=False)
+        cell = cellpy.get(SOURCE, instrument="arbin_res", mass=1.0, testing=True)
+
+    legacy = pl.from_pandas(cell.data.raw.reset_index(drop=True))
+    for column in (
+        "potential",
+        "current",
+        "cumulative_charge_capacity",
+        "cumulative_discharge_capacity",
+        "test_time",
+    ):
+        difference = (
+            raw[column].cast(pl.Float64) - legacy[column].cast(pl.Float64)
+        ).abs().max()
+        assert difference is not None and difference < 1e-9, (
+            f"{column} differs by {difference}"
+        )

tests/test_loader_port_parity.py

@@ -52,6 +52,10 @@
     # pec_csv is not configuration-driven; its parse()/declarations() are
     # hand-written (canonical rename in parse, static column map). Plan §2.6a.
     ("pec_csv", "testdata/data/pec.csv", {}),
+    # arbin_res reads an Access .res database (ODBC on Windows, mdbtools on
+    # posix). CI has mdbtools, so this runs there; environments without a
+    # backend skip gracefully (see _skip_if_unloadable), like the golden.
+    ("arbin_res", "testdata/data/20160805_test001_45_cc_01.res", {}),
 )
 
 #: Legacy post-processor → native columns it changes, for the ones that have no
@@ -94,8 +98,8 @@ def _parse_with_a_loader(instrument, source, kwargs):
     with warnings.catch_warnings():
         warnings.simplefilter("ignore")
         vendor = loader.parse(source, **parse)
-    # config_params only exists on configuration-driven loaders; pec_csv has
-    # none, and only the post-processor excusal (_excused) reads it.
+    # config_params only exists on configuration-driven loaders; arbin_res and
+    # pec_csv have none, and only the post-processor excusal reads it.
     return vendor, loader.declarations(), getattr(loader, "config_params", None)
 
 
@@ -139,9 +143,30 @@ def _excused(config_params) -> set[str]:
     return excused
 
 
+def _skip_if_unloadable(instrument, source, kwargs):
+    """Skip when the loader's backend is unavailable, as the golden does.
+
+    Only ``arbin_res`` needs it: its Access-database backend (ODBC/mdbtools) is
+    not present everywhere. A missing backend is an environment fact, not a
+    parity failure, so it skips rather than fails — matching
+    ``LoaderGoldenSpec.skip_reason``.
+    """
+    import cellpy
+
+    try:
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore")
+            cellpy.get(
+                source, instrument=instrument, mass=1.0, testing=True, **kwargs
+            )
+    except Exception as exc:  # noqa: BLE001 — an unavailable backend, not a bug
+        pytest.skip(f"{instrument} loader unavailable here: {exc}")
+
+
 def _harmonized_and_legacy(instrument, source, kwargs):
     import cellpy
 
+    _skip_if_unloadable(instrument, source, kwargs)
     vendor, declarations, config_params = _parse_with_a_loader(
         instrument, source, kwargs
     )