Issue 1778 - heat of mixing #2837
Conversation
pybamm-team#1778 add heat of mixing option
This reverts commit 3ad0c75.
brosaplanella
requested review from
valentinsulzer and
TomTranter
as code owners
| a_n = variables["Negative electrode surface area to volume ratio [m-1]"] | ||
| R_n = variables["Negative particle radius [m]"] | ||
| N_n = a_n / (4 * pi * R_n**2) | ||
| if self.x_average: |
There was a problem hiding this comment.
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.
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.
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) to broadcast(2 * var), but there is an edge case here that hadn't come up before
There was a problem hiding this comment.
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 and T_n to be the non-averaged quantities?
There was a problem hiding this comment.
Yes, it should work just always using the x_average is False branch
There was a problem hiding this comment.
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.
Ah damn ok. Let's just merge as is then. My guess is that U(broadcast(c)) becomes broadcast(U(c)) and then it doesn't find broadcast(c) in the children so it thinks the derivative is zero
…and compute the derivative in the model directly instead
|
@all-contributors |
|
I've put up a pull request to add @ikorotkin! 🎉 We had trouble processing your request. Please try again later. |
|
We'll need to request additions for each person in separate comments |
|
Yes, according to the bot docs what I have done should have worked (https://allcontributors.org/docs/en/bot/usage) but for some reason it didn't. |
Maybe it's because the full stops are missing after each line? That's the only difference I can see. Anyway, silly bot :) |
|
@all-contributors add @smitasahu2 for code |
|
@smitasahu2 already contributed before to code |
|
@all-contributors add @Afgr1087 for code |
|
I've put up a pull request to add @Afgr1087! 🎉 |
|
@all-contributors add @smitasahu2 for code |
|
@smitasahu2 already contributed before to code |
* fix * fix exception * added a Newfile * pseudo * pybamm-team#1778 add heat of mixing option * pybamm-team#1778 fixed indentation * pybamm-team#1778 fixed test * pybamm-team#1778 fixed test * pybamm-team#1788 added heat of mixing tests * First commit * pybamm-team#1778 fixed test * First iteration * pybamm-team#1788 try to fix diff of U * first push from mac * Fixed full broadcast * Fixed domain in the integral * pybamm-team#1778 fix parameter values example * Added children[0].children[0] * Added half cell * style: pre-commit fixes * pybamm-team#1788 revert some unrelated changes * change ocv hack * Revert "change ocv hack" This reverts commit 3ad0c75. * pybamm-team#1778 fix heat of mixing * pybamm-team#1839 fix thermal submodels to take x_average * pybamm-team#1778 add example heat of mixing * ruff * style: pre-commit fixes * pybamm-team#1778 fix lead acid models * fix SPM with the right broadcast of temperature * pybamm-team#1778 refactor heat of mixing * style: pre-commit fixes * Add Richardson2021 citation for heat of mixing * Fixed heat of mixing sign * Rewritten heat of mixing example, added comparison plot to compare with the paper * Fixed ruff formatting errors in the heat of mixing example script * pybamm-team#1778 fix x-full * pybamm-team#1778 fix tests * pybamm-team#1778 improve coverage * style: pre-commit fixes * pybamm-team#1778 Valentin's suggestion to remove the dUdsto function and compute the derivative in the model directly instead * pybamm-team#1778 fix failing test * pybamm-team#1778 add heat of mixing to CHANGELOG --------- Co-authored-by: Alec Bills <[email protected]> Co-authored-by: smitasahu2 <[email protected]> Co-authored-by: Afgr1087 <[email protected]> Co-authored-by: Ferran Brosa Planella <[email protected]> Co-authored-by: Ivan Korotkin <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
implemented heat of mixing
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #1778
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.
Key checklist:
$ pre-commit run(see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all$ python run-tests.py --doctestYou can run unit and doctests together at once, using
$ python run-tests.py --quick.Further checks: