#560: re-land pec_csv two-stage port (lost in #605 stacked-merge)#607
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
1 participant
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.
Re-lands the
pec_csvtwo-stage port. The original #605 was marked merged but its changes never reached master — it was a stacked PR whose base was #602's branch, and when #602 squash-merged, #605's diff merged into the intermediate branch rather than master.git merge-base --is-ancestorconfirms #605's merge commit is not on master, andpec_csv.pyon master has noparse().This is the same commit, cherry-picked cleanly onto current master (which now has #602, #603, #604, #606). Opened against master so CI actually runs this time.
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. Design decisions in the plan (§2.6a).parse()reads → matches alias sets → renames matched columns to canonical vendor-neutral names → 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. The parity oracle catches exactly that.declarations()reusesderive_column_maps(inverting_COLUMN_KEY_TO_CELLPY_HEADER, composing with cellpy-core's legacy→native map) rather than hand-transcribing — so pec inherits the provenance rule and energy mapping from the same code the config loaders use. Only the passthrough set is per file.None, which was read as a mismatch (and would have hidden a real values-vs-null diff). The comparison now checks null masks first, then non-null values. Mutation-checked both ways.pec reaches value parity on every mapped and passthrough column (only exclusion: the shared
date_timegap). Five parity cases;tests/test_pec_two_stage.pypins the decisions. Full suite underPYTHONUTF8=1: 1243 passed, 0 failed.🤖 Generated with Claude Code