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

Domain degradation parameters #4463

Draft
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

DrSOKane
Copy link
Contributor

Description

Added domains to the SEI and plating degradation parameters, allowing them to be different on each electrode. This is useful for negative half-cells, where the positive electrode is graphite and the negative electrode is lithium metal.

  • The names of almost all SEI and plating parameters have been changed, making this a breaking change
  • All built-in parameter sets have been updated to work with the new syntax
  • The example notebook for half-cells has been updated to showcase the new feature. By adjusting the SEI parameters on each electrode, it is shown that SEI on the lithium metal electrode can provide enough resistance to prevent plating on the graphite electrode.
  • A parameter set for a half-cell with a graphite+silicon composite has been added. I considered adding this to the example notebook but it is already very long and I still don't have much experience using the composite model.

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: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

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

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 58.19672% with 51 lines in your changes missing coverage. Please review.

Project coverage is 99.23%. Comparing base (1b6ef03) to head (01ed5ed).

Files with missing lines Patch % Lines
...ameters/lithium_ion/Chen2020_composite_halfcell.py 0.00% 51 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4463      +/-   ##
===========================================
- Coverage    99.46%   99.23%   -0.23%     
===========================================
  Files          293      294       +1     
  Lines        22384    22447      +63     
===========================================
+ Hits         22264    22276      +12     
- Misses         120      171      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Thanks Simon, given that this is a breaking change I'd be inclined to wait and lump it in with the breaking changes that will come from the parameters refactor ... @BradyPlanden @kratman when is that likely to come in?

Comment on lines 4 to 8

- Added sensitivity calculation support for `pybamm.Simulation` and `pybamm.Experiment` ([#4415](https://github.com/pybamm-team/PyBaMM/pull/4415))
- Added OpenMP parallelization to IDAKLU solver for lists of input parameters ([#4449](https://github.com/pybamm-team/PyBaMM/pull/4449))
- Porosity change now works for composite electrode ([#4417](https://github.com/pybamm-team/PyBaMM/pull/4417))
- Added sensitivity calculation support for `pybamm.Simulation` and `pybamm.Experiment` ([#4415](https://github.com/pybamm-team/PyBaMM/pull/4415))
- Added phase-dependent particle options to LAM #4369
Copy link
Contributor

Choose a reason for hiding this comment

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

PR #4417 is still open this probably should not be in the log. Not sure if those changes are also in here, but we should probably either separate these PRs or close the other PR.

@DrSOKane DrSOKane marked this pull request as draft September 25, 2024 12:22
@DrSOKane
Copy link
Contributor Author

This PR follows on from PR #4417 and does indeed contain everything from there. I've decided to convert this PR to draft and reopen when #4417 - which has far more use cases in practice - is complete.

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.

3 participants