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

Li metal spm #1731

Merged
merged 10 commits into from
Nov 5, 2021
Merged

Li metal spm #1731

merged 10 commits into from
Nov 5, 2021

Conversation

valentinsulzer
Copy link
Member

Description

Add li metal (half cell) models for SPMe and SPM

Also fixes #1720

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.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ flake8
  • All tests pass: $ python run-tests.py --unit
  • The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@valentinsulzer
Copy link
Member Author

@brosaplanella now ready to review

@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #1731 (bfd4df3) into develop (77f7169) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1731   +/-   ##
========================================
  Coverage    99.28%   99.28%           
========================================
  Files          343      343           
  Lines        18911    19004   +93     
========================================
+ Hits         18776    18869   +93     
  Misses         135      135           
Impacted Files Coverage Δ
...m/models/full_battery_models/base_battery_model.py 99.71% <100.00%> (ø)
...ttery_models/lithium_ion/base_lithium_ion_model.py 100.00% <100.00%> (ø)
...bamm/models/full_battery_models/lithium_ion/dfn.py 100.00% <100.00%> (ø)
...bamm/models/full_battery_models/lithium_ion/spm.py 100.00% <100.00%> (ø)
...amm/models/full_battery_models/lithium_ion/spme.py 100.00% <100.00%> (ø)
...mm/models/submodels/electrode/ohm/composite_ohm.py 100.00% <100.00%> (ø)
...bamm/models/submodels/electrode/ohm/leading_ohm.py 100.00% <100.00%> (ø)
pybamm/models/submodels/electrode/ohm/li_metal.py 100.00% <100.00%> (ø)
...lyte_conductivity/base_electrolyte_conductivity.py 98.98% <100.00%> (+0.02%) ⬆️
...electrolyte_conductivity/composite_conductivity.py 100.00% <100.00%> (ø)
... and 6 more

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 77f7169...bfd4df3. Read the comment docs.

Copy link
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Looks good, just a few comments. Might need to fix the scikits issue first to check if this passes the tests though.

delta_phi_s_dim = pot_scale * delta_phi_s

variables = {
"Negative electrode potential drop": delta_phi_s,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have negative electrode variables in the lithium metal model?

Copy link
Member Author

Choose a reason for hiding this comment

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

Potential drop across the li metal anode (linear)



class TestDFNWithSEI(unittest.TestCase):
def test_well_posed_constant(self):
Copy link
Member

Choose a reason for hiding this comment

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

Does SEI still work if the working electrode is the positive one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The SEI grows on the surface of the li metal electrode

import unittest


class TestSPMHalfCell(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Here they are called TestSPMHalfCell as opposed to TestSPM as before. It would be good to unify the notation.

valentinsulzer and others added 3 commits November 4, 2021 13:34
…thium_ion/test_spme_half_cell.py

Co-authored-by: Ferran Brosa Planella <[email protected]>
…thium_ion/test_spm_half_cell.py

Co-authored-by: Ferran Brosa Planella <[email protected]>
@valentinsulzer valentinsulzer merged commit 7b748f8 into develop Nov 5, 2021
@valentinsulzer valentinsulzer deleted the li-metal-spm branch November 5, 2021 15:00
@brosaplanella
Copy link
Member

@tinosulzer you didn't add a line to CHANGELOG for this, but you can introduce it through #1782

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.

sim.plot() crashes for DFN half-cell
2 participants