-
-
Notifications
You must be signed in to change notification settings - Fork 543
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
Other useful variables #2740
Other useful variables #2740
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2740 +/- ##
========================================
Coverage 99.68% 99.68%
========================================
Files 270 271 +1
Lines 18949 18991 +42
========================================
+ Hits 18890 18932 +42
Misses 59 59
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This PR looks nasty but it's mainly a lot of renaming variables, as detailed in CHANGELOG |
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.
Looks good, thanks! Just a couple of small comments
CHANGELOG.md
Outdated
## Features | ||
|
||
- Renamed "Terminal voltage [V]" to just "Voltage [V]". "Terminal voltage [V]" can still be used and will return the same value as "Voltage [V]". | ||
- Added "Anode potential [V]", which is the value of the surface potential difference (`phi_s - phi_e`) at the anode/separator interface, commonly controlled in fast-charging algorithms to avoid plating. Also added "Cathode potential [V]", which is the value of the surface potential difference at the cathode/separator interface. ([#2740](https://github.com/pybamm-team/PyBaMM/pull/2740)) |
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.
Shouldn't it be "Positive electrode" and "Negative electrode" so it is consistent with the other variables?
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.
The full name for this would be "Negative electrode surface potential difference at the separator interface". But I think in the fast-charging literature it's just called "Anode potential". Which do you think is better here?
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.
Mmmm good point. The first choice is a bit of a mouthful, but I think we should use it to be consistent (i.e. use positive and negative electrode everywhere, rather than mixing it with anode and cathode). Especially, as just calling it "Anode potential" would be confusing with the existing "Negative electrode potential [V]", right?
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 true, I'll change it to the longer one
Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>
This caused liionpack to break as I was using the Measured battery ocv. Also when I changed to use the new OCV results looked strange and so I'm now using surface ocv. |
I think it might be nice to have some example of what they all mean and where they are used |
What was "Measured open circuit voltage [V]" has been renamed to "Surface open-circuit voltage [V]". This is the OCV calculated from the surface concentration. "Open-circuit voltage [V]" is calculated from the bulk. Since the thing you would actually measure as (pseudo) OCV in an experiment is the surface OCV, I wonder if "Open-circuit voltage [V]" and "Surface open-circuit voltage [V]" should be the same, and we should also have "Bulk open-circuit voltage [V]". We could explain this more thoroughly in the plot voltage components notebook |
I agree if the more observable one is Surface that should be the "Open-circuit voltage [V]", what is the bulk one used for? |
So you can separate out the effect of concentration gradients within the particle on the voltage |
I think the "Open-circuit voltage" should be the one that you would eventually relax to if you turned the current off, i.e. the bulk one. What makes the surface one more observable? |
It's just a point of notation where we should be consistent. If you asked people "what is the OCV?" in their model they would probably say |
Got it. There's also the option of just not defining "Open-circuit voltage" so you have to specify bulk or surface explicitly each time. We should update this either way before releasing |
Description
Add some useful variables, fix plot_voltage_components and add example
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 --doctest
You can run unit and doctests together at once, using
$ python run-tests.py --quick
.Further checks: