Skip to content
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

Merged
merged 4 commits into from
Jun 27, 2019

Conversation

wbessler
Copy link
Contributor

@wbessler wbessler commented May 6, 2019

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:

  • the lithium_ion_battery.cti and .m files had minor errors (wrong density of the materials, missing T AND P setting inialization of the interfaces, unrealistic stoichiometry ranges)
  • comments were extended and improved for more clarity, and entries in cti file were re-ordered.

% 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
Copy link
Member

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.

@decaluwe
Copy link
Member

decaluwe commented Jun 7, 2019

Thanks, @wbessler - I think these changes are all pretty clearly improvements! I have one minor question on the matlab example (see earlier comment), but other than that @speth I think this is good to go.

@codecov
Copy link

codecov bot commented Jun 26, 2019

Codecov Report

Merging #637 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/transport/GasTransport.cpp 90.37% <0%> (-0.21%) ⬇️
src/thermo/ThermoPhase.cpp 67.88% <0%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae792dd...29de6c5. Read the comment docs.

@decaluwe
Copy link
Member

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.

@bryanwweber
Copy link
Member

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.

wbessler and others added 4 commits June 26, 2019 18:08
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).
@speth
Copy link
Member

speth commented Jun 26, 2019

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.

@speth speth merged commit edcc9c5 into Cantera:master Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants