-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add electrode balancing example #472
Add electrode balancing example #472
Conversation
Thanks @brosaplanella, I like your use of the Thevenin model in this example, but unfortunately it doesn't seem to work on the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #472 +/- ##
===========================================
+ Coverage 98.70% 98.76% +0.05%
===========================================
Files 48 48
Lines 3172 3153 -19
===========================================
- Hits 3131 3114 -17
+ Misses 41 39 -2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
I just merged develop and the tests are passing. The only one that breaks it the Lychee one, as for some reason it picks up a string I define in the notebook that is not complete (I append the extra bit later on). Not sure if there is a way to tell Lychee to ignore that (@agriyakhetarpal might know). Otherwise, I will hardcode each of the URLs which should make the test pass. |
It passes when pip installing from |
Yes, you can add an exclusion for the broken link to the Lychee workflow file: PyBOP/.github/workflows/lychee_links.yaml Line 39 in 1a76470
It would look like this: |
…lla/PyBOP into issue-222-electrode-balancing
I have now made Lychee ignore the incomplete URL so it passes. About the issue with develop branch, I tried creating a new virtual environment and installing the develop branch and the example still runs as expected. I also double checked and the GitHub actions should install the version in the tested branch, which I have now made sure is up-to-date with develop. If someone else try to reproduce Nicola's that would be great! |
Thanks for the update! I tested again and now think the issue may not be due to the branch, just some initial values. Please could you try an initial soc >= 0.5 and capacity >= 50? In my case, Other than that, just two things, please:
|
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.
Nice work, thanks @brosaplanella !
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 great, thanks @brosaplanella! The bottom figure will be correctly rendered when #410 lands with nbviewer, so that should be sorted soon.
@all-contributors please add @brosaplanella for examples |
I've put up a pull request to add @brosaplanella! 🎉 |
Description
Added an example for electrode balancing. The main issue was defining the model, for now I have hacked it via an ECM but this means we can only balance half cells. In the future it would be interesting to define specific models for electrode balancing, which could support full cells too.
Issue reference
Fixes #222
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
Type of change
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)$ nox -s tests
$ nox -s doctest
You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.