Make particle size distribution work with composite electrode.#4687
Make particle size distribution work with composite electrode.#4687aabills merged 21 commits intopybamm-team:developfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4687 +/- ##
========================================
Coverage 99.22% 99.22%
========================================
Files 302 303 +1
Lines 23000 23070 +70
========================================
+ Hits 22821 22891 +70
Misses 179 179 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
valentinsulzer
left a comment
There was a problem hiding this comment.
Generally looks fine to me, the proof will be in the testing and coverage. A lot of places have unnecessarily duplicated code but not a hill I'll die on
…mpatibility' into improve-submodel-compatibility merge
|
s/b gtg |
| name, domains=symbol.domains, coord_sys="cartesian" | ||
| ) | ||
| if ["negative particle size"] in symbol.domains.values(): | ||
| if ["negative particle size"] in symbol.domains.values() or [ |
There was a problem hiding this comment.
FYI geo.domain_params["negative"] is equivalent to geo.n
There was a problem hiding this comment.
I'm not sure how to interpret this comment, as far as I can tell, I never use geo.domain_params['negative']. Is there something here you want changed?
There was a problem hiding this comment.
sorry I wasn’t clear. I meant that instead of doing an if statement by domain and getting eg geo.n you can do geo.domain_params[domain]. More of a comment then necessarily a suggestion / required change.
There was a problem hiding this comment.
looks like I left this comment in the wrong place too lol
There was a problem hiding this comment.
sounds good, thanks
Description
Makes particle size distribution work with composite electrode.
Test is a bit weird, the standard model test fails due to some shape error -- I'll debug that tomorrow.
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.
Key checklist:
$ 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)$ python -m pytest(or$ nox -s tests)$ python -m pytest --doctest-plus src(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick.Further checks: