Spherical finite volume

[valentinsulzer]

Description

Fixes a bug in finite_volumes.py for spherical diffusion.
(r_edge[i+1]**3 - r_edge[i]**3)/3 is now used instead of r_node**2. This is what it should be according to Finite Volume theory e.g. https://journals.aps.org/prapplied/abstract/10.1103/PhysRevApplied.6.064010

Previous convergence:

New convergence:

Code:

import pybamm
import numpy as np

model = pybamm.lithium_ion.DFN()

chemistry = pybamm.parameter_sets.Ecker2015
parameter_values = pybamm.ParameterValues(chemistry=chemistry)

var = pybamm.standard_spatial_vars
npts = [10, 20, 25, 30, 40, 50]

solver = pybamm.CasadiSolver(mode="safe")
t_eval = np.linspace(0, 3800, 200)

sols = []
for N in npts:
    var_pts = {var.x_n: 20, var.x_s: 20, var.x_p: 20, var.r_n: N, var.r_p: N}
    sim = pybamm.Simulation(
        model,
        solver=solver,
        parameter_values=parameter_values,
        var_pts=var_pts,
        # C_rate=5,
    )
    sol = sim.solve(t_eval=t_eval)
    print("N={}: {} s".format(N, sol.solve_time))
    sols.append(sol)

pybamm.dynamic_plot(sols, ["Terminal voltage [V]"], time_unit="seconds", labels=npts)

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]

Since the convergence is better, we could reduce the default number of r points to 20 (10 still gives inaccurate results at higher C-rates). See #1075

[codecov]

Codecov Report

Merging #1782 (c67c3db) into develop (93ead88) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1782      +/-   ##
===========================================
+ Coverage    99.28%   99.29%   +0.01%     
===========================================
  Files          343      343              
  Lines        18910    19218     +308     
===========================================
+ Hits         18775    19083     +308     
  Misses         135      135              

Impacted Files	Coverage Δ
...m/models/full_battery_models/base_battery_model.py	99.71% <ø> (ø)
pybamm/util.py	100.00% <ø> (ø)
pybamm/spatial_methods/finite_volume.py	98.14% <100.00%> (-0.01%)	⬇️
pybamm/models/submodels/electrode/ohm/li_metal.py	100.00% <0.00%> (ø)
...bamm/models/full_battery_models/lithium_ion/dfn.py	100.00% <0.00%> (ø)
...bamm/models/full_battery_models/lithium_ion/spm.py	100.00% <0.00%> (ø)
...bamm/models/submodels/electrode/ohm/leading_ohm.py	100.00% <0.00%> (ø)
...amm/models/full_battery_models/lithium_ion/spme.py	100.00% <0.00%> (ø)
...mm/models/submodels/electrode/ohm/composite_ohm.py	100.00% <0.00%> (ø)
...ttery_models/lithium_ion/base_lithium_ion_model.py	100.00% <0.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93ead88...c67c3db. Read the comment docs.

[rtimms]

Thanks, @tinosulzer , nice catch! This has been lurking for a while...

Yes, let's reduce the default points, and maybe we can also check to see if we can now use fewer points in this notebook? I'm guessing we might be able to since the method is correct now.

[brosaplanella]

Looks good, thanks Tino!

[valentinsulzer]

How did you come up with 150 points for the ORegan parameter set
before:

now:

?

[brosaplanella]

There were some issues with the convergence with the low diffusion areas, but maybe with the final parameter set is not needed any more. If it converges, happy to reduce the number of points 😄