Issue 4884 unify hysteresis #4893
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@ejfdickinson would appreciate your feedback on this |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4893 +/- ##
===========================================
- Coverage 98.71% 98.68% -0.03%
===========================================
Files 304 305 +1
Lines 23495 23479 -16
===========================================
- Hits 23193 23171 -22
- Misses 302 308 +6 ☔ View full report in Codecov by Sentry. |
|
@rtimms I'll look over it. Relating to #4884 incomplete items currently excluded from the PR:
|
There was a problem hiding this comment.
In addition to specific comments on the docs, one additional comment:
- Should we use this as an opportunity to move from arbitrary author-name definitions to more descriptive inputs options, e.g.
"Axen" -> "one-state"
"Wycisk" -> "one-state differential capacity"
and similarly for module names? Although they may be the original reports, I don't think that "Axen" or "Wycisk" are really that intuitively descriptive for what are actually quite general hysteresis descriptions.
| "source": [ | ||
| "## Model Equations\n", | ||
| "\n", | ||
| "Herein, we outline the equations for the \"Curret Sigmoid\" model, as described in Ai et al. (2022), the Differential Capacity Hysteresis State open-circuit potential model, as described in Wycisk (2022), and the Hysteresis State open-circuit potential model, as described in Axen (2022).\n", |
| "\n", | ||
| "$$U = \\frac{1+h}{2} U_{delith} + \\frac{1-h}{2} U_{lith},$$\n", | ||
| "\n", | ||
| "where $h$ is a hysteresis state variable, $U_{delith}$ is the delithiation open-circuit potential, and $U_{lith}$ is the lithiation open-circuit potential. The hysteresis state variable $h$ is defined between -1 and 1, where -1 corresponds to the delithiation branch and 1 corresponds to the lithiation branch.\n", |
There was a problem hiding this comment.
Definition of
| "\n", | ||
| "In all of the models, the open-circuit potential is given by\n", | ||
| "\n", | ||
| "$$U = \\frac{1+h}{2} U_{delith} + \\frac{1-h}{2} U_{lith},$$\n", |
There was a problem hiding this comment.
Prefer throughout to use Roman (non-italic) text for textual subscripts.
| "$$ U = \\text{sigmoid}\\left(-\\frac{KI}{Q_{cell}}\\right) U_{delith} + \\text{sigmoid}\\left(\\frac{KI}{Q_{cell}}\\right) U_{lith}\n", | ||
| "$$\n", | ||
| "\n", | ||
| "Where $K$ is a fitting parameter, $I$ is the cell current, and $Q_{cell}$ is the cell capacity. To simplify the comparison with the other models, we can rewrite this expression in terms of the hysteresis state variable $h$ given by\n", |
There was a problem hiding this comment.
I suggest to use another symbol for
Since the current sigmoid '$K$' is hardcoded in the implementation, quote the value here and perhaps consider the current lower bound (C/1000? C/1e6?) at which the current sigmoid implementation will noticeably no longer obey the intended relation, due to the finite step window.
| "\n", | ||
| "### Hysteresis State Variable (Axen)\n", | ||
| "In this model, the state variable $h(z,t)$ is both stoichiometry and time-dependent, and is governed by the following ODEs:\n", | ||
| "$$ \\frac{dh(z,t)}{dt} = K_{lith} \\, \\frac{I_{cell}}{Q_{cell}} \\, (1 - h(z,t)) \\qquad \\text{for lithiation}$$\n", |
There was a problem hiding this comment.
Even if this is how it's presented in the original literature, it's inherently confusing that the implied range of
I would recommend that we use this document to collectively paraphrase all the original literature models into a common notation and definition. Do not worry about exactly how the original authors approached symbols or factorisation. The main point of this document should be for PyBaMM users to distinguish the various model options, and understand their basis in scientific literature where appropriate.
Description
Unifies the hysteresis models in PyBaMM and adds heat generation due to hysteresis. In particular:
BaseHysteresisOpenCircuitPotentialclass that sets variables for the lithiation and delithiation OCP, as well as the average (simply the mean), and the hysteresis voltage (H = U_lith - U_delith).CurrentSigmoidOpenCircuitPotential, are expressed in terms of a hysteresis state variable-1<h<1Q_hys = i_vol * (U - U_eq)wherei_volis the volumetric interfacial current density,Uis the OCP (i.e. includes hysteresis), andU_eqis the "equilibrium OCP" (currently hard-coded to be the mean of the delithaiton and lithiation branches)differential-capacity-hysteresis-state.ipynbtohysteresis-state-models.ipynband extends it to include a comparison of all of the existing hysteresis modelsRelated #4884
What this doesn't do from #4884:
\gammain the subclasses. This would be an easy change to make, but I'm not sure it is much harder to just specify the wholerhsin the base class and it makes the code more readable (even if there is some repetition). The way it currently is, you see the whole ODE in theset_rhsmethod and don't need to chase back to the base class. Happy to take input on this."... electrode OCP [V]"parameter to use as the equilibrium OCP and compute it as the mean of the two branches if unspecified, so that the breaking change arises only when it was actively specified. We don't currently have cases of having defaults for missing parameters, and my experience in the past is that it is better to be explicit. My preference would be either 1) always require"... electrode OCP [V]"to be provided, or 2) compute it as the mean. The current implementation does the latter.Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests