-
-
Notifications
You must be signed in to change notification settings - Fork 535
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
Issue 1778 - heat of mixing #2837
Merged
Merged
Changes from all commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
e06d5b9
fix
aabills 688c9a0
fix exception
aabills 0d84720
Merge branch 'pybamm-team:develop' into energy-summary
aabills 1cd0655
Merge branch 'pybamm-team:develop' into issue-1778
aabills 6dbe8d6
added a Newfile
smitasahu2 69e6ba6
pseudo
aabills fccea75
Merge branch 'issue-1778' of github.com:abillscmu/PyBaMM into issue-1778
aabills beb5205
#1778 add heat of mixing option
Afgr1087 161dc82
#1778 fixed indentation
Afgr1087 93777da
#1778 fixed test
Afgr1087 908632a
#1778 fixed test
Afgr1087 6c92466
#1788 added heat of mixing tests
brosaplanella 0da6f2e
First commit
ikorotkin 35f96e3
#1778 fixed test
Afgr1087 f39cc6f
Merge branch 'issue-1778-test' of github.com:abillscmu/PyBaMM into is…
brosaplanella f6fc9d1
First iteration
ikorotkin 53d3078
Merge pull request #5 from abillscmu/issue-1778-test
aabills 2463a0b
#1788 try to fix diff of U
brosaplanella dda5340
first push from mac
smitasahu2 549e1e2
Fixed full broadcast
ikorotkin 67a2e34
Fixed domain in the integral
ikorotkin 33e8726
#1778 fix parameter values example
brosaplanella c60d6eb
Added children[0].children[0]
ikorotkin cd1f868
Added half cell
ikorotkin 7c1812d
Merge branch 'pybamm-team:develop' into issue-1778
aabills 4bc354e
style: pre-commit fixes
pre-commit-ci[bot] d2e6274
#1788 revert some unrelated changes
brosaplanella 3ad0c75
change ocv hack
aabills 537b2bf
Revert "change ocv hack"
aabills a5e3b06
Merge branch 'develop' into issue-1778
brosaplanella 9817dfc
#1778 fix heat of mixing
brosaplanella 7881045
#1839 fix thermal submodels to take x_average
brosaplanella 969b464
#1778 add example heat of mixing
brosaplanella c07899d
ruff
brosaplanella e910e77
style: pre-commit fixes
pre-commit-ci[bot] 6402448
#1778 fix lead acid models
brosaplanella 00cee21
fix SPM with the right broadcast of temperature
brosaplanella 7d54a60
#1778 refactor heat of mixing
brosaplanella 8380ec4
style: pre-commit fixes
pre-commit-ci[bot] 76bae06
Add Richardson2021 citation for heat of mixing
ikorotkin bb6134e
Fixed heat of mixing sign
ikorotkin 442f2d4
Rewritten heat of mixing example, added comparison plot to compare wi…
ikorotkin cfaf2c0
Fixed ruff formatting errors in the heat of mixing example script
ikorotkin 420bb78
Merge remote-tracking branch 'upstream/develop' into issue-1778
brosaplanella e90356e
Merge branch 'issue-1778' of github.com:abillscmu/PyBaMM into issue-1778
brosaplanella 65d4a08
#1778 fix x-full
brosaplanella 892ed8f
Merge remote-tracking branch 'upstream/develop' into issue-1778
brosaplanella d894297
#1778 fix tests
brosaplanella f1605bd
#1778 improve coverage
brosaplanella 5818458
style: pre-commit fixes
pre-commit-ci[bot] 8ab206a
#1778 Valentin's suggestion to remove the dUdsto function and compute…
brosaplanella accde29
#1778 fix failing test
brosaplanella 2c7e8df
Merge branch 'issue-1778' of github.com:abillscmu/PyBaMM into issue-1778
brosaplanella a1b05bd
#1778 add heat of mixing to CHANGELOG
brosaplanella a88c897
Merge branch 'develop' into issue-1778
brosaplanella File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
# | ||
# Compare SPMe model with and without heat of mixing | ||
# | ||
import pybamm | ||
import numpy as np | ||
import matplotlib.pyplot as plt | ||
|
||
pybamm.set_logging_level("INFO") | ||
|
||
# load models | ||
models = [ | ||
pybamm.lithium_ion.SPMe( | ||
{"heat of mixing": "true", "thermal": "lumped"}, name="SPMe with heat of mixing" | ||
), | ||
pybamm.lithium_ion.SPMe({"thermal": "lumped"}, name="SPMe without heat of mixing"), | ||
] | ||
|
||
# set parametrisation (Ecker et al., 2015) | ||
parameter_values = pybamm.ParameterValues("Ecker2015") | ||
|
||
# set mesh | ||
# (increased number of points in particles to avoid oscillations for high C-rates) | ||
var_pts = {"x_n": 16, "x_s": 8, "x_p": 16, "r_n": 128, "r_p": 128} | ||
|
||
# set the constant current discharge C-rate | ||
C_rate = 5 | ||
|
||
# set the simulation time and increase the number of points | ||
# for better integration in time | ||
t_eval = np.linspace(0, 720, 360) | ||
|
||
# set up an extra plot with the heat of mixing vs time in each electrode and | ||
# the integrated heat of mixing vs time in each electrode to compare with | ||
# Figure 6(a) from Richardson et al. (2021) | ||
fig, axs = plt.subplots(2, len(models), figsize=(12, 7)) | ||
|
||
# extract some of the parameters | ||
L_n = parameter_values["Negative electrode thickness [m]"] | ||
L_p = parameter_values["Positive electrode thickness [m]"] | ||
A = parameter_values["Electrode width [m]"] * parameter_values["Electrode height [m]"] | ||
|
||
# create and run simulations | ||
sims = [] | ||
for m, model in enumerate(models): | ||
sim = pybamm.Simulation( | ||
model, parameter_values=parameter_values, var_pts=var_pts, C_rate=C_rate | ||
) | ||
sim.solve(t_eval) | ||
sims.append(sim) | ||
|
||
# extract solution for plotting | ||
sol = sim.solution | ||
|
||
# extract variables from the solution | ||
time = sol["Time [h]"].entries | ||
Q_mix = sol["Heat of mixing [W.m-3]"].entries | ||
|
||
# heat of mixing in negative and positive electrodes multiplied by the electrode | ||
# width, represents the integral of heat of mixing term across each of the | ||
# electrodes (W.m-2) | ||
Q_mix_n = Q_mix[0, :] * L_n | ||
Q_mix_p = Q_mix[-1, :] * L_p | ||
|
||
# heat of mixing integrals (J.m-2) | ||
Q_mix_n_int = 0.0 | ||
Q_mix_p_int = 0.0 | ||
|
||
# data for plotting | ||
Q_mix_n_plt = [] | ||
Q_mix_p_plt = [] | ||
|
||
# performs integration in time | ||
for i, t in enumerate(time[1:]): | ||
dt = (t - time[i]) * 3600 # seconds | ||
Q_mix_n_avg = (Q_mix_n[i] + Q_mix_n[i + 1]) * 0.5 | ||
Q_mix_p_avg = (Q_mix_p[i] + Q_mix_p[i + 1]) * 0.5 | ||
# convert J to kJ and divide the integral by the electrode area A to compare | ||
# with Figure 6(a) from Richardson et al. (2021) | ||
Q_mix_n_int += Q_mix_n_avg * dt / 1000 / A | ||
Q_mix_p_int += Q_mix_p_avg * dt / 1000 / A | ||
Q_mix_n_plt.append(Q_mix_n_int) | ||
Q_mix_p_plt.append(Q_mix_p_int) | ||
|
||
# plots heat of mixing in each electrode vs time in minutes | ||
axs[0, m].plot(time * 60, Q_mix_n, ls="-", label="Negative electrode") | ||
axs[0, m].plot(time * 60, Q_mix_p, ls="--", label="Positive electrode") | ||
axs[0, m].set_title(f"{model.name}") | ||
axs[0, m].set_xlabel("Time [min]") | ||
axs[0, m].set_ylabel("Heat of mixing [W.m-2]") | ||
axs[0, m].grid(True) | ||
axs[0, m].legend() | ||
|
||
# plots integrated heat of mixing in each electrode vs time in minutes | ||
axs[1, m].plot(time[1:] * 60, Q_mix_n_plt, ls="-", label="Negative electrode") | ||
axs[1, m].plot(time[1:] * 60, Q_mix_p_plt, ls="--", label="Positive electrode") | ||
axs[1, m].set_xlabel("Time [min]") | ||
axs[1, m].set_ylabel("Integrated heat of mixing [kJ.m-2]") | ||
axs[1, m].grid(True) | ||
axs[1, m].legend() | ||
|
||
# plot | ||
pybamm.dynamic_plot( | ||
sims, | ||
output_variables=[ | ||
"X-averaged cell temperature [K]", | ||
"X-averaged heat of mixing [W.m-3]", | ||
"X-averaged total heating [W.m-3]", | ||
"Heat of mixing [W.m-3]", | ||
"Voltage [V]", | ||
"Current [A]", | ||
], | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug in the expression tree but it should be possible to treat x-averaged and non-x-averaged the same way. Happy to leave this workaround in for now until the bug is resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it for now, as most submodels use this anyway. When fixing the bug, it should be caught with the Ctrl+F for
if self.x_average
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to leave it. Just a clarification about why this is different from most other submodels: In the other submodels, we are specifying whether the variable we are solving for is full or x-averaged (e.g. full or x-averaged particle concentration, if we used full concentration in SPM we'd just be solving the exact same equation 20 times instead of 1). Here, we're just evaluating an expression and returning an object of the same shape at the end, and the expression tree logic should be able to figure out how to deal with the full and x-averaged cases with the same syntax by doing things like rewrite
2 * broadcast(var)
tobroadcast(2 * var)
, but there is an edge case here that hadn't come up beforeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I thought it was the same case. Then happy to fix it here. Is it a matter of removing the if statement and using
c_n
andT_n
to be the non-averaged quantities?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should work just always using the
x_average is False
branchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just gave it a try and now there is an issue with dU/dsto returning 0 for the SPMe. Not sure if that's related to the other PR or not (need to dig more on it). I won't have time to take a look in the next few days, so happy to hold this until I have time or just merge it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah damn ok. Let's just merge as is then. My guess is that
U(broadcast(c))
becomesbroadcast(U(c))
and then it doesn't findbroadcast(c)
in the children so it thinks the derivative is zero