Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify definition of PyBaMM's SPMe #4274

Open
brosaplanella opened this issue Jul 17, 2024 · 2 comments · May be fixed by #4329
Open

Clarify definition of PyBaMM's SPMe #4274

brosaplanella opened this issue Jul 17, 2024 · 2 comments · May be fixed by #4329
Assignees
@brosaplanella

Comments

@brosaplanella
Copy link
Member

brosaplanella commented Jul 17, 2024 •
edited
Loading

The definition of the SPMe remains a sticking point in PyBaMM. Even though the cited article for its definition is the Marquis et al (2019) paper, the actual implementation is a bit different (more accurate) as some terms have been "unlinearised"*. As part of improving the Creating Models notebooks (#3844) I wanted to write a BasicSPMe file and document the SPMe definition better. However, I have also noticed that the current approach falls somewhat in the middle: some terms are unlinearised and some are not. The marginal cost of doing the SPMe fully "unlinearised". All this discussion pertains to composite_conductivity.py

*by "unlinearised" I mean that some terms in the asymptotic expansion get later grouped again in their nonlinear form.

Current implementation

$$\phi_\mathrm{e,const} = - \overline{\Delta \phi}_\mathrm{n} + \overline{\phi}_\mathrm{n} - \left[ \chi(\overline{c}_\mathrm{e}) \frac{R T}{F} \frac{1}{L_\mathrm{n}} \int_0^{L_\mathrm{n}} \log \left(\frac{c_\mathrm{e,n}}{\overline c_\mathrm{e}} \right) \mathrm{d} x - i_\mathrm{app} L_\mathrm{n} \left(\frac{1}{3 \kappa_\mathrm{e,eff} (\overline{c}_\mathrm{e,n})} - \frac{1}{\kappa_\mathrm{e,eff} (\overline{c}_\mathrm{e,s})} \right)\right]$$ $$\phi_\mathrm{e,n} = \phi_\mathrm{e,const} + \chi(\overline{c}_\mathrm{e,n}) \frac{R T}{F} \log \left( \frac{c_\mathrm{e,n}}{\overline{c}_\mathrm{e}} \right) - \frac{i_\mathrm{app}}{\kappa_\mathrm{e,eff} (\overline{c}_\mathrm{e,n})} \frac{x_\mathrm{n}^2 - L_\mathrm{n}^2}{2 L_\mathrm{n}} - \frac{i_\mathrm{app} L_\mathrm{n}}{\kappa_\mathrm{e,eff} (\overline{c}_\mathrm{e,s})}$$ $$\phi_\mathrm{e,s} = \phi_\mathrm{e,const} + \chi(\overline{c}_\mathrm{e,s}) \frac{R T}{F} \log \left( \frac{c_\mathrm{e,s}}{\overline{c}_\mathrm{e}} \right) - \frac{i_\mathrm{app} x_\mathrm{s}}{\kappa_\mathrm{e,eff} (\overline{c}_\mathrm{e,s})}$$ $$\phi_\mathrm{e,p} = \phi_\mathrm{e,const} + \chi(\overline{c}_\mathrm{e,p}) \frac{R T}{F} \log \left( \frac{c_\mathrm{e,p}}{\overline{c}_\mathrm{e}} \right) - \frac{i_\mathrm{app} }{\kappa_\mathrm{e,eff} (\overline{c}_\mathrm{e,p})} \frac{x_\mathrm{p} (2 L_x - x_\mathrm{p}) + L_\mathrm{p}^2 - L_x^2}{2 L_\mathrm{p}}$$

Proposed implementation

$$\phi_\mathrm{e,const} = - \overline{\Delta \phi}_\mathrm{n} + \overline{\phi}_\mathrm{n} - \frac{R T}{F} \frac{1}{L_\mathrm{n}} \int_0^{L_\mathrm{n}} \int_0^x \chi(c_\mathrm{e,n}) \frac{\partial \log c_\mathrm{e,n}}{\partial s} \mathrm{d} s \mathrm{d} x + \frac{1}{L_\mathrm{n}} \int_0^{L_\mathrm{n}} \int_0^x \frac{i_\mathrm{e,n}}{\kappa_\mathrm{e,eff}(c_\mathrm{e,n})} \mathrm{d} s \mathrm{d} x$$ $$\phi_\mathrm{e,n} = \phi_\mathrm{e,const} + \frac{R T}{F} \int_0^x \chi(c_\mathrm{e,n}) \frac{\partial \log c_\mathrm{e,n}}{\partial s} \mathrm{d} s - \int_0^x \frac{i_\mathrm{e,n}}{\kappa_\mathrm{e,eff}(c_\mathrm{e,n})} \mathrm{d} s$$ $$\phi_\mathrm{e,s} = \phi_\mathrm{e,n} ( x = L_\mathrm{n}) + \frac{R T}{F} \int_{L_\mathrm{n}}^x \chi(c_\mathrm{e,s}) \frac{\partial \log c_\mathrm{e,s}}{\partial s} \mathrm{d} s - \int_{L_\mathrm{n}}^x \frac{i_\mathrm{e,s}}{\kappa_\mathrm{e,eff}(c_\mathrm{e,s})} \mathrm{d} s$$ $$\phi_\mathrm{e,p} = \phi_\mathrm{e,s} ( x = L_x - L_\mathrm{p}) + \frac{R T}{F} \int_{L_x - L_\mathrm{p}}^x \chi(c_\mathrm{e,p}) \frac{\partial \log c_\mathrm{e,p}}{\partial s} \mathrm{d} s - \int_{L_x - L_\mathrm{p}}^x \frac{i_\mathrm{e,p}}{\kappa_\mathrm{e,eff}(c_\mathrm{e,p})} \mathrm{d} s$$

I suggest moving to this version, which I believe is the most general one (I have dropped T dependence from parameters for simplicity, but will be accounted for). The version is taken from Brosa Planella & Widanage (2022), but the derivation is not explained with a lot of detail as the focus was the side reaction. My older paper Brosa Planella et al (2021) has a lot more detail but still assumes $\chi$ to be constant. This would make the integrated conductivity model redundant so we can get rid of it.
@brosaplanella brosaplanella self-assigned this Jul 17, 2024
@valentinsulzer
Copy link
Member

Sounds good to me

@ejfdickinson ejfdickinson mentioned this issue Jul 26, 2024
Open
@rtimms
Copy link
Contributor

rtimms commented Jul 26, 2024

Me too, thanks!

brosaplanella added a commit that referenced this issue Aug 9, 2024
@brosaplanella
#4274 fix small typo in docstring LaTeX rendering
8f255c3
brosaplanella added a commit that referenced this issue Aug 9, 2024
@brosaplanella
#4274 update electrolyte conductivity
e826821
@brosaplanella brosaplanella linked a pull request Aug 9, 2024 that will close this issue
Draft
12 tasks
brosaplanella added a commit that referenced this issue Aug 9, 2024
@brosaplanella
#4274 fully remove Integrated conductivity model
0d8be92
brosaplanella added a commit that referenced this issue Aug 9, 2024
@brosaplanella
#4274 fix test options
f1c876f
brosaplanella added a commit that referenced this issue Aug 25, 2024
@brosaplanella
#4274 fix issue with latexify
455d488
brosaplanella added a commit that referenced this issue Aug 28, 2024
@brosaplanella
#4274 start debugging shape error
bffbe81
brosaplanella added a commit that referenced this issue Aug 28, 2024
@brosaplanella
#4274 define electrolyte potential BCs for compatibility with thermal…
57bf506 
… models
brosaplanella added a commit that referenced this issue Oct 16, 2024
@brosaplanella
#4274 add SPM description
437ffc5
brosaplanella added a commit that referenced this issue Oct 16, 2024
@brosaplanella
#4274 update model definition
4db41b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brosaplanella brosaplanella

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update SPMe to generalised version pybamm-team/PyBaMM
3 participants
@valentinsulzer @brosaplanella @rtimms