#566: one pure ICA core, IcaOptions, a specced output frame, and dvdq - #590
Conversation
The dQ/dV recipe is a chain of order-sensitive operations (interpolate V(q) -> optional pre-smooth -> invert to q(V) -> optional smooth -> differentiate -> optional gaussian -> normalize). Reordering any two of them still yields a plausibly-shaped curve, so shape assertions would not catch a botched port. These eight suites record values. Every case goes through the *old* public entry points on purpose. After the redesign those become deprecation shims over the new core, and `regenerate_goldens.py --verify` then shows the shims are bit-identical. Verified the net bites: raising the Converter's default voltage_fwhm from 0.01 to 0.02 turns 10 of the 16 tests red. (The 6 that stay green are the dqdv_np cases, which carry their own voltage_fwhm default and so never read the Converter's.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces four entry points x three ways of passing the same ~20 options with
two verbs over one stateless core.
cellpy/ica.py new home (cellpy.utils.ica re-exports it)
IcaOptions one frozen recipe object, validated at construction
transform_half_cycle(voltage, capacity, options, derivative=...)
the pure core; public, since it is the honest
replacement for dqdv_np
dqdv() / dvdq() same sources (cell / curve frame / two arrays),
same options, symmetric signatures
to_wide() wide format as an explicit conversion, not a mode
dvdq is new capability. cellpy could not compute differential voltage
analysis at all - some loaders ingested a dv_dq column when the cycler
exported one, but nothing derived it. It needs no q(V) inversion, so it is
the simpler of the two derivatives: ~30 lines on the existing pipeline.
The specced output frame is the part that had to happen in 2.0 rather than
2.1, because users index the returned columns directly:
dqdv -> cycle, direction, voltage, capacity, dqdv
dvdq -> cycle, direction, capacity, voltage, dvdq
`direction` is now spelled "charge"/"discharge". It used to be the raw +-1
code from get_cap, whose meaning flips with cycle_mode - the reader was left
to reconstruct it. `dq` is kept as a duplicate of `dqdv` for one release.
Also fixed on the way through:
- `from scipy.ndimage.filters import ...` (deprecated alias, already warning
on scipy 1.18) -> `scipy.ndimage`.
- inspect_data wrote the derived normalizing factor back onto the converter,
so a reused converter carried one half-cycle's normalization into the next.
The core returns it instead; there is a determinism test for it.
- A failing half-cycle was replaced by empty arrays and a log line. Failures
are now collected, warned about, and recorded in frame.attrs; strict=True
raises.
- The ValueError-retry-without-post-smoothing was copy-pasted per half-cycle;
it is one path now.
- The savgol window arithmetic existed twice, identically.
Numerics are unchanged: the 1.x entry points are reimplemented on the new
core and reproduce all eight golden suites bit-for-bit, split path and wide
path included. `regenerate_goldens.py --verify` is the check.
The 19 characterization tests still pass; three needed updating and each is
a real contract change: dqdv_np(None, None) now raises NullData rather than
TypeError from an assert, and the specced frame has one row fewer per cycle
because it drops the NaN "splitter" row that 1.x inserted so naive line
plots would not join the two half-cycles.
Deferred rather than guessed at: `ica_collector` still builds the 1.x frame.
`ica_plotter` selects `direction < 0` and calls it "charge", but get_cap
gives -1 to the first half-cycle, which for the default cycle_mode="anode"
is the cell discharge. Electrode-centric vs cell-centric - both defensible,
and picking one would silently flip the labels on every batch ICA plot.
Needs a maintainer decision.
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. |
My own bug, caught by CI on Linux. The metrics file rounds each column sum to six decimals for readability, and the test then compared those rounded values with `==`. Rounding does not make a float comparable across platforms - a total that lands near a rounding boundary still flips - and one did: 122.614595 on Linux against a golden of 122.614594 recorded on Windows. Each sum runs over hundreds to thousands of floats, so the last bit of the total legitimately differs. Compared with rel=1e-9 now, and the frame comparison tightened from pandas' 1e-5 default to rtol=1e-8 (measured cross-platform noise is ~5e-10 per value). Checked this did not blunt the oracle: perturbing the processed voltage by a factor of 1.0000001 - a 1e-7 relative change, ~100x smaller than the noise being tolerated - turns all 16 red. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
My previous fix was another guess, and it was wrong in the other direction. I derived "~5e-10 per value" from how much the column *sums* moved, but sums average errors out - individual values move much more. Tightening the frame comparison to rtol=1e-8 on that basis made all 16 fail on Linux. Measured from the CI log instead: scipy's interpolation and filtering differ between Windows and Linux by 1e-7 to 5e-7 relative on this data (258.2250118395604 vs 258.2250426021722 at the first point). So: - frames: back to pandas' 1e-5 default, which leaves 20-100x headroom over that noise - metric sums: rel=1e-5 to match This also retires a claim I made in the previous commit. The 1.0000001 perturbation I used to "prove" the oracle was sharp was itself inside the platform noise band - it only failed because I ran it on the machine the goldens came from. Re-checked at 1e-4, above the tolerance: all 16 red. Two standards, now written down in the goldens README: `--verify` regenerates twice on one machine and demands byte-identical files; the pytest comparison runs against someone else's machine and uses a tolerance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Correction to the PR description above. I claimed the shims reproduce the 1.x numbers "bit-for-bit". That is true only on a single machine —
Measured properly from the CI log: scipy's interpolation and filtering differ between Windows and Linux by 1e-7 to 5e-7 relative on this data ( This also retires the sensitivity check I quoted in the second commit: the So the accurate claim is: byte-identical on one machine, and equal to within ~1e-5 relative across platforms — which is still far tighter than any change to the recipe would produce. |
Closes #566.
Replaces four entry points × three ways of passing the same ~20 options with two verbs over one stateless core.
cellpy/ica.pycellpy.utils.icare-exports itIcaOptionstransform_half_cycle(voltage, capacity, options, derivative=...)dqdv_npdqdv()/dvdq()to_wide()dvdq is new capability
cellpy could not compute differential voltage analysis at all. Some loaders ingested a
dv_dqcolumn when the cycler exported one, but nothing derived it from the curves. It needs no q(V) inversion — it differentiates the smoothed V(q) representation the pipeline already builds — so it is the simpler of the two derivatives, about 30 lines on the existing machinery.The specced frame is why this is a 2.0 item
Users index the returned columns directly, so the output schema is a data contract:
directionis now spelled"charge"/"discharge". It used to be the raw ±1 code fromget_cap, whose meaning flips withcycle_mode— the reader was left to reconstruct it.dqis kept as a duplicate ofdqdvfor one release.Fixed on the way through
from scipy.ndimage.filters import ...— deprecated alias, already warning on scipy 1.18 →scipy.ndimage.inspect_datawrote the derived normalizing factor back onto the converter, so a reused converter carried one half-cycle's normalization into the next. The core returns it instead; there is a determinism test for exactly this.frame.attrs;strict=Trueraises.Numerics are unchanged, and that is checked
The 1.x entry points are reimplemented on the new core, not kept as a second copy of the math — which is what makes the oracles meaningful. All eight golden suites recorded against the 1.x code reproduce bit-for-bit, split path and wide path included:
Those goldens landed first, in their own commit, and I checked they bite: raising the default
voltage_fwhmfrom 0.01 to 0.02 turns 10 of 16 red.Test changes
The 19 characterization tests still pass. Three needed updating, and each is a real contract change rather than a test fixup:
dqdv_np(None, None)now raisesNullDatainstead of aTypeErrorfrom anassert len(...).directioncolumn makes unnecessary.New: 42 API-contract tests.
dvdqis verified against closed-form derivatives of a line and a parabola (no shared implementation), plus a reciprocity check againstdqdvon a real half-cycle.Deliberately not done
ica_collectorstill builds the 1.x frame.ica_plotterselectsdirection < 0and calls it "charge", butget_capgives -1 to the first half-cycle, which for cellpy's defaultcycle_mode="anode"is the cell discharge. Electrode-centric vs cell-centric — both defensible, and picking one silently flips the labels on every batch ICA plot. That wants a maintainer decision, so the collector calls the private legacy builder (no user-facing warning from inside a collector) with a comment explaining why.Also still open from the plan's §6: the fate of the never-finished
"hist"binning method (quarantined in the deprecatedConverter; the newIcaOptionsrejects it),normalize="nom_cap", and full-cell support.🤖 Generated with Claude Code