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

Port lithium_ion_battery.m to Python #1263

Merged
merged 2 commits into from
Apr 30, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Apr 29, 2022

Changes proposed in this pull request

Port lithium_ion_battery.m to Python. Ultimately, this was the most efficient resolution.

If applicable, fill in the issue number this pull request is fixing

Closes #1259

If applicable, provide an example illustrating new features this pull request is introducing

Results ... @decaluwe, please confirm that this is consistent, as I currently don't have a MATLAB version installed.

lithium_ion_battery

If the CTI input is used, the output is zero (see #1256). I will add a unit test once #1256 is merged.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@speth
Copy link
Member

speth commented Apr 29, 2022

Here's the MATLAB version of that graph -- looks good to me:
image

@ischoegl ischoegl force-pushed the test-electron-density-formulation branch from 5421e42 to 65fad04 Compare April 30, 2022 00:52
@codecov
Copy link

codecov bot commented Apr 30, 2022

Codecov Report

Merging #1263 (5667358) into main (d2be7fc) will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1263      +/-   ##
==========================================
+ Coverage   65.56%   65.72%   +0.15%     
==========================================
  Files         329      329              
  Lines       46668    46671       +3     
  Branches    19855    19855              
==========================================
+ Hits        30598    30673      +75     
+ Misses      13508    13428      -80     
- Partials     2562     2570       +8     
Impacted Files Coverage Δ
src/thermo/ThermoPhase.cpp 68.46% <0.00%> (+0.10%) ⬆️
src/kinetics/Reaction.cpp 81.90% <0.00%> (+0.17%) ⬆️
src/base/ctml.cpp 43.33% <0.00%> (+0.83%) ⬆️
src/thermo/Species.cpp 78.94% <0.00%> (+1.05%) ⬆️
src/thermo/IdealSolidSolnPhase.cpp 63.51% <0.00%> (+1.47%) ⬆️
src/kinetics/importKinetics.cpp 80.95% <0.00%> (+1.58%) ⬆️
src/kinetics/InterfaceKinetics.cpp 75.67% <0.00%> (+3.21%) ⬆️
src/thermo/BinarySolutionTabulatedThermo.cpp 82.41% <0.00%> (+23.32%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ischoegl ischoegl marked this pull request as ready for review April 30, 2022 01:06
@ischoegl
Copy link
Member Author

This is ready for review … @decaluwe please feel free to push suggestions directly to this branch.

@ischoegl ischoegl requested a review from a team April 30, 2022 01:20
@ischoegl ischoegl force-pushed the test-electron-density-formulation branch from 65fad04 to 69186cd Compare April 30, 2022 14:51
speth
speth previously approved these changes Apr 30, 2022
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ischoegl, I think this is good. I had just a couple of very minor formatting comments.

@ischoegl
Copy link
Member Author

@speth ... thanks for the quick review! Re-approval is needed as I squashed the fixes ...

@ischoegl ischoegl requested a review from speth April 30, 2022 15:47
@speth speth merged commit 1132d40 into Cantera:main Apr 30, 2022
@ischoegl ischoegl deleted the test-electron-density-formulation branch April 30, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lithium-ion battery example (and unit tests) missing from Python
2 participants