-
-
Notifications
You must be signed in to change notification settings - Fork 667
Make particle size distribution work with composite electrode. #4687
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
Make particle size distribution work with composite electrode. #4687
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 |
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.
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 |
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.
thanks @aabills looks great, just a few minor comments
) | ||
if ["negative particle size"] in symbol.domains.values(): | ||
if ["negative particle size"] in symbol.domains.values() or [ |
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.
FYI geo.domain_params["negative"]
is equivalent to geo.n
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like I left this comment in the wrong place too lol
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.
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: