#3611 use actual cell volume for average total heating

[rtimms]

Description

Computes the average heat source over the entire cell volume, not just the electrode volume.

Changes:

• The parameters GeometricParameters.A_cooling and GeometricParameters.V_cell are now computed from the electrode heights, widths and thicknesses if the "cell geometry" option is "pouch" and from the parameters "Cell cooling surface area [m2]" and "Cell volume [m3]", respectively, otherwise.
• New variables "Ohmic heating per unit electrode-pair area [W.m-2]", "Irreversible electrochemical heating per unit electrode-pair area [W.m-2]", "Reversible heating per unit electrode-pair area [W.m-2]" and "Total heating per unit electrode-pair area [W.m-2]" that correspond to the various heat source terms per unit area of simulated electrode
• The variables "Volume-averaged ... heating [W.m-3]" are computed by integrating over the simulated electrode volume (of volume L*L_y*L_z) and then dividing by the actual cell volume GeometricParameters.V_cell.

Example:

import pybamm

model = pybamm.lithium_ion.DFN(options={"thermal": "lumped"})

experiment = pybamm.Experiment(["Rest for 1 s", "Discharge at 1C until 3.6 V"])

parameter_values = pybamm.ParameterValues("ORegan2022")
parameter_values["Cell volume [m3]"] = 70e-3 * ((21e-3 / 2) ** 2) * 3.14159

sim = pybamm.Simulation(
    model=model, experiment=experiment, parameter_values=parameter_values
)

sol = sim.solve()

contributing_layers = [
    "Negative current collector",
    "Negative electrode",
    "Separator",
    "Positive electrode",
    "Positive current collector",
]

thickness = sum(
    [parameter_values[layer + " thickness [m]"] for layer in contributing_layers]
)
electrode_volume = (
    thickness
    * parameter_values["Electrode height [m]"]
    * parameter_values["Electrode width [m]"]
)

print(
    "Difference of local and global volumetric heat source:",
    sol["X-averaged total heating [W.m-3]"].entries
    - sol["Volume-averaged total heating [W.m-3]"].entries,
)

print(
    "Difference of volume-corrected local and global volumetric heat source:",
    sol["X-averaged total heating [W.m-3]"].entries * electrode_volume
    - sol["Volume-averaged total heating [W.m-3]"].entries
    * parameter_values["Cell volume [m3]"],
)
print("Global (cell) volume:", parameter_values["Cell volume [m3]"])
print("Local (modelled) volume:", electrode_volume)

Fixes #3611

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[rtimms]

ejfdickinson @darryl-ad can you take a look to see if this fixes the issue, please?

@Saransh-cpp this is a bug fix that would be good to get into 24.1 once it is approved

[darryl-ad]

@rtimms I'll leave the semantics discussion to Edmund, but I can confirm that this produces results in line with what we expected to see for Volume-averaged cell temperature [K] and Volume-averaged total heating [W.m-3].

However, should you also account for Number of electrode pairs connected in parallel to make a cell (see line comment)?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3e57733) 99.59% compared to head (5457e74) 99.59%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3707   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20798    20811   +13     
========================================
+ Hits         20713    20726   +13     
  Misses          85       85           

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

[brosaplanella]

Looks good, thanks!

[brosaplanella]

Not totally convinced by the name. I find clearer "Total reversible heating per unit electrode-pair area [W.m-2]", but at the same time I can see how a user might find this more confusing, so happy to leave it like that.

In my mind, "total" already means the integrated value and the "per ... area" and units make it clear enough how it has been integrated.

[rtimms]

I avoided "total" as we already use that to mean "sum of irreversible and reversible", but happy to take suggestions

[ejfdickinson]

I'd suggest that you just delete the word "Integrated"; I agree with @brosaplanella that "per __" intrinsically makes clear how the quantity is scaled. You have long established the approach of using the same name with different units to account for normalisation, and I think that is also fine.

If you are always using "Total" to mean "sum of irreversible and irreversible" then the name "Total reversible heating" is malformed, though.

In the realm of breaking changes, I would call the [W.m-3] quantities a "volumetric heat source" and the [W] quantities a "heat rate".

Also (pedantically), "ohmic" should be lowercase (c.f. voltaic, faradaic). Not worth changing if breaking.

[rtimms]

Thanks, yes, those “Total…” shouldn’t be there, not sure why I put them there. Probably not worth changing the capitalisation of Ohmic.

To avoid any other breaking changes I’ll keep the same names and infer things from units.

[rtimms]

I remember now, the “Total” came from copy the definition of the existing “Total heating [W]” variable. I’ll make these changes. Otherwise, does this look correct?

[rtimms]

@ejfdickinson I'll wait for your comments before merging, but want to get this merged soon so it is in the next release - thanks!

[ejfdickinson]

Added one comment under the nomenclature discussion - the implementation seems fine based on @darryl-ad 's review.

[ejfdickinson]

I'd suggest that you just delete the word "Integrated"; I agree with @brosaplanella that "per __" intrinsically makes clear how the quantity is scaled. You have long established the approach of using the same name with different units to account for normalisation, and I think that is also fine.

If you are always using "Total" to mean "sum of irreversible and irreversible" then the name "Total reversible heating" is malformed, though.

In the realm of breaking changes, I would call the [W.m-3] quantities a "volumetric heat source" and the [W] quantities a "heat rate".

Also (pedantically), "ohmic" should be lowercase (c.f. voltaic, faradaic). Not worth changing if breaking.

[rtimms]

@martinjrobins @jsbrittain this JAX test failure seems unrelated to the changes here. any idea what the issue may be?