Polynomial concentration #1130

rtimms · 2020-08-21T15:46:18Z

Description

Adds particle submodels that approximate the concentration profile within the particle as a polynomial in r. Note the "fast diffusion" option is now accessed as the "uniform profile" option (fast diffusion leads to uniform concentration in r, but it makes sense to have uniform concentration as the zero-order case of the polynomial model).

Fixes #1128

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

codecov · 2020-08-24T15:01:18Z

Codecov Report

Merging #1130 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1130      +/-   ##
===========================================
+ Coverage    97.90%   97.92%   +0.02%     
===========================================
  Files          247      247              
  Lines        13750    13910     +160     
===========================================
+ Hits         13462    13622     +160     
  Misses         288      288

Impacted Files	Coverage Δ
pybamm/solvers/casadi_solver.py	`100.00% <ø> (ø)`
...m/models/full_battery_models/base_battery_model.py	`99.66% <100.00%> (+<0.01%)`	⬆️
...bamm/models/full_battery_models/lithium_ion/dfn.py	`100.00% <100.00%> (ø)`
...bamm/models/full_battery_models/lithium_ion/spm.py	`100.00% <100.00%> (ø)`
...amm/models/full_battery_models/lithium_ion/spme.py	`100.00% <100.00%> (ø)`
pybamm/models/standard_variables.py	`100.00% <100.00%> (ø)`
pybamm/models/submodels/particle/__init__.py	`100.00% <100.00%> (ø)`
pybamm/models/submodels/particle/base_particle.py	`100.00% <100.00%> (ø)`
...odels/submodels/particle/fickian_many_particles.py	`100.00% <100.00%> (ø)`
...dels/submodels/particle/fickian_single_particle.py	`100.00% <100.00%> (ø)`
... and 4 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 71630c5...fbb9af4. Read the comment docs.

rtimms · 2020-08-24T15:37:24Z

pybamm/models/submodels/particle/polynomial_many_particles.py

+            self.initial_conditions.update({c_s_surf: c_init})
+        if self.order == 4:
+            # We also need to provide an initial condition (initial guess for the
+            # algebraic solver) for the average concentration gradient


I will remove the comment about algebraic solver here, as it is an ODE for q (accidentally left the comment here from copy/paste above)

rtimms · 2020-08-24T15:40:09Z

pybamm/models/submodels/particle/polynomial_single_particle.py

+                (5 / 2) * (c_s_surf_xav - c_s_rxav), [self.domain.lower() + " particle"]
+            )
+            if self.domain == "Negative":
+                # TODO: figure out how to just use r from standard_spatial_vars


I kept getting shape errors here and couldn't figure out the correct way to broadcast things so I could use r from standard_spatial_vars here. I thought I should broadcast A, B, (C), then use that to define c_s, and then average to get c_s_av, but I kept getting bogged down in shape errors

Since c_s_xav doesn't depend on x, it makes sense that you should use a different r which only has "negative particle" and "current collector" as domains

rtimms · 2020-08-24T15:40:42Z

pybamm/models/submodels/particle/polynomial_single_particle.py

+
+        self.initial_conditions = {c_s_rxav: c_init}
+        if self.order == 4:
+            # We also need to provide an initial condition (initial guess for the


see above comment about removing the comment about algebraic solvers here

valentinsulzer

Thanks @rtimms , looks good to me!

valentinsulzer · 2020-08-24T15:56:24Z

pybamm/models/full_battery_models/lithium_ion/dfn.py

+            ] = pybamm.particle.PolynomialManyParticles(self.param, "Negative", order)
+            self.submodels[
+                "positive particle"
+            ] = pybamm.particle.PolynomialManyParticles(self.param, "Positive", order)


I think here it makes more sense to pass the name of the option rather than the order. Otherwise one might naively wonder why there are no orders 1 and 3

valentinsulzer · 2020-08-24T15:57:27Z

pybamm/models/submodels/particle/base_particle.py

@@ -49,6 +66,10 @@ def _get_standard_concentration_variables(self, c_s, c_s_xav):
            "R-averaged "
            + self.domain.lower()
            + " particle concentration [mol.m-3]": c_s_rav * c_scale,
+            "R-X-averaged " + self.domain.lower() + " particle concentration": c_s_av,
+            "R-X-averaged "


I think we have been using "Average" to refer to "R-X-averaged". Do you think this is clearer?

I'm happy to change to "Average". I think it is obvious and less cumbersome.

valentinsulzer · 2020-08-24T16:00:56Z

pybamm/models/submodels/particle/polynomial_single_particle.py

+                (5 / 2) * (c_s_surf_xav - c_s_rxav), [self.domain.lower() + " particle"]
+            )
+            if self.domain == "Negative":
+                # TODO: figure out how to just use r from standard_spatial_vars


Since c_s_xav doesn't depend on x, it makes sense that you should use a different r which only has "negative particle" and "current collector" as domains

TomTranter

Hey Rob, thanks for this, seems like quite a lot of work in the end. Looks great and works well. I added another notebook for the models which included comparing at different C-rates. Feel free to change bits or move it to another folder. I did get some ugly solver errors in the notebook at 5 A which I guess we could hide but are they anything to worry about? Still gets a solution...

rtimms · 2020-08-26T10:28:01Z

thanks @TomTranter notebook looks great! made some minor adjustments. the solver errors were just because of the high current - the solver was taking steps/doing jacobian calculations that were later rejected and the step size decreased. in practice they are nothing to worry about, but I've changed the currents in the notebooks to avoid the warnings and still show off the same results. probably best to avoid potentially confusing solver errors in the examples

rtimms added 7 commits August 7, 2020 20:47

start adding polynomial particle models

426c025

#1128 two-parameter model

e64ed47

#1128 order 0 and 2 working

0ee9ac4

#1128 quadratic and quartic concentration models

c13bed4

Merge branch 'develop' into polynomial-concentration

c3cd2f9

#1128 changelog

9949966

#1128 fix examples

dc31462

rtimms commented Aug 24, 2020

View reviewed changes

rtimms requested review from valentinsulzer and TomTranter August 24, 2020 15:41

valentinsulzer approved these changes Aug 24, 2020

View reviewed changes

Add notebook comparing models for particle diffusion

94db36a

TomTranter approved these changes Aug 25, 2020

View reviewed changes

#1128 tino comments

72e07c6

#1128 fix examples

fbb9af4

rtimms merged commit cb3ad43 into develop Aug 26, 2020

rtimms deleted the polynomial-concentration branch August 26, 2020 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polynomial concentration #1130

Polynomial concentration #1130

rtimms commented Aug 21, 2020 •

edited

Loading

codecov bot commented Aug 24, 2020 •

edited

Loading

rtimms Aug 24, 2020

rtimms Aug 24, 2020

valentinsulzer Aug 24, 2020

rtimms Aug 24, 2020

valentinsulzer left a comment

valentinsulzer Aug 24, 2020

rtimms Aug 24, 2020

valentinsulzer Aug 24, 2020

rtimms Aug 24, 2020

valentinsulzer Aug 24, 2020

TomTranter left a comment

rtimms commented Aug 26, 2020

Polynomial concentration #1130

Polynomial concentration #1130

Conversation

rtimms commented Aug 21, 2020 • edited Loading

Description

Type of change

Key checklist:

Further checks:

codecov bot commented Aug 24, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinsulzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomTranter left a comment

Choose a reason for hiding this comment

rtimms commented Aug 26, 2020

rtimms commented Aug 21, 2020 •

edited

Loading

codecov bot commented Aug 24, 2020 •

edited

Loading