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]: Suface form effective conductivity in separator #3495

Closed
Scottmar93 opened this issue Nov 2, 2023 · 3 comments · Fixed by #3905
Closed

[Bug]: Suface form effective conductivity in separator #3495

Scottmar93 opened this issue Nov 2, 2023 · 3 comments · Fixed by #3905
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

@Scottmar93
Copy link
Contributor

PyBaMM Version

23.5

Python Version

23.5

Describe the bug

On lines 81 and 85, of pybamm/models/submodels/electrolyte_conductivity/surface_potential_form/full_surface_form_conductivity.py
is seems that the separator porosity is used in the calculation of the effective conductivity in the separator instead of the separator transport efficiency. In the end, I think this is only an order of a few mV errors in the predicted voltage.

image

Steps to Reproduce

N/A

Relevant log output

No response

@Scottmar93 Scottmar93 added the bug Something isn't working label Nov 2, 2023
@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 Nov 2, 2023
@TomTranter
Copy link
Contributor

Separators usually have pretty low tortuosity but it's worth including the change you suggest for sure. I can make the change as I'm familiar with transport efficiency and have just been revamping it. There might be other instances in the code like this I can check for too

@TomTranter TomTranter self-assigned this Nov 13, 2023
@rtimms
Copy link
Contributor

rtimms commented Jan 15, 2024

@TomTranter any update on this?

@TomTranter
Copy link
Contributor

Completely forgot about it

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