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.

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

[Bug]: create_from_bpx(): effective values of diffusivity activation energies are overwritten by "electrolyte conductivity activation energy" #3051

Closed
ejfdickinson opened this issue Jun 19, 2023 · 3 comments · Fixed by #3242
Assignees
Labels
bug Something isn't working difficulty: easy A good issue for someone new. Can be done in a few hours priority: high To be resolved as soon as possible

Comments

@ejfdickinson
Copy link

PyBaMM Version

23.4

Python Version

3.9.16

Describe the bug

When temperature-dependent functions are created in create_from_bpx(), the apparent activation energy for the following callable parameter definitions in the generated parameter_values instance is replaced by the value of "Electrolyte conductivity activation energy [J.mol-1]".

This appears to be a late-binding bug due to the repeat use of non-local variable E_a in the declaration these functions, which is repeatedly redefined, the electrolyte conductivity activation energy being the last such redefinition.

Affected functions:

  • "Negative electrode diffusivity [m2.s-1]"
  • "Positive electrode diffusivity [m2.s-1]"
  • "Electrolyte diffusivity [m2.s-1]"

Note: identified in 23.4 but no relevant code changes in latest version

Steps to Reproduce

Using nmc_pouch_cell_BPX.json from the BPX example download:

import pybamm
from pybamm import constants, exp, log

# From BPX example download
params = pybamm.ParameterValues.create_from_bpx("nmc_pouch_cell_BPX.json")

Ea_cond_el   = params["Electrolyte conductivity activation energy [J.mol-1]"]
Ea_diff_neg  = params["Negative electrode diffusivity activation energy [J.mol-1]"]

# Evaluate negative electrode diffusivity at Tref
T_ref        = params["Reference temperature [K]"]
Ds_neg_ref   = params["Negative electrode diffusivity [m2.s-1]"](0.5, T_ref)

# Evaluate negative electrode diffusivity at Ttest =/= Tref
T_test       = 290
Ds_neg_test  = params["Negative electrode diffusivity [m2.s-1]"](0.5, T_test)

# Evaluate expected negative electrode diffusivity at Ttest
D_neg_test_expected = Ds_neg_ref * exp(-Ea_diff_neg / constants.R * (1/T_test - 1/T_ref))

# Evaluate apparent activation energy
Ea_diff_neg_apparent = -constants.R / (1/T_test - 1/T_ref) * log(Ds_neg_test/Ds_neg_ref)

print("Negative electrode diffusivity (test temperature, expected):", D_neg_test_expected)
print("Negative electrode diffusivity (test temperature, actual):", Ds_neg_test)

print("Negative electrode diffusivity activation energy (stated):", Ea_diff_neg)
print("Negative electrode diffusivity activation energy (apparent):", Ea_diff_neg_apparent)

print("Electrolyte conductivity activation energy (stated):", Ea_cond_el)

Relevant log output

Negative electrode diffusivity (test temperature, expected): 1.941507320004235e-14
Negative electrode diffusivity (test temperature, actual): 2.2472548468421424e-14
Negative electrode diffusivity activation energy (stated): 30000.0
Negative electrode diffusivity activation energy (apparent): 17099.999999999996
Electrolyte conductivity activation energy (stated): 17100.0
@ejfdickinson ejfdickinson added the bug Something isn't working label Jun 19, 2023
@ejfdickinson
Copy link
Author

Simplest fix, if cause is what it looks like - use distinct variable names for all activation energies in bpx.py.

@valentinsulzer valentinsulzer added difficulty: easy A good issue for someone new. Can be done in a few hours priority: high To be resolved as soon as possible labels Jun 19, 2023
darryl-ad added a commit to darryl-ad/PyBaMM that referenced this issue Aug 7, 2023
@darryl-ad
Copy link
Contributor

Fixed as a pre-cursor to #3225

@darryl-ad
Copy link
Contributor

This PR now also fixes #3225

@brosaplanella brosaplanella linked a pull request Aug 7, 2023 that will close this issue
8 tasks
brosaplanella pushed a commit that referenced this issue Sep 19, 2023
* add unique variable names for activation energies

* Update CHANGELOG.md [#3051]

* Update CHANGELOG.md

* fix Ea_k verbose name to match schema

* fix formatting

* refactor arrhenius definition

* add arrhenius unit test

* add test for BPX constant functions

* update changelog

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working difficulty: easy A good issue for someone new. Can be done in a few hours priority: high To be resolved as soon as possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants