-
-
Notifications
You must be signed in to change notification settings - Fork 551
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
Composite porosity #4417
base: develop
Are you sure you want to change the base?
Composite porosity #4417
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4417 +/- ##
========================================
Coverage 99.21% 99.21%
========================================
Files 302 302
Lines 22858 22879 +21
========================================
+ Hits 22679 22700 +21
Misses 179 179 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Simon! The PR looks nice, just the tests need addressing. Note that once the unit tests are updated to increase coverage, it will also have the same parameter issues as the integration tests.
That's odd. My working example ran just fine 10 days ago before I went on holiday, but it's now throwing the same parameter error as the automated tests. Was there a recent change to how phase parameters are handled? |
@parkec3 has also been working on some changes to make composite electrodes compatible with more cases |
It looks like the degradation parameters need to be updated to be defined by phase. I'm finishing up work getting the LAM submodel compatible with composite electrodes. I had to make a custom parameter set from Chen2020 composite and Ai2020 for my own local testing. The default parameter set doesn't have those defined. |
The SEI parameters are defined by phase, but not by domain, which was the underlying cause of the bug. I rewrote the code with if statements to cover three possibilities:
There is still a potential error if the electrode with SEI has one phase and the other has two. This can be fixed by giving all the SEI parameter domains, but that would be a separate PR... |
UPDATE: I've simply set it so that |
…composite-porosity
… into composite-porosity
Labelled this as skip release, given that we need to wait for #3457 |
… into composite-porosity
…composite-porosity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small couple of cosmetic changes
CHANGELOG.md
Outdated
@@ -30,6 +30,7 @@ | |||
- Added `BasicDFN` model for sodium-ion batteries ([#4451](https://github.com/pybamm-team/PyBaMM/pull/4451)) | |||
- 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be moved to the unreleased section
if domain == "separator": | ||
delta_eps_k = 0 # separator porosity does not change | ||
pass # separator porosity does not change | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be refactored to use
if domain != "separator"
…composite-porosity
… into composite-porosity
…composite-porosity
Description
Modified reaction_driven_porosity.py so that porosity change still works for composite electrodes
Fixes # (issue)
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.