#872 added ambient temperature #873

viviantran27 · 2020-03-05T15:32:06Z

Description

Added ambient temperature

Fixes #872

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: $ flake8
All tests pass: $ python run-tests.py --unit
The documentation builds: $ cd docs and then $ make clean; make html

You can run all three 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

codecov · 2020-03-05T16:58:45Z

Codecov Report

Merging #873 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #873   +/-   ##
========================================
  Coverage    97.70%   97.70%           
========================================
  Files          201      201           
  Lines        10415    10433   +18     
========================================
+ Hits         10176    10194   +18     
  Misses         239      239

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ff39b9...e6c877b. Read the comment docs.

rtimms

This is a nice feature, thanks! Just a couple of comments re the stuff in 2D. I think we could improve the documentation for how to write the source terms in finite elements. It is a bit hacky at the moment, and could be reviewed in the future.

rtimms · 2020-03-05T17:15:59Z

pybamm/models/submodels/thermal/isothermal/isothermal.py

@@ -33,6 +35,10 @@ def get_fundamental_variables(self):
        T_cp = T_x_av

        variables = self._get_standard_fundamental_variables(T, T_cn, T_cp)
+        variables.update(


could this be moved to base_thermalto avoid repeating in each of the separate base classes?

rtimms · 2020-03-05T17:17:30Z

pybamm/models/submodels/thermal/x_lumped/x_lumped_2D_current_collectors.py

-                pybamm.laplacian(T_av)
-                + self.param.B * pybamm.source(Q_av, T_av)
-                + cooling_coeff * pybamm.source(T_av, T_av)
+                pybamm.laplacian(T_av - T_amb)


I don't think there should be a T_amb inside the laplacian

rtimms · 2020-03-05T17:19:08Z

pybamm/models/submodels/thermal/x_lumped/x_lumped_2D_current_collectors.py

-                + self.param.B * pybamm.source(Q_av, T_av)
-                + cooling_coeff * pybamm.source(T_av, T_av)
+                pybamm.laplacian(T_av - T_amb)
+                + self.param.B * pybamm.source(Q_av, T_av - T_amb)


the second argument in pybamm.source is just the variable for which this is the equation for and the first argument is the source term in the equation (so that boundary conditions are handled correctly in finite elements). sorry, I don't think that is particularly well documented. As a result this should just be pybamm.source(Q_av, T_av)

same for laplacian in other files

ohh i see, thanks for the clarification

rtimms · 2020-03-05T17:19:43Z

pybamm/models/submodels/thermal/x_lumped/x_lumped_2D_current_collectors.py

-                + cooling_coeff * pybamm.source(T_av, T_av)
+                pybamm.laplacian(T_av - T_amb)
+                + self.param.B * pybamm.source(Q_av, T_av - T_amb)
+                + cooling_coeff * pybamm.source(T_av, T_av - T_amb)


similar to above this should be pybamm.source T_av-T_amb,T_av)

rtimms · 2020-03-05T17:20:17Z

pybamm/models/submodels/thermal/x_lumped/x_lumped_2D_current_collectors.py

                - (self.param.h / self.param.delta)
-                * pybamm.source(T_av, T_av, boundary=True)
+                * pybamm.source(T_av - T_amb, T_av - T_amb, boundary=True)


pybamm.source T_av-T_amb,T_av, boundary=True)

rtimms · 2020-03-06T09:15:03Z

pybamm/models/submodels/thermal/x_lumped/x_lumped_1D_current_collectors.py

+                pybamm.laplacian(T_av - T_amb)
+                + self.param.B * Q_av
+                + cooling_coeff * (T_av - T_amb)
+            )
            / self.param.C_th
        }


just thought - for the model x-lumped with 1D current collectors I think you also need to change the boundary conditions to T-T_amb as well. the colling in the rhs should just be from the large faces of the cell, and the boundary conditions are cooling around the remaining edges (y,z). it shouldn't actually make that much differences as the cooling from those sides is small by comparison. I think the details of these model are a bit opaque -- I'll make a separate issue to simplify the thermal models and make the cooling and boundary conditions more explicit

rtimms · 2020-03-06T09:21:46Z

is there an implicit assumption that the dimensionless ambient temp is zero at t=t0? i.e. you start at the reference temperature? if so, maybe there could be a check for this?

viviantran27 · 2020-03-06T19:14:55Z

is there an implicit assumption that the dimensionless ambient temp is zero at t=t0? i.e. you start at the reference temperature? if so, maybe there could be a check for this?

Should we keep T_init separate from T_amb(0)? For example, it could be used to simulate a case where an EV exits an air-conditioned garage to a different temperature outside or other rapid temperature change. Can you clarify what sort of check you're imagining?

rtimms · 2020-03-06T20:01:13Z

is there an implicit assumption that the dimensionless ambient temp is zero at t=t0? i.e. you start at the reference temperature? if so, maybe there could be a check for this?

Should we keep T_init separate from T_amb(0)? For example, it could be used to simulate a case where an EV exits an air-conditioned garage to a different temperature outside or other rapid temperature change. Can you clarify what sort of check you're imagining?

ah yeah I hadn’t thought of that use case. Yeah I agree to keep T_init and T_amb(0) separate. I had in my head that the battery would start at the ambient temperature, but I see now that isn’t the case, so no need to check. Thanks!

rtimms

great, thanks!

rtimms · 2020-03-09T13:16:56Z

examples/scripts/DFN_ambient_temperature.py

@@ -0,0 +1,52 @@
+#
+# Example showing how to load and solve the DFN


the title could be updated to explain this is showing the ambient temperature feature

I'll merge this PR and make that change before merging #868

viviantran27 added 2 commits March 5, 2020 10:25

pybamm-team#872 added ambient temperature

30fe709

merge upstream

6430b02

valentinsulzer requested review from Scottmar93 and rtimms March 5, 2020 16:10

pass lead-acid model tests by adding heat transfer coeff to csv

b514d7e

rtimms requested changes Mar 5, 2020

View reviewed changes

rtimms reviewed Mar 6, 2020

View reviewed changes

Robb's comments

e6c877b

valentinsulzer requested a review from rtimms March 6, 2020 20:55

rtimms approved these changes Mar 9, 2020

View reviewed changes

valentinsulzer merged commit a9b1081 into pybamm-team:develop Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#872 added ambient temperature #873

#872 added ambient temperature #873

viviantran27 commented Mar 5, 2020

codecov bot commented Mar 5, 2020 •

edited

Loading

rtimms left a comment

rtimms Mar 5, 2020

rtimms Mar 5, 2020

rtimms Mar 5, 2020

rtimms Mar 5, 2020

viviantran27 Mar 6, 2020

rtimms Mar 5, 2020

rtimms Mar 5, 2020

rtimms Mar 6, 2020 •

edited

Loading

rtimms commented Mar 6, 2020

viviantran27 commented Mar 6, 2020

rtimms commented Mar 6, 2020

rtimms left a comment

rtimms Mar 9, 2020

valentinsulzer Mar 9, 2020

		@@ -0,0 +1,52 @@
		#
		# Example showing how to load and solve the DFN

#872 added ambient temperature #873

#872 added ambient temperature #873

Conversation

viviantran27 commented Mar 5, 2020

Description

Type of change

Key checklist:

Further checks:

codecov bot commented Mar 5, 2020 • edited Loading

Codecov Report

rtimms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtimms Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

rtimms commented Mar 6, 2020

viviantran27 commented Mar 6, 2020

rtimms commented Mar 6, 2020

rtimms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2020 •

edited

Loading

rtimms Mar 6, 2020 •

edited

Loading