Skip to content

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

❌ 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).
⚠️ Report is 567 commits behind head on develop.

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

@kratman
Copy link
Contributor

kratman commented Jul 2, 2025

@DrSOKane Are you still working on this?

@kratman kratman added the needs-reply Needs further information from the author and may be closed if no response is received label Jul 2, 2025
@DrSOKane
Copy link
Contributor Author

DrSOKane commented Jul 3, 2025

@DrSOKane Are you still working on this?

Not recently. #4869 is much higher priority.

@github-actions github-actions bot removed the needs-reply Needs further information from the author and may be closed if no response is received label Jul 3, 2025
@kratman
Copy link
Contributor

kratman commented Jul 3, 2025

Can we close this one for now, there are a huge number of conflicts

@DrSOKane DrSOKane closed this Jul 3, 2025
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