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 method incompatible with "lumped" thermal model #2860

Closed
darryl-ad opened this issue Apr 5, 2023 · 14 comments
Closed
Assignees
Labels
bug Something isn't working difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows

Comments

@darryl-ad
Copy link
Contributor

PyBaMM Version

23.1

Python Version

3.9.12

Describe the bug

The parameter set created using the create_from_BPX method is not compatible with the "lumped" thermal model.

The following issues exist:

  1. The "lumped" model calls the current collector variables, which create_from_BPX does not generate. These include density, heat capacity, thermal conductivity, thickness, and conductivity.

  2. The specific heat capacities generated by create_from_BPX are of units "[J.K-1.kg-1]" instead of the "[J.kg-1.K-1]" expected by PyBaMM. This applies to the negative electrode, positive electrode, and separator specific heat capacities.

Steps to Reproduce

params = pybamm.ParameterValues.create_from_bpx("exampleBPX.json")

sim = pybamm.Simulation( pybamm.lithium_ion.DFN(options={"thermal":"lumped", "cell geometry": "arbitrary"}), parameter_values=params )

sol = sim.solve()

Relevant log output

{
	"name": "KeyError",
	"message": "\"'Negative current collector density [kg.m-3]' not found. Best matches are ['Negative electrode density [kg.m-3]', 'Positive electrode density [kg.m-3]', 'Separator density [kg.m-3]']\"",
	....
@darryl-ad darryl-ad added the bug Something isn't working label Apr 5, 2023
@rtimms
Copy link
Contributor

rtimms commented Apr 5, 2023

Thanks for pointing this out! Currently we assign the lumped value to each component but we miss the current collectors. Should be an easy fix by adding the current collectors domains to this line. Do you think you could have a go?

@darryl-ad
Copy link
Contributor Author

Thanks @rtimms, I'm happy to give this a go. However I don't think your quick fix will cover all the issues listed, just the current collector density, heat capacity, and thermal conductivity. Let me know what you think of the following:

  • The current collector thickness and conductivity parameters are still required by the "lumped" thermal model, but aren't part of BPX. What would be the repurcussions of zeroing these parameters out?
  • I'll rearrange the units in this line to address point 2.

@rtimms
Copy link
Contributor

rtimms commented Apr 11, 2023

  • The current collector thicknesses will be arbitrary for this setup - they’re only used to compute a weighted mean of the thermal properties, but since they’re hard-coded to be the same the end result won’t care what the thickness are. So, for a quick fix, either zero out or pick some nominal value.
  • Thanks.

The better fix for this would be to use the lumped properties directly if you choose {“thermal”: “lumped”, “cell geometry”: “arbitrary”} and compute them by component if you choose {“thermal”: “lumped”, “cell geometry”: “pouch”}. Similar to how we do surface area for cooling and cell volume here. This calculation could probably be moved to LithiumIonParameters.

@rtimms
Copy link
Contributor

rtimms commented May 2, 2023

@darryl-ad any progress on this?

@valentinsulzer valentinsulzer added difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows labels May 8, 2023
rtimms added a commit that referenced this issue May 16, 2023
rtimms added a commit that referenced this issue May 16, 2023
@darryl-ad
Copy link
Contributor Author

darryl-ad commented May 17, 2023

@rtimms sorry I've been on holiday. I'll can get to it soon if you still need, so you can include it in the 23.5 release.

I just need to get up to speed on your contribution guidelines and dev install instructions

@rtimms
Copy link
Contributor

rtimms commented May 17, 2023

Thanks, @darryl-ad! @RuiheLi has started looking at this so I'll let you two coordinate

@darryl-ad
Copy link
Contributor Author

@RuiheLi I'm working on this now. Took me a long time to get a PyBaMM dev install working with all tests passing.

@RuiheLi
Copy link
Contributor

RuiheLi commented May 25, 2023

Hi~ @darryl-ad, I will start working on it today.

@darryl-ad
Copy link
Contributor Author

@RuiheLi I'm set up now so I'm happy to do it :)

@rtimms
Copy link
Contributor

rtimms commented May 25, 2023

@darryl-ad thanks! Let us know if there any installation issues we can fix or areas we can improve the docs

@RuiheLi
Copy link
Contributor

RuiheLi commented May 25, 2023

@darryl-ad Thanks! I am very happy to discuss with you if you need any support.

@darryl-ad
Copy link
Contributor Author

@RuiheLi @rtimms that should be fixed now.

  • Added domains for the current collector
  • Corrected the wrong unit notation for heat capacity
  • Assigned arbitrary values for current collector thickness and conductivity (0 m and 4e7 S/m)

I left the total heat transfer coefficient out, but if you want it might make sense to add a default value of 0 to allow simulations to run without any extra user parameters required.

I agree with Rob that these are quick-fix solutions, and also recommend that the assignment of the component thermal properties should be moved elsewhere.

I also recommend correcting the specific heat capacity units in the BPX code, then removing my correction here. Some extra unit tests to cover BPX functionality might be needed too!

@darryl-ad
Copy link
Contributor Author

PS - I left my solutions to the installation issues I encountered here

rtimms added a commit that referenced this issue May 26, 2023
@rtimms
Copy link
Contributor

rtimms commented May 30, 2023

Fixed by #2987

@rtimms rtimms closed this as completed May 30, 2023
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: medium To be resolved if time allows
Projects
None yet
Development

No branches or pull requests

4 participants