Update SPMe to generalised version

[brosaplanella]

Description

This issue generalises the SPMe formulation. The main contributions are

• [x] Upgrade SPMe to the general formulation in #4274.
• [x] Remove Integrated model as it is no longer needed.
• [x] Document SPMe definition
• [ ] Implement BasicSPMe

Fixes #4274

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• [ ] New feature (non-breaking change which adds functionality)
• [x] Optimization (back-end change that speeds up the code)
• [ ] Bug fix (non-breaking change which fixes an issue)

Key checklist:

• [ ] No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• [ ] All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• [ ] The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• [ ] Code is commented, particularly in hard-to-understand areas
• [ ] Tests added that prove fix is effective or that feature works

[brosaplanella]

I ran some comparison between all the SPMe models.

Accuracy

I compared the outputs of the models and there are no appreciable differences across SPMe implementations. That might sound a bit disappointing as we are comparing a good approximation with a slightly better approximation. In O'Regan 2022 maybe one can argue that the new model does slightly better, as there the thermodynamic factor and transfer number are also dependent on concentration, which is what the new model captures. I did similar comparison for the half-cell and it also agrees very well.

Marquis 2019

O'Regan 2022

Solving times

Here are the solving times. It is just one occurrence, but I ran the example multiple times and they are representative. Basically, the new SPMe is not significantly slower than the all one so all good. These times are for the Marquis2019 parameter set and the full cell.

Model	Build time [ms]	Solve time [ms]
new SPMe	44.578	11.814
old SPMe	45.518	12.957
integrated SPMe	50.563	11.331
DFN	54.589	113.854

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.69%. Comparing base (86b00ce) to head (1c9993a).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4329      +/-   ##
===========================================
- Coverage    98.70%   98.69%   -0.01%     
===========================================
  Files          304      303       -1     
  Lines        23432    23367      -65     
===========================================
- Hits         23129    23063      -66     
- Misses         303      304       +1     

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

[rtimms]

@brosaplanella do you think it's possible to get this done in time for the next release?

[brosaplanella]

@brosaplanella do you think it's possible to get this done in time for the next release?

When are we planning to make the release? I haven't had time to work on this, I will probably have some time in the next couple of weeks.

[brosaplanella]

Hi @aabills! I have cleaned the code down to what I had before I started trying to debug. The issue is with the eta_c terms as they involve a gradient inside of the integral (https://github.com/pybamm-team/PyBaMM/issues/4274). In order to make the gradient work, I have to add boundary conditions for c_e in every subdomain. However, this means when I use the surface form models the boundary conditions are defined twice, which causes another error.

I am pretty convinced that the problem are the eta_c terms as, if we multiply them all by 0 and comment out the set_boundary_conditions method the tests pass. And it seems the issue is that the model imposes boundary conditions on the concatenated variable c_e so PyBaMM is unable to figure the boundary conditions in each subdomain.

I have been going in circles for a long time with this, so any insight helps.