Pe degrade #3771

mzzhuo · 2024-01-27T16:55:13Z

Description

(Draft for reviewing) Add the core-shell submodel for phase transition caused PE (NMC811) degradation.

Fixes # (issue)

Type of change

New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

All tests pass: $ python run-tests.py --all (or $ nox -s tests)

Further checks:

Code is commented, particularly in hard-to-understand areas

…ving phase boundary conditions

…cross electrode thickness

…gotten

… paper results

…ose magnitude is unknown is often dangerous, may leading to associativity issues of floating point computation

kratman · 2024-03-19T14:44:22Z

@mzzhuo Are you still working on this?

mzzhuo · 2024-03-19T16:26:31Z

@mzzhuo Are you still working on this?

From my side, the submodel is done, not working on it now but waiting for the feedback from pybamm team. I think Feran and Rob are reviewing the code

kratman · 2024-03-19T16:39:51Z

@mzzhuo I was primarily asking since the CI has been failing for this PR. Can you update the branch to resolve some of the issues? That needs to be done before it is merged anyway

rtimms

Thanks for working on this. @mzzhuo can you point to implementation details? The way this has been set up is different from what I was anticipating, so I'm getting a little lost. It seems like the model and spatial implementation are a little tangled here, and I think it would be great to have the general moving boundary stuff be handled by spatial methods and just have the PDEs in the model.

rtimms · 2024-04-17T08:42:02Z

pybamm/expression_tree/unary_operators.py

@@ -1014,6 +1014,46 @@ def _sympy_operator(self, child):
            return sympy.Symbol(latex_child)


+class BoundaryCellValue(BoundaryOperator):


It seems odd to me that this is part of unary_operators rather than being part of a spatial method. can we talk more about how the problem is posed for the moving boundary?

Ok, I see why this might be here. It's essentially BoundaryValue but taking cell centre rather than edge. I still kind of feel like this is a concern of spatial methods. Let's talk more.

rtimms · 2024-04-17T08:43:28Z

pybamm/geometry/battery_geometry.py

+        if phases == 1:
+            geometry.update(
+                {
+                    "positive core": {"r_co": {"min": 0, "max": geo_domain.prim.R_typ}},


I was expecting to have the core/shell be solved on [0, 1] and then have transformed PDEs that solve for the lengths to handle the moving boundary. Can you point to the paper or some notes for the implementation?

rtimms · 2024-04-17T08:45:52Z

pybamm/geometry/standard_spatial_vars.py

+        "secondary": "positive electrode",
+        "tertiary": "current collector",
+    },
+    # note the coordinate system is different from that of r_co


if you're doing a transformation anyway, why not pose both eqns on a catersian domain [0, 1], then do the mapping later? assume this is to make plotting easier later so everything lives on [0, R]?

rtimms · 2024-04-17T08:47:33Z

pybamm/models/base_model.py

@@ -102,7 +102,7 @@ def __init__(self, name="Unnamed model"):
        self._rhs = {}
        self._algebraic = {}
        self._initial_conditions = {}
-        self._boundary_conditions = {}
+        self._boundary_conditions = BoundaryConditionsDict({})


don't think this is needed

rtimms · 2024-04-17T08:48:01Z

pybamm/models/full_battery_models/base_battery_model.py

@@ -146,6 +146,12 @@ class BatteryModelOptions(pybamm.FuzzyDict):
                Sets the model to include a single active particle size or a
                distribution of sizes at any macroscale location. Can be "single"
                (default) or "distribution". Option applies to both electrodes.
+            * "PE degradation": str
+                Set the PE degradation submodel (core-shell) to be used. Options are:


can you write "Positive Electrode" in these docs so people know what "PE" means

We need to agree on a name for this option, for compatibility for a planned acid dissolution model

rtimms · 2024-04-17T08:50:45Z

pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py

+        domain = "positive"
+
+        # replace the fickian particle submodel
+        # make sure set_pe_degradation_submodel comes after


there is already code that loops over the submodels in a way that the order doesn't matter. does it fail in this case? if so, it would be good to see if we can fix that part of the code rather than having a special case.

rtimms · 2024-04-17T08:51:07Z

pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py

+                        "Make sure it is invoked before "
+                        "calling phase transition submodel."
+                    )
+                elif not isinstance(


can this be caught earlier as as options check?

rtimms · 2024-04-17T08:52:08Z

pybamm/models/submodels/external_circuit/base_external_circuit.py

@@ -20,6 +20,8 @@ def get_fundamental_variables(self):
        variables = {
            "Discharge capacity [A.h]": Q_Ah,
            "Throughput capacity [A.h]": Qt_Ah,
+            # initial SoC is 0 as initial Q_Ah is set null
+            "Cell SoC": -Q_Ah / self.param.Q,


I don't think we should define cell SoC as everyone has their own definition (see #2553)

rtimms · 2024-04-17T08:53:08Z

pybamm/models/submodels/pe_degradation/base_phase_transition.py

+        # Read from options to see if we have a particle size distribution
+        domain_options = getattr(self.options, domain)
+        self.size_distribution = domain_options["particle size"] == "distribution"
+        if self.size_distribution is True:


can this be caught earlier as an option check?

rtimms · 2024-04-17T08:54:28Z

pybamm/models/submodels/pe_degradation/base_phase_transition.py

+
+        return variables
+
+    def _r_average_core(self, symbol):


again, I think this kind of thing should be handled by the spatial methods code, rather than explicitly as part of the model

Hi Rob, thanks for reviewing the codes. Regarding your main concern, the PE degradation submodel consists of two diffusion equations residing in a shrinking core and expanding shell, and a set of ODEs describing the moving of the phase boundary. The first step is to do variable change so that the computational domains (core and shell) are fixed, this will naturally change the governing diffusion equations by adding extra terms associated with the moving boundary, which is an unknown variable. The extra terms are caused by both the original temporal differential term and spatial differential terms, so it does not make too much sense to incorporate into the discretization method. Another important reason for me doing the current way is to minimize the modification to current pybamm, so I leave the first step to manual calculation. To be honest, I have no idea whether this can be incorporated into the discretized method, or if yes, how much cost it will take. Regrading the [0, 1] issues, that is how I did in the initial version, now to be consistent with current pybamm release, I scaled both up with a constant R_typ. I share the paper and the formula for implementation: https://doi.org/10.1016/j.jpowsour.2022.232461 and https://hkustconnect-my.sharepoint.com/:b:/g/personal/mzhuo_connect_ust_hk/EYmFXnyOpP9KlUsrHg_0RtIBqQoIkN6qIpVCKu262uSgLA?e=EH0SJX

DrSOKane · 2024-05-13T12:00:17Z

pybamm/expression_tree/broadcasts.py

        ):
            raise pybamm.DomainError(
                """Primary broadcast from current collector domain must be to electrode
-                or separator or particle or particle size domains"""
+                or separator or particle (core or shell) or particle size domains"""


Not all particles have core/shell though

DrSOKane · 2024-05-13T12:06:52Z

pybamm/expression_tree/broadcasts.py

+        elif child.domain[0] in [
+            "negative particle",
+            "positive particle",
+            "negative core",


There is no provision for core/shell in the negative electrode, but this might be worth keeping for future-proofing

DrSOKane · 2024-05-13T12:09:02Z

pybamm/expression_tree/variable.py

@@ -150,7 +150,7 @@ def to_json(

 class Variable(VariableBase):
    """
-    A node in the expression tree represending a dependent variable.
+    A node in the expression tree represending an independent variable.


This is wrong, please undo this

DrSOKane · 2024-05-13T12:52:15Z

pybamm/models/submodels/pe_degradation/base_phase_transition.py

+import pybamm
+
+
+class BasePhaseTransition(pybamm.BaseSubModel):


This name will cause problems if anyone wants to add a different positive electrode degradation model, such as acid dissolution. Please change to something like BasePositiveDegradation or BasePositiveElectrodeDegradation.

Good point. How about create a subfolder called "phase transition" inside the pe_degradation folder, and other subfolder like "acid dissolution" etc. pe_degradation is general and can include other non-core-shell submodels.

DrSOKane

Hi MZ, thank you so much for writing this code, which must have taken a huge amount of time! Robert Timms and I made some comments in specific areas of the code, but I also have some general comments that must be addressed before we can proceed any further.

Firstly, the model is still incompatible with the particle mechanics submodel as things stand. You can either accept this and add code to base_battery_model that prevents users from trying to use both, or alter the code for particle mechanics so that they are compatible.

Secondly, I don't understand why you need a separate set of "cyclable lithium" variables. If the lithium in the shell phase is dead with no posibility of recovery, then all you need to do is arange it so PyBaMM does not count the lithium in the shell when calculating "Total lithium in particle [mol]".

That said, both Ghosh et al. (2021) and Zhuo et al. (2023) are rigorous pieces of research that deserve to be included in PyBaMM. I also like how you've added a "positive electrode degradation" option, so that more models like yours can be added in the future.

mzzhuo · 2024-06-01T16:56:34Z

Hi MZ, thank you so much for writing this code, which must have taken a huge amount of time! Robert Timms and I made some comments in specific areas of the code, but I also have some general comments that must be addressed before we can proceed any further.

Firstly, the model is still incompatible with the particle mechanics submodel as things stand. You can either accept this and add code to base_battery_model that prevents users from trying to use both, or alter the code for particle mechanics so that they are compatible.

Secondly, I don't understand why you need a separate set of "cyclable lithium" variables. If the lithium in the shell phase is dead with no posibility of recovery, then all you need to do is arange it so PyBaMM does not count the lithium in the shell when calculating "Total lithium in particle [mol]".

That said, both Ghosh et al. (2021) and Zhuo et al. (2023) are rigorous pieces of research that deserve to be included in PyBaMM. I also like how you've added a "positive electrode degradation" option, so that more models like yours can be added in the future.

Hi Simone, thank you so much for your reviewing. 1) yes, incompatible at the moment, we can prevent the use of both submodels now for simplicity. 2) what you said about "Total lithium" is what was defined and calculated in the paper with LLI_tot, see Eq. 16 and Fig. 4. I guess you wonder why we bother to define an extra variable called cyclable lithium. OK, when we were discussing the degradation model, Greg had an assumption based on mass conservation at the interface, that is, the total lithium may increase and not always decrease. This scenario is only possible with the definition of cyclable lithium. The results in Fig. 4 with negative LLI_cyc shows that the model (particularly the mass conservation at the interface) and its implementation are validated, this is what we should expect. Besides this, LLI_total is always positive, i.e., we always lose lithium, but this info. is not that relevant to lithium shutting between the two electrodes. The stochiometry change from 0 to 100% corresponds to the cyclable lithium shutting. Note that in Figs. 5, 7 and 9 the results are all about cyclable lithium so that we can see the subtle difference between different scenarios.

Mingzhao Zhuo added 17 commits January 26, 2024 18:52

add the pe_degradation phase transition core-shell submodel files

61e04f8

add parameter set Zhuo2023 for PE degradation submodel

59b6cd8

add new unary operators and their finite volume implementation for mo…

4dd3521

…ving phase boundary conditions

modify main pybamm packages so that pe degradation submodel can be used

47b1313

add pe degradation submodel parameters into lithium_ion_parameters

905ab72

add a bottom concentration limit for cyclable lithium calculation

fff284d

add a variable representing the normalized averaged particle radius a…

7d06fd2

…cross electrode thickness

add shell overpotential effect

9067ad6

add a variable cell SoC for Zhuo2023 paper plot

f9a526e

fix a potential bug that may lead to crash if geometry definition for…

3847239

…gotten

fix typos

80b5829

enable boundary condition specification using index

3b58405

to pass the test

b026653

add an example to run the pe degradation model and reproduce Zhuo2023…

ad33d30

… paper results

add spyder project auxiliary files to the ignore list

32befd4

identify a bug in harmonic mean, adding a small number to a number wh…

d7f462a

…ose magnitude is unknown is often dangerous, may leading to associativity issues of floating point computation

Merge branch 'develop' of github.com:pybamm-team/PyBaMM into pe_degrade

9de4f6c

mzzhuo requested review from martinjrobins, Saransh-cpp, agriyakhetarpal, kratman, arjxn-py, a team, rtimms, valentinsulzer, brosaplanella, DrSOKane and TomTranter as code owners January 27, 2024 16:55

pre-commit-ci bot and others added 2 commits January 27, 2024 16:55

style: pre-commit fixes

058b028

Merge branch 'develop' into pe_degrade

5e844b5

pre-commit-ci bot and others added 3 commits February 15, 2024 14:07

style: pre-commit fixes

f59e301

Merge branch 'develop' into pe_degrade

8f429b3

style: pre-commit fixes

a510e71

Merge branch 'develop' into pe_degrade

993cffb

rtimms reviewed Apr 17, 2024

View reviewed changes

DrSOKane reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pe degrade #3771

Pe degrade #3771

mzzhuo commented Jan 27, 2024

kratman commented Mar 19, 2024

mzzhuo commented Mar 19, 2024

kratman commented Mar 19, 2024

rtimms left a comment

rtimms Apr 17, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

DrSOKane May 13, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

rtimms Apr 17, 2024

DrSOKane May 13, 2024

rtimms Apr 17, 2024

mzzhuo Apr 17, 2024

DrSOKane May 13, 2024

DrSOKane May 13, 2024

DrSOKane May 13, 2024

DrSOKane May 13, 2024

mzzhuo Jun 1, 2024

DrSOKane left a comment

mzzhuo commented Jun 1, 2024

		@@ -1014,6 +1014,46 @@ def _sympy_operator(self, child):
		return sympy.Symbol(latex_child)


		class BoundaryCellValue(BoundaryOperator):

		import pybamm


		class BasePhaseTransition(pybamm.BaseSubModel):

Pe degrade #3771

Are you sure you want to change the base?

Pe degrade #3771

Conversation

mzzhuo commented Jan 27, 2024

Description

Type of change

Key checklist:

Further checks:

kratman commented Mar 19, 2024

mzzhuo commented Mar 19, 2024

kratman commented Mar 19, 2024

rtimms 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

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

DrSOKane left a comment

Choose a reason for hiding this comment

mzzhuo commented Jun 1, 2024