#560: port pec_csv to the two-stage design (canonical rename in parse)#605
Merged
Merged
Conversation
pec_csv is not configuration-driven - it identifies columns by alias set and carries a different unit per column in the header - so its parse()/declarations() are hand-written rather than derived. loader() is untouched; this adds the two stages beside it. parse() reads the file, matches the alias sets, renames matched columns to canonical vendor-neutral names (voltage, current, ...), and scales each column to its canonical unit. That last step is not optional: harmonize() has no per-column unit scaling, so leaving a mV column unscaled would tag it as V - silent corruption. The parity oracle catches exactly that (a mutation removing the scaling shows a 3795 mV diff on potential). declarations() does not hand-transcribe the canonical->native map. It reuses derive_column_maps by inverting _COLUMN_KEY_TO_CELLPY_HEADER and composing with cellpy-core's legacy->native map, so pec gets the provenance rule (vendor `test` dropped rather than mapped onto the framework test_id) and the 0.2.3 energy mapping for free - the same code the config loaders use. Only the passthrough set is per file: the export's other columns (temperatures, OCV, peak power) are kept under their sanitised names, matching loader(). Building it surfaced a false negative in the parity oracle, now fixed: pec carries sparse per-measurement columns that are all-null in the fixture, and the numeric diff of two all-null columns is None, which the oracle counted as a mismatch. That rule would also have hidden a real values-vs-null difference. The comparison now checks null masks first, then non-null values. Mutation-checked both directions (the neware PER_STEP mutant still fails; an all-null column agrees). pec reaches value parity on every mapped and passthrough column; the only exclusion is the shared date_time representation gap. tests/test_pec_two_stage.py pins the decisions (units normalised in parse, static canonical->native map, passthroughs preserved, provenance dropped). Full suite under PYTHONUTF8=1: 1205 passed, 0 failed. Loader plan §2.6a updated with the execution record. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #560. Stacked on #602 — review that first; this diff is against its branch.
What
pec_csvgetsparse()anddeclarations()beside its existingloader(), which is untouched. It was the one loader that couldn't be handled mechanically: aBaseLoader(noconfig_params), columns identified by alias set rather than fixed strings, and a different unit per column embedded in the header (Voltage (V)vs(mV)).Design decisions and their rationale are in the plan (§2.6a); the pivotal one — canonical rename in
parse()→ static declarations — was a maintainer call.How
parse()reads → matches alias sets → renames matched columns to canonical vendor-neutral names (voltage,current, …) → scales each to its canonical unit → shifts 0-based cycles. The unit scaling is not optional:harmonize()has no per-column unit scaling, so an unscaled mV column would be tagged as V. Silent corruption — and the parity oracle catches it (a mutation removing the scaling shows a 3795 mV diff onpotential).declarations()does not hand-transcribe the canonical→native map. It reusesderive_column_mapsby inverting_COLUMN_KEY_TO_CELLPY_HEADERand composing with cellpy-core's legacy→native map — so pec inherits the provenance rule (vendortestdropped, not mapped onto the frameworktest_id) and the 0.2.3 energy mapping from the same code the config loaders use. Only the passthrough set is per file: the export's other columns (temperatures, OCV, peak power) are preserved under sanitised names, matchingloader().A false negative it fixed in the oracle
pec carries sparse per-measurement columns that are all-null in the fixture (
cell_id,station_temperature_degc, the internal resistances). The numeric diff of two all-null columns isNone, which the oracle had been counting as a mismatch — and which would equally have hidden a real values-vs-null difference. The comparison now checks null masks first, then non-null values. Mutation-checked both ways: the newarePER_STEPmutant still fails; an all-null-vs-all-null column agrees.Result
date_timerepresentation gap.tests/test_pec_two_stage.pypins the decisions directly.PYTHONUTF8=1: 1205 passed, 0 failed.Next on #560
arbin_res(ODBC — CI can only check the committed fixture) and the tier-2 arbin_sql family, then the switchover. Config-drivenmaccor_txt_zeroandbatmo_bdf_bdfremain without parity coverage.🤖 Generated with Claude Code