Added "use lumped thermal capacity" option in lumped thermal model. by Rishab87 · Pull Request #4968 · pybamm-team/PyBaMM

Rishab87 · 2025-04-10T12:33:07Z

Description

Added "use lumped thermal capacity" option in lumped thermal model so that we can simulate properly for those who do not have detailed thermal characterization of each component of the cell.

Fixes #4612

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

No style issues: nox -s pre-commit
All tests pass: nox -s tests
The documentation builds: nox -s doctests
Code is commented for hard-to-understand areas
Tests added that prove fix is effective or that feature works

codecov · 2025-04-10T14:03:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.64%. Comparing base (da9846f) to head (fbe028f).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4968   +/-   ##
========================================
  Coverage    98.64%   98.64%           
========================================
  Files          304      304           
  Lines        23742    23750    +8     
========================================
+ Hits         23421    23429    +8     
  Misses         321      321

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aabills

Thanks @Rishab87 !

Overall it looks really great, there are just a few small issues.

I'd also like to see an integration test where we ensure that the equation is doing what it is supposed to be doing, maybe by back calculating the lumped capacity and comparing it with the standard thermal model.

aabills · 2025-04-10T17:15:20Z

        "Initial concentration in negative electrode [mol.m-3]": 24108.0,
        "Initial concentration in positive electrode [mol.m-3]": 21725.0,
        "Initial temperature [K]": 298.15,
+        "Cell heat capacity [J.K-1.m-3]": 2.5e6,


In general, we try not to add parameters that were not in the original paper to the parameter sets, so could you delete this from all the parameter sets?

aabills · 2025-04-10T17:16:12Z

            "voltage as a state": ["false", "true"],
            "working electrode": ["both", "positive"],
            "x-average side reactions": ["false", "true"],
+            "use lumped capacity": ["false", "true"],


Let's rename this to "use lumped thermal capacity" for clarity, given that batteries also have an charge capacity.

aabills · 2025-04-10T17:18:01Z

+        y_eval = np.zeros((sim._built_model.len_rhs, 1))
+        heat_capacity_eval = var.evaluate(t_eval, y_eval)
+
+        assert abs(heat_capacity_eval - lumped_value) < lumped_value * 1e-10


I'm not sure that this is a great test. What is this trying to test exactly?

Also you can use np.assert_almost_equal

I've changed this unit test a bit, it checks whether the heat capacity parameter is being used correctly when the option is enabled.

aabills · 2025-04-10T17:19:37Z

        self.h_total = self.therm.h_total
        self.rho_c_p_eff = self.therm.rho_c_p_eff
        self.lambda_eff = self.therm.lambda_eff
+        self.cell_heat_capacity = self.therm.cell_heat_capacity


This is the same thing as rho_c_p_eff right? So why make a new parameter for it?

I think rho_c_p_eff calculates the effective volumetric heat capacity of the entire battery cell by combining the heat capacities of each component but cell_heat_capacity is the lumped heat capacity of the whole cell, we can define this parameter directly in lumped.py too but I made it like this for consistency of the codebase

yeah I think it makes sense to keep it separate after further consideration.

…/PyBaMM into properly-lumped-thermal

Rishab87 · 2025-04-11T14:00:01Z

@aabills thanks for the review!
I've added an integration test and made changes accordingly.

rtimms · 2025-04-11T20:42:15Z

@@ -224,6 +224,10 @@ class BatteryModelOptions(pybamm.FuzzyDict):
                the respective porosity change) over the x-axis in Single Particle
                Models, can be "false" or "true". Default is "false" for SPMe and
                "true" for SPM.


Can you add a check that raises an OptionError for incompatible options? There are some examples of this further down in this file.

ohh right, missed that, adding

rtimms · 2025-04-11T20:43:24Z

            Q_cr_W = pybamm.Scalar(0)
            Q_cr_vol_av = Q_cr_W

+        # Add lumped heat capacity if option is enabled


It might be cleaner to have this option check in the base class rather than overwriting the variable for the lumped model.

aabills · 2025-04-12T00:02:21Z

Hey @Rishab87 I'll re-review once you've implemented Rob's changes

Rishab87 · 2025-04-12T11:11:53Z

@rtimms, thanks for the review
I've made those changes.

@aabills it's ready to be re-reviewed

aabills

Ok at this point it looks great substantively. The only thing is, the testing in unit/ doesn't really match what we did for the other models. Could you please add it to unit/test_models/test_full_battery_models/base_lithium_ion_tests.py and base_lithium_ion_half_cell_tests.py? You should be able to do both the option error and the check well posedness there. And then move the parameter test from unittests to test_thermal_parameters, which is in test_lithium_ion_parameters.py

Thanks so much!

aabills · 2025-04-12T18:02:29Z

        self.h_total = self.therm.h_total
        self.rho_c_p_eff = self.therm.rho_c_p_eff
        self.lambda_eff = self.therm.lambda_eff
+        self.cell_heat_capacity = self.therm.cell_heat_capacity


yeah I think it makes sense to keep it separate after further consideration.

Rishab87 · 2025-04-13T05:43:45Z

@aabills, shifted those tests and changed them accordingly

aabills

Looks awesome, thanks a lot!!

Rishab87 added 3 commits April 10, 2025 17:32

use lumped thermal capacity option and few tests

6c03c77

minor changes

87caab4

cleanups

c408f89

Rishab87 requested a review from a team as a code owner April 10, 2025 12:33

added to changelog

6b28947

Merge branch 'develop' into properly-lumped-thermal

34525c3

aabills requested changes Apr 10, 2025

View reviewed changes

Rishab87 added 2 commits April 11, 2025 19:16

made changes according to comments

9242c5c

Merge branch 'properly-lumped-thermal' of https://github.com/Rishab87…

72f9982

…/PyBaMM into properly-lumped-thermal

minor change in docstring

d776f73

Rishab87 requested a review from aabills April 11, 2025 14:02

rtimms reviewed Apr 11, 2025

View reviewed changes

added options and moved lumped capacity handling to BaseThermal

23d3664

aabills requested changes Apr 12, 2025

View reviewed changes

changed and shifted unit tests to suggested files

fbe028f

Rishab87 requested a review from aabills April 13, 2025 05:43

Merge branch 'develop' into properly-lumped-thermal

ccf2121

aabills approved these changes Apr 14, 2025

View reviewed changes

aabills merged commit 9989403 into pybamm-team:develop Apr 14, 2025

Uh oh!

Uh oh!

Conversation

Rishab87 commented Apr 10, 2025

Description

Type of change

Important checks:

Uh oh!

codecov Bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aabills left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rishab87 Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rishab87 commented Apr 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rishab87 Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabills commented Apr 12, 2025

Uh oh!

Rishab87 commented Apr 12, 2025

Uh oh!

aabills left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rishab87 commented Apr 13, 2025

Uh oh!

aabills left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 10, 2025 •

edited

Loading

Rishab87 Apr 11, 2025 •

edited

Loading

Rishab87 Apr 12, 2025 •

edited

Loading