Issue 4884 unify hysteresis

[rtimms]

Description

Fixes #5038 #5081 #5021

Unifies the hysteresis models in PyBaMM and adds heat generation due to hysteresis. In particular:

• Adds a new BaseHysteresisOpenCircuitPotential class 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).
• Makes sure all models, even CurrentSigmoidOpenCircuitPotential, are expressed in terms of a hysteresis state variable -1<h<1
• Allows the initial hysteresis state to be a function of position through the electrode
• Allows the hysteresis decay rates of the Axen and Wycisk models to be a function of stoichiometry and temperature
• Adds a heat source term in each active material phase Q_hys = i_vol * (U - U_eq) where i_vol is the volumetric interfacial current density, U is the OCP (i.e. includes hysteresis), and U_eq is the "equilibrium OCP"
• Renames the notebook differential-capacity-hysteresis-state.ipynb tohysteresis-state-models.ipynb and extends it to include a comparison of all of the existing hysteresis models

Related #4884

What this doesn't do from #4884:

• Reformulate the Wycisk model to use $\gamma = K \cdot \left(\frac{\frac{\partial q_\mathrm{vol}}{\partial U}}{\left.\frac{\partial q_\mathrm{vol}}{\partial U}\right|_\mathrm{ref}}\right)^{-n}$. This would be breaking as it requires rescaling parameters. Is this a breaking change worth making? Changes to parameters have, in the past, been problematic/easy for users to miss.
• Express all the models simply as $\frac{\partial h}{\partial t} = \gamma\cdot\left(\frac{i_\mathrm{surf}a_\mathrm{vol}}{\phi_\mathrm{act}Fc_\mathrm{max}}\right)\cdot \left(1 - \mathrm{sgn}(i_\mathrm{surf})h\right)$ using the base class, and then specify \gamma in the subclasses. This would be an easy change to make, but I'm not sure it is much harder to just specify the whole rhs in 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 the set_rhs method and don't need to chase back to the base class. Happy to take input on this.

Breaking

• Need to explicitly give the equilibrium, delithiation, and lithiation OCPs when using a hysteresis model. E.g. you must provide all three of "Negative electrode OCP [V]", "Negative electrode delithiation OCP [V]", and "Negative electrode lithiation OCP [V]"

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:

• No style issues: nox -s pre-commit
• All tests pass: nox -s tests
• The documentation builds: nox -s doctests
• Code is commented for hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[rtimms]

@ejfdickinson would appreciate your feedback on this

[codecov[bot]]

Codecov Report

:x: Patch coverage is 97.76119% with 6 lines in your changes missing coverage. Please review. :warning: Please upload report for BASE (develop@273ccd4). Learn more about missing BASE report. :warning: Report is 155 commits behind head on develop.

Files with missing lines	Patch %	Lines
...m/models/full_battery_models/base_battery_model.py	83.78%	6 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             develop    #4893   +/-   ##
==========================================
  Coverage           ?   99.09%           
==========================================
  Files              ?      306           
  Lines              ?    23659           
  Branches           ?        0           
==========================================
  Hits               ?    23446           
  Misses             ?      213           
  Partials           ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ejfdickinson]

@rtimms I'll look over it.

Relating to #4884 incomplete items currently excluded from the PR:

• If the breaking change to introduce a reference value is unappetising (fair enough), I recommend that we consider this a "specification bug fix" where we redefine from

  $\left(\frac{\partial q_\mathrm{vol}}{\partial U}\right)^{-n}$

  to

  $\left(\frac{\frac{\partial q_\mathrm{vol}}{\partial U}}{C_\mathrm{ref,vol}}\right)^{-n}$

  where $C_\mathrm{ref,vol}$ is (implicitly) hardcoded = 1 F/m3. Then the equation has legitimate unit syntax but the implementation in code doesn't need altering.

• Current approach (avoid inheriting an equation using $\gamma$ from the base class) is fine imo.

• In my view we should prefer a solution with future-proofed flexibility for the true equilibrium OCP to be unequal to the mean of the two branches. Not doing this now is just storing another breaking change for the future, isn't it? From my opinion and in terms of my use cases, I'd be ok to suck up a breaking change that impacts the lumped heat source. Of course, it'd be good to check this against a general policy.

[rtimms]

To do:

Fix #5038

and

-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"

[kratman]

I am looking into the docs failure now, probably something pretty minor

[kratman]

I am looking into the docs failure now, probably something pretty minor

I pushed a temporary fix for the failing citation so that review could continue, I will push a in a real fix once I get it working again

[kratman]

Docs are fixed now. I also updated the description to make sure all tickets get closed

[rtimms]

I also prefer the previous names (Axen/Wycisk) over the new names which are a real mouthful, but not a strong opinion

The argument for them is that they at least infer what the model is from the option. I don't think the option names are so much of a mouthful ("one-state hysteresis"), but I agree the classes could be shortened.

[rtimms]

@aabills I removed the factor of 2 so that the model implementation is non-breaking. Passing the old option names now logs a warning and renames them to the new names. "Axen"-->"one-state hysteresis" and "Wycisk"->"one-state differential capacity hysteresis". The models are the same equations, just expressed in a more unified way.