Implement Integrated electrolyte conductivity #1188

brosaplanella · 2020-10-13T10:24:51Z

Description

Implementation of the Integrated electrolyte conductivity model as defined in Brosa Planella et al. (2020).

Paper is not yet on arXiv, we can wait to merge until it is to avoid another PR in a couple of weeks.

Fixes #884

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)

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

…://github.com/ferranbrosa/PyBaMM into issue-884-electrolyte-integrated-conductivity

valentinsulzer

Looks good @brosaplanella , you should also add a test in test_spme.py (both unit and integration)

valentinsulzer · 2020-10-19T13:11:42Z

...ut/parameters/lithium-ion/electrolytes/lipf6_Nyman2008/electrolyte_conductivity_Nyman2008.py

@@ -27,7 +27,7 @@ def electrolyte_conductivity_Nyman2008(c_e, T):
        0.1297 * (c_e / 1000) ** 3 - 2.51 * (c_e / 1000) ** 1.5 + 3.329 * (c_e / 1000)
    )

-    E_k_e = 34700


was this a mistake in the original implementation?
you could argue we should just not have the arrhenius term in this case

Not sure if a mistake, or we decided to keep the Arrhenius term from the Marquis2019 set (originally it all was in parameters.csv. However, if now it is specified as a function I think it would be good to stick to what the paper provides, and it this case, Nyman et al. 2008, does not provide temperature dependence so I removed it.

If that makes sense, I will remove the whole Arrhenius term so it is clearer.

yeah, that makes sense, just remove the arrhenius term and put in a comment

valentinsulzer · 2020-10-19T13:11:48Z

...put/parameters/lithium-ion/electrolytes/lipf6_Nyman2008/electrolyte_diffusivity_Nyman2008.py

@@ -24,7 +24,7 @@ def electrolyte_diffusivity_Nyman2008(c_e, T):
    """

    D_c_e = 8.794e-11 * (c_e / 1000) ** 2 - 3.972e-10 * (c_e / 1000) + 4.862e-10
-    E_D_e = 37040


valentinsulzer · 2020-10-19T13:12:38Z

pybamm/models/submodels/electrolyte_conductivity/integrated_conductivity.py

+        The domain in which the model holds
+
+    **Extends:** :class:`pybamm.electrolyte_conductivity.BaseElectrolyteConductivity`
+    """


add reference to your paper in the docstring

valentinsulzer · 2020-10-19T13:14:14Z

pybamm/models/submodels/electrolyte_conductivity/integrated_conductivity.py

+            / param.gamma_e
+        )
+
+        integral_n = indef_integral_n - pybamm.boundary_value(indef_integral_n, "left")


left boundary value of an indefinite integral should always be zero by definition (and implementation) but worth checking

I checked, and maybe it evaluates to zero (actually it should), but is not zero by definition. The definition is

<bound method UnaryOperator.evaluate of BoundaryValue(-0x33bf8d7889aadb3a, boundary value, children=["* integrated w.r.t x_n on ['negative electrode'](3.0 * x_n ** 2.0)"], domain=['current collector'], auxiliary_domains={})>

valentinsulzer · 2020-10-19T13:15:38Z

pybamm/models/submodels/electrolyte_conductivity/integrated_conductivity.py

+        integral_s = (
+            indef_integral_s
+            - pybamm.boundary_value(indef_integral_s, "left")
+            + pybamm.boundary_value(integral_n, "right")


pybamm.Integral(a) should give the same value as pybamm.boundary_value(pybamm.IndefiniteIntegral(a), "right") while building a smaller tree

but maybe it automatically gets simplified down to the same tree anyway

How can I check if they reduce to the same tree? In any case, I think that if they are equivalent, using pybamm.Integral(a) and removing the boundary values on the left should result in a much more readable code.

Just tried replacing the boundary value at the "right" by Integral and I get a very long error (see below) which basically looks like a dimension mismatch. So I guess they are not the same object...

Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 817, in process_symbol return self._discretised_symbols[symbol.id] KeyError: 1041553401910644705 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 817, in process_symbol return self._discretised_symbols[symbol.id] KeyError: 2459353937759019131 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 817, in process_symbol return self._discretised_symbols[symbol.id] KeyError: 4130260032263747209 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 817, in process_symbol return self._discretised_symbols[symbol.id] KeyError: 7060176036539492958 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 817, in process_symbol return self._discretised_symbols[symbol.id] KeyError: 8911840126246680561 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 817, in process_symbol return self._discretised_symbols[symbol.id] KeyError: -7919469058354380383 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/symbol.py", line 594, in evaluate_for_shape return self._saved_evaluate_for_shape AttributeError: 'MatrixMultiplication' object has no attribute '_saved_evaluate_for_shape' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/symbol.py", line 801, in test_shape self.shape_for_testing File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/symbol.py", line 785, in shape_for_testing evaluated_self = self.evaluate_for_shape() File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/symbol.py", line 596, in evaluate_for_shape self._saved_evaluate_for_shape = self._evaluate_for_shape() File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/binary_operators.py", line 201, in _evaluate_for_shape return self._binary_evaluate(left, right) File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/binary_operators.py", line 497, in _binary_evaluate return left @ right File "/home/ferranbrosa/coding/reduced-TEC/env/lib/python3.7/site-packages/scipy/sparse/base.py", line 564, in __matmul__ return self.__mul__(other) File "/home/ferranbrosa/coding/reduced-TEC/env/lib/python3.7/site-packages/scipy/sparse/base.py", line 502, in __mul__ raise ValueError('dimension mismatch') ValueError: dimension mismatch During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ferranbrosa/miniconda3/lib/python3.7/runpy.py", line 193, in _run_module_as_main "__main__", mod_spec) File "/home/ferranbrosa/miniconda3/lib/python3.7/runpy.py", line 85, in _run_code exec(code, run_globals) File "/home/ferranbrosa/.vscode-server/extensions/ms-python.python-2020.9.114305/pythonFiles/lib/python/debugpy/__main__.py", line 45, in <module> cli.main() File "/home/ferranbrosa/.vscode-server/extensions/ms-python.python-2020.9.114305/pythonFiles/lib/python/debugpy/../debugpy/server/cli.py", line 430, in main run() File "/home/ferranbrosa/.vscode-server/extensions/ms-python.python-2020.9.114305/pythonFiles/lib/python/debugpy/../debugpy/server/cli.py", line 267, in run_file runpy.run_path(options.target, run_name=compat.force_str("__main__")) File "/home/ferranbrosa/miniconda3/lib/python3.7/runpy.py", line 263, in run_path pkg_name=pkg_name, script_name=fname) File "/home/ferranbrosa/miniconda3/lib/python3.7/runpy.py", line 96, in _run_module_code mod_name, mod_spec, pkg_name, script_name) File "/home/ferranbrosa/miniconda3/lib/python3.7/runpy.py", line 85, in _run_code exec(code, run_globals) File "/home/ferranbrosa/coding/reduced-TEC/TSPMe_compare_DFN.py", line 91, in <module> sim.solve([0, 3700 / Crate]) File "/home/ferranbrosa/PyBaMM/pybamm/simulation.py", line 338, in solve self.build(check_model=check_model) File "/home/ferranbrosa/PyBaMM/pybamm/simulation.py", line 291, in build self._model_with_set_params, inplace=False, check_model=check_model File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 173, in process_model self.set_internal_boundary_conditions(model) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 419, in set_internal_boundary_conditions rbc = (boundary_gradient(first_orphan, next_orphan), "Neumann") File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 399, in boundary_gradient right_symbol_disc = self.process_symbol(right_symbol) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 819, in process_symbol discretised_symbol = self._process_symbol(symbol) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 853, in _process_symbol disc_right = self.process_symbol(right) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 819, in process_symbol discretised_symbol = self._process_symbol(symbol) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 853, in _process_symbol disc_right = self.process_symbol(right) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 819, in process_symbol discretised_symbol = self._process_symbol(symbol) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 862, in _process_symbol disc_child = self.process_symbol(child) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 819, in process_symbol discretised_symbol = self._process_symbol(symbol) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 852, in _process_symbol disc_left = self.process_symbol(left) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 819, in process_symbol discretised_symbol = self._process_symbol(symbol) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 852, in _process_symbol disc_left = self.process_symbol(left) File "/home/ferranbrosa/PyBaMM/pybamm/discretisations/discretisation.py", line 821, in process_symbol discretised_symbol.test_shape() File "/home/ferranbrosa/PyBaMM/pybamm/expression_tree/symbol.py", line 803, in test_shape raise pybamm.ShapeError("Cannot find shape (original error: {})".format(e))

brosaplanella · 2020-10-20T09:04:54Z

I have addressed all the comments except the one about the Integral which seems to be a deeper issue with the integral. Once I submit the draft to arXiv (hopefully in the next two weeks) I will have to update the citation. Is it better to merge now and update later or to wait to merge?

valentinsulzer

Thanks @brosaplanella , looks good now, don't worry about the integral thing

valentinsulzer · 2020-10-20T14:50:45Z

tests/integration/test_models/test_full_battery_models/test_lithium_ion/test_spme.py

+        model.submodels[
+            "electrolyte conductivity"
+        ] = pybamm.electrolyte_conductivity.Integrated(model.param)
+        model.build_model()


You might want to provide an option for doing this automatically

I thought about that, but wasn't sure what approach we want to take. We discussed it some time ago, but can't remember what were the arguments against being able to pass options to set all the submodels.

I don't remember the arguments against either. I think you should add the option

…lyte conductivity and made it compatible with surface form

…vity

valentinsulzer · 2020-10-28T17:26:09Z

Ready to merge?

…vity

brosaplanella added 30 commits April 22, 2020 10:29

pybamm-team#884 added integrated conductivity model

92de548

pybamm-team#884 debugging new submodel

e5d7c14

pybamm-team#884 added integrated conductivity model

fb667b3

pybamm-team#884 debugging new submodel

7899fdd

Merge branch 'issue-884-electrolyte-integrated-conductivity' of https…

7b3df30

…://github.com/ferranbrosa/PyBaMM into issue-884-electrolyte-integrated-conductivity

pybamm-team#884 added integrated conductivity model

573db34

pybamm-team#884 debugging new submodel

eec8354

Merge branch 'issue-884-electrolyte-integrated-conductivity' of https…

5d7ca5d

…://github.com/ferranbrosa/PyBaMM into issue-884-electrolyte-integrated-conductivity

flake8

334dd17

pybamm-team#884 added integrated conductivity model

3eb9c68

pybamm-team#884 debugging new submodel

4a3ebf7

flake8

5c5b7fa

added tab parameters (set to zero) so thermal model runs

00bfd7c

debugged conductivity

a8496db

added new SPMe model

34b94f3

fix merge conflict

e439182

flake8

3ebae0b

pybamm-team#884 added integrated conductivity model

5548930

pybamm-team#884 debugging new submodel

4c10748

flake8

7e3c823

debugged conductivity

3f76b27

added new SPMe model

577f5fc

flake8

a39ac10

solved merge conflict

9d30d37

pybamm-team#884 added integrated conductivity model

e7897be

pybamm-team#884 debugging new submodel

428e4aa

flake8

e30ddf9

debugged conductivity

f5bef85

added new SPMe model

7ebf86a

flake8

f0aa087

brosaplanella requested review from rtimms and valentinsulzer October 19, 2020 12:51

pybamm-team#884 update CHANGELOG

e615e4a

valentinsulzer requested changes Oct 19, 2020

View reviewed changes

brosaplanella added 6 commits October 19, 2020 15:48

pybamm-team#884 add docs

08d191c

pybamm-team#884 removed arrhenius term

6a89b7d

pybamm-team#884 included docstring reference

3f04c15

pybamm-team#884 removed redundant right boundary integrals

86051b9

pybamm-team#884 added SPMe tests

a019e6b

flake8

8341306

brosaplanella added 2 commits October 20, 2020 11:56

pybamm-team#884 fixed typo in docs

4890bab

pybamm-team#884 fix documentation

f35dd26

valentinsulzer approved these changes Oct 20, 2020

View reviewed changes

brosaplanella added 11 commits October 22, 2020 10:13

pybamm-team#884 added options to set integrated conductivity

30ab27c

pybamm-team#884 pybamm-team#1207 implemented option to choose electro…

6aa1bb3

…lyte conductivity and made it compatible with surface form

pybamm-team#1207 update CHANGELOG

0308466

Merge branch 'develop' into issue-884-electrolyte-integrated-conducti…

ddb5ac5

…vity

pybamm-team#884 set SPMe surface form to NotImplemented for the moment

006dc25

pybamm-team#884 update CHANGELOG

480765f

flake8

9818f21

pybamm-team#884 removed SPMe from test_compare_outputs for surface form

3afea4e

Merge branch 'develop' into issue-884-electrolyte-integrated-conducti…

8672eff

…vity

Merge branch 'develop' into issue-884-electrolyte-integrated-conducti…

e286ce0

…vity

pybamm-team#884 fix typo CITATIONS.txt

c301814

brosaplanella added 3 commits November 3, 2020 12:01

pybamm-team#884 add test citation Brosa Planella 2020

f67357b

pybamm-team#884 update citation TSPMe

5d4bd51

Merge branch 'develop' into issue-884-electrolyte-integrated-conducti…

49fa413

…vity

brosaplanella merged commit d77ed8a into pybamm-team:develop Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Integrated electrolyte conductivity #1188

Implement Integrated electrolyte conductivity #1188

brosaplanella commented Oct 13, 2020 •

edited

Loading

valentinsulzer left a comment

valentinsulzer Oct 19, 2020

brosaplanella Oct 19, 2020

valentinsulzer Oct 19, 2020

valentinsulzer Oct 19, 2020

valentinsulzer Oct 19, 2020

valentinsulzer Oct 19, 2020

brosaplanella Oct 20, 2020

valentinsulzer Oct 19, 2020

valentinsulzer Oct 19, 2020

brosaplanella Oct 20, 2020

brosaplanella Oct 20, 2020

brosaplanella commented Oct 20, 2020

valentinsulzer left a comment

valentinsulzer Oct 20, 2020

brosaplanella Oct 20, 2020

valentinsulzer Oct 20, 2020

valentinsulzer commented Oct 28, 2020

Implement Integrated electrolyte conductivity #1188

Implement Integrated electrolyte conductivity #1188

Conversation

brosaplanella commented Oct 13, 2020 • edited Loading

Description

Type of change

Key checklist:

Further checks:

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

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

brosaplanella commented Oct 20, 2020

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

valentinsulzer commented Oct 28, 2020

brosaplanella commented Oct 13, 2020 •

edited

Loading