Bug fix: added SEI on cracks to total_kinetics.py

[DrSOKane]

Description

Previously, the loop in total_kinetics.py did not include the SEI on cracks reaction, so the SEI on cracks did not actually remove any lithium from the system. This has now been fixed.

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)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ flake8
• All tests pass: $ python run-tests.py --unit
• The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

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

[valentinsulzer]

The space in reaction_name moved, because reasons

It's to keep you on your toes

[codecov]

Codecov Report

Merging #2262 (34f6a00) into develop (2c60416) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #2262   +/-   ##
========================================
  Coverage    99.40%   99.40%           
========================================
  Files          364      364           
  Lines        20021    20034   +13     
========================================
+ Hits         19901    19914   +13     
  Misses         120      120           

Impacted Files	Coverage Δ
pybamm/expression_tree/functions.py	100.00% <100.00%> (ø)
...m/models/full_battery_models/base_battery_model.py	99.78% <100.00%> (+<0.01%)	⬆️
...ybamm/models/submodels/interface/base_interface.py	100.00% <100.00%> (ø)
pybamm/models/submodels/interface/sei/base_sei.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[valentinsulzer]

Wouldn't it be easier to put the roughness variable into the SEI current? I'm just thinking about conservation of lithium with this approach (we need to check conservation of lithium under side reactions more carefully)

[DrSOKane]

Wouldn't it be easier to put the roughness variable into the SEI current? I'm just thinking about conservation of lithium with this approach (we need to check conservation of lithium under side reactions more carefully)

You would think so, but the fundamental variable for SEI - including on cracks - is thickness, not concentration, so multiplying by roughness would give the wrong SEI thickness, which matters a lot in the diffusion-limited cases.

There are three places where I've deemed it necessary to multiply by roughness:

1. When calculating the SEI concentration, which also determines the "Loss of lithium to SEI" variables.
2. When calculating the volumetric current density, because that determines all the source terms.
3. When incrementing the "sum of interfacial current densities" variables, to ensure current conservation.

The only way to make it less confusing that I can think of is to change the fundamental variable to concentration instead of thickness.

[valentinsulzer]

So is concentration not always directly proportional to thickness? If not, what is the equation that relates thickness and concentration in the general case?

[DrSOKane]

So is concentration not always directly proportional to thickness? If not, what is the equation that relates thickness and concentration in the general case?

n_inner_cr = L_inner_cr * (roughness - 1) # inner SEI cracks concentration
n_outer_cr = L_outer_cr * (roughness - 1) # outer SEI cracks concentration

[valentinsulzer]

In that case it would be cleaner to have the fundamental variable be concentration (and for lithium plating too). Could you try this? It shouldn't be too difficult.

[DrSOKane]

In that case it would be cleaner to have the fundamental variable be concentration (and for lithium plating too). Could you try this? It shouldn't be too difficult.

I started to code this before realizing that the conversion between the two depends on the surface area to volume ratio. This matters because the initial condition is (a) given in metres, not mol.m-3 and (b) not zero. Whereas for plating there is no distiction between concentration and thickness, because the initial condition tends to be zero.

I could change it so the initial condition is defined as a concentration, but that would be a breaking change that would render all previous results obtained using PyBaMM irreproducible unless the initial conditions were adjusted by performing the conversion manually, and not all users will know how to do this.

[valentinsulzer]

Could you calculate the initial concentration based on the initial thickness? We know the initial surface area to volume ratio, even as a function of x.

[DrSOKane]

Could you calculate the initial concentration based on the initial thickness? We know the initial surface area to volume ratio, even as a function of x.

I could do that next week.

To be honest, the initial SEI concentration should be part of the cell balancing process. That's beyond the scope of this PR, but making concentration the fundamental variable is a stepping stone that will make it possible in the future.

[DrSOKane]

Hi all, I've swapped the SEI concentration and thickness so that concentration is now the fundamental variable, but the tests reveal two bugs:

1. The half-cell tests are failing because it says "Lithium metal total interfacial curent density" is not defined, although I have no idea why.

2. Some other tests that use inverse_butler_volmer claims "X-averaged total SEI thickness" is not defined. This could be because as part of the swap, the thickness is now a coupled variable as opposed to a fundamental one.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[valentinsulzer]

Thanks, will merge once tests pass