-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Improved lithium-ion battery cti file and Matlab example #637
Conversation
samples/matlab/lithium_ion_battery.m
Outdated
% Input parameters | ||
SOC = 0:0.02:1; % [-] Input state of charge (0...1) | ||
X_Li_an = (0.75-0.01)*SOC+0.01; % anode balancing | ||
X_Li_ca = (0.99-0.49)*(1-SOC)+0.49; % cathode balancing |
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.
Tiniest little quibble here, @wbessler - might it be clearer for the user if this balancing were made more explicit?
- Would it make sense to specify the upper and lower SOC limits as variables? I'm not sure if a user would ever want to change them, so there's a risk there, but then the user would understand what 0.01, 0.75, et al. represent
- Or would it be better to just add a comment, explaining what we mean by balancing?
- Lastly, the comment you have removed above, which commented on the anode and cathode capacities being perfectly balanced, has some relevant info here, as well. Just thinking of ways to make the example a little more transparent to somebody not well versed in battery modeling.
Codecov Report
@@ Coverage Diff @@
## master #637 +/- ##
==========================================
- Coverage 70.78% 70.78% -0.01%
==========================================
Files 373 373
Lines 43447 43447
==========================================
- Hits 30754 30752 -2
- Misses 12693 12695 +2
Continue to review full report at Codecov.
|
86975ed
to
c4ef74e
Compare
Thanks, @wbessler - these changes all look good to me. I caught one small typo in the matlab example, which I corrected, and also converted the hand-coded Faraday constant to instead use the function now included in the matlab toolbox. There was something weird with a rebase I did on my local machine, so I had to do a force push to get this back to the correct state. @speth and @bryanwweber - any comments? I think this should be ready to go. |
It seems like there's a problem with the lithium_ion_battery.yaml input file, the elements key cannot contain an empty list it seems like. I'm not sure why only the macOS build is passing. @decaluwe I think the tests should pass before we merge this. |
Extended and clarified comments, corrected density data, improved functionality
MATLAB example: better comments, faster calculation, consistent signs; CTI file: thermally-activated kinetics
Corrected one typo (stray mid-line comment symbol) and converted hard-coded faraday constant to the corresponding Matlab toolbox function (added with PR Cantera#640).
I think the YAML conversion issue should be fixed. I also removed some end-of-line whitespace from the CTI and Matlab files. Otherwise, this looks OK to me. |
Extended and clarified comments, corrected density data, improved functionality
Please fill in the issue number this pull request is fixing:
Changes proposed in this pull request: