set T_init as a function in the x domain #3151

tommaull · 2023-07-14T16:45:59Z

Description

Changed T_init to functional parameter in the x domain

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 (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
The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

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

tommaull · 2023-07-14T16:46:42Z

Lots of test errors, opening PR for visibility only.

tommaull · 2023-07-14T17:18:31Z

So I'm getting an error that I don't fully understand within the symbol.py, called through the equation tree. There appears to be math operation where two different domains are being subtracted. (Current collector and particle).

brosaplanella · 2023-07-17T07:48:15Z

My guess is that the errors are caused because now T_init is a function of x but this has not been changed in the other scripts, so you need to pass the additional argument to all instances of T_init.

valentinsulzer · 2023-07-17T14:07:43Z

You'll also need to make sure that you set pybamm.x_average(T_init(x)) as the initial condition for the lumped thermal model, and maybe do some broadcasting for the pouch models.

tommaull · 2023-07-18T13:36:05Z

OK, so got some more stuff working, but now it seems like there's some confusion around the domain variable between the submodels and the base model.

Error trace:

File c:\users\tom.maull\desktop\pybammdev\pybamm\pybamm\models\submodels\thermal\lumped.py:24, in Lumped.init(self, param, domain, options)
23 def init(self, param, domain, options=None):
---> 24 super().init(param, domain, options=options)
25 pybamm.citations.register("Timms2021")

File c:\users\tom.maull\desktop\pybammdev\pybamm\pybamm\models\submodels\thermal\base_thermal.py:23, in BaseThermal.init(self, param, domain, options)
21 def init(self, param, domain, options=None):
22 super().init(param, options=options)
---> 23 self.domain = domain

File c:\users\tom.maull\desktop\pybammdev\pybamm\pybamm\models\submodels\base_submodel.py:123, in BaseSubModel.domain(self, domain)
120 @domain.setter
121 def domain(self, domain):
122 if domain is not None:
--> 123 domain = domain.lower()
124 ok_domain_list = ["negative", "separator", "positive", None]
125 if domain in ok_domain_list:

tommaull · 2023-08-10T13:00:40Z

tommaull · 2023-08-22T08:28:34Z

Hey is anyone able to take a look at this one?

brosaplanella · 2023-08-29T14:57:53Z

pybamm/parameters/thermal_parameters.py

+        x = pybamm.SpatialVariable(
+            f"x_{domain[0]}",
+            domain=[f"{domain} electrode"],
+            auxiliary_domains={"secondary": "current collector"},
+            coord_sys="cartesian",
+        )


I think the issue with the tests is here as the domain is inherited from the for loop and it is not a string.

this should be the "whole cell" x, i.e.

whole_cell = ["negative electrode", "separator", "positive electrode"] x = pybamm.SpatialVariable( "x", domain=whole_cell, auxiliary_domains={"secondary": "current collector"}, coord_sys="cartesian", )

or, even better, just do pybamm.standard_spatial_vars.x which will fetch the same defn from standard_spatial_vars.py

brosaplanella · 2023-10-03T12:51:48Z

Hi @tommaull! Let me know if you have any questions about the comments above (not sure I was very clear haha).

tommaull · 2023-10-04T08:52:29Z

Hi @brosaplanella and @rtimms, here's the latest. I've definitely bitten off more than I should have here. Appreciate your help.

rtimms · 2023-10-05T15:01:04Z

pybamm/parameters/thermal_parameters.py

-        for domain in self.domain_params.values():
-            domain._set_parameters()
+        for domain, params in self.domain_params.items():
+            params._domain = domain  # Set _domain attribute


I don't think this is needed, since T is defined across all domains, not by component

rtimms · 2023-10-05T15:02:44Z

pybamm/parameters/thermal_parameters.py

+            auxiliary_domains={"secondary": "current collector"},
+            coord_sys="cartesian",
+        )
+        # Domain parameters


now this can go, for the same reasons as above

rtimms · 2023-10-05T15:03:32Z

pybamm/parameters/thermal_parameters.py

+        # )
+
+        self.T_init = pybamm.FunctionParameter(
+            f"{domain}Initial temperature in {domain} electrode [mol.m-3]",


just call it "Initial temperature [K]"

pybamm/models/submodels/thermal/base_thermal.py

rtimms · 2023-10-05T15:05:36Z

pybamm/models/submodels/thermal/lumped.py

@@ -67,4 +67,4 @@ def set_rhs(self, variables):

    def set_initial_conditions(self, variables):
        T_vol_av = variables["Volume-averaged cell temperature [K]"]
-        self.initial_conditions = {T_vol_av: self.param.T_init}
+        self.initial_conditions = {T_vol_av: pybamm.x_average(self.param.T_init)}


will also need to do this in the "x-lumped" models for pouch cell (in the folder submodels/thermal/pouch_cell/)

rtimms · 2023-10-05T15:06:48Z

thanks @tommaull it's getting there - just some confusion around the cell temperature being set on the "whole cell" domain instead of separately for negative electrode/separator/positive electrode

tommaull · 2023-10-27T18:17:07Z

Failing tests where non-lumped models are used due to mismatch between eqn and var domains.

Seems to be because the variable is set for Negative electrode temperature [K], Separator temperature [K], Positive electrode temperature [K] but the equation just has initial temp.

pybamm/parameters/thermal_parameters.py

rtimms · 2023-11-27T16:24:31Z

pybamm/parameters/thermal_parameters.py

@@ -28,17 +31,39 @@ def __init__(self):

    def _set_parameters(self):
        """Defines the dimensional parameters"""
-        for domain in self.domain_params.values():
-            domain._set_parameters()
+        for domain, params in self.domain_params.items():


I think this should just be

for domain in self.domain_params.values(): domain._set_parameters()

rtimms · 2023-11-27T16:29:46Z

For the x-full model you need to update the boundary conditions to be

    def set_initial_conditions(self, variables):
        T = variables["Cell temperature [K]"]
        T_cn = variables["Negative current collector temperature [K]"]
        T_cp = variables["Positive current collector temperature [K]"]
        T_init = self.param.T_init
        self.initial_conditions = {
            T_cn: pybamm.boundary_value(T_init, "left"),
            T: T_init,
            T_cp: pybamm.boundary_value(T_init, "right"),
        }

so that the current collector initial temperatures are T_init at the left and right (since T_init is the temperature in the domain neg/sep/pos.

It looks like you'll also need to update the BasicSPM and Basic DFN models since they are written assuming a uniform temperature.

tommaull · 2023-12-05T15:30:37Z

So I've been trying to suss out the reason for the domain mismatch, and it seems to be, at least partially, because the x_average or Xaverage functions only return a value in the secondary domain. I'm not sure I understand why this occurs?

valentinsulzer · 2023-12-05T18:50:24Z

domain can be thought of as "the primary spatial dimension along which this variable changes" which is why XAverage has this behavior. T(x,y,z,t) changes primarily along the through-cell (x) dimension, then secondarily along the current collector (y,z) dimension(s), its x-average is T_av(y,z,t) which changes primarily along the current collector dimension. Depending on the geometry the "current collector" dimension might just be 0D. Variables that depend only on time have no spatial dimension hence no domain.

If you want a variable with primary domains matching the original variable, you can re-broadcast

pybamm.PrimaryBroadcast(T_av, ["negative electrode", "separator", "positive electrode"])

tommaull mentioned this pull request Jul 24, 2023

Assign a spacial temperature vector as initial temperatrue field for DFN model #3131

Open

tommaull marked this pull request as ready for review August 10, 2023 13:00

brosaplanella reviewed Aug 29, 2023

View reviewed changes

rtimms reviewed Oct 5, 2023

View reviewed changes

pybamm/models/submodels/thermal/base_thermal.py Outdated Show resolved Hide resolved

rtimms reviewed Oct 5, 2023

View reviewed changes

tommaull commented Nov 24, 2023

View reviewed changes

pybamm/parameters/thermal_parameters.py Outdated Show resolved Hide resolved

rtimms reviewed Nov 27, 2023

View reviewed changes

tommaull closed this Apr 16, 2024

tommaull force-pushed the develop branch from 0beaff2 to cf09d5e Compare April 16, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set T_init as a function in the x domain #3151

set T_init as a function in the x domain #3151

tommaull commented Jul 14, 2023 •

edited

Loading

tommaull commented Jul 14, 2023 •

edited

Loading

tommaull commented Jul 14, 2023

brosaplanella commented Jul 17, 2023

valentinsulzer commented Jul 17, 2023

tommaull commented Jul 18, 2023

tommaull commented Aug 10, 2023

tommaull commented Aug 22, 2023

brosaplanella Aug 29, 2023

rtimms Oct 5, 2023

brosaplanella commented Oct 3, 2023

tommaull commented Oct 4, 2023

rtimms Oct 5, 2023

rtimms Oct 5, 2023

rtimms Oct 5, 2023

rtimms Oct 5, 2023

rtimms commented Oct 5, 2023

tommaull commented Oct 27, 2023

rtimms Nov 27, 2023

rtimms commented Nov 27, 2023

tommaull commented Dec 5, 2023

valentinsulzer commented Dec 5, 2023

set T_init as a function in the x domain #3151

set T_init as a function in the x domain #3151

Conversation

tommaull commented Jul 14, 2023 • edited Loading

Description

Type of change

Key checklist:

Further checks:

tommaull commented Jul 14, 2023 • edited Loading

tommaull commented Jul 14, 2023

brosaplanella commented Jul 17, 2023

valentinsulzer commented Jul 17, 2023

tommaull commented Jul 18, 2023

tommaull commented Aug 10, 2023

tommaull commented Aug 22, 2023

brosaplanella Aug 29, 2023

Choose a reason for hiding this comment

rtimms Oct 5, 2023

Choose a reason for hiding this comment

brosaplanella commented Oct 3, 2023

tommaull commented Oct 4, 2023

rtimms Oct 5, 2023

Choose a reason for hiding this comment

rtimms Oct 5, 2023

Choose a reason for hiding this comment

rtimms Oct 5, 2023

Choose a reason for hiding this comment

rtimms Oct 5, 2023

Choose a reason for hiding this comment

rtimms commented Oct 5, 2023

tommaull commented Oct 27, 2023

rtimms Nov 27, 2023

Choose a reason for hiding this comment

rtimms commented Nov 27, 2023

tommaull commented Dec 5, 2023

valentinsulzer commented Dec 5, 2023

tommaull commented Jul 14, 2023 •

edited

Loading

tommaull commented Jul 14, 2023 •

edited

Loading