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.
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
#3611 use actual cell volume for average total heating #3707
#3611 use actual cell volume for average total heating #3707
Changes from 2 commits
d348a62
650f435
56539de
d9a18d6
5457e74
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
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.
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.
I avoided "total" as we already use that to mean "sum of irreversible and reversible", but happy to take suggestions
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.
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.
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.
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.
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.
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?