-
-
Notifications
You must be signed in to change notification settings - Fork 546
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
solid tau now uses correct bruggeman coeff #1773
solid tau now uses correct bruggeman coeff #1773
Conversation
Since this is my first pull request i appreciate feedback for handling this process. |
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 good, but there is a test related to the sensitivities which is not passing. @martinjrobins is it just a matter of the tolerances or is there a bigger issue?
Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>
Let's try using even tighter tolerances for solving in the sensitivities test: change these both to 1e-12 PyBaMM/tests/integration/test_models/standard_model_tests.py Lines 113 to 114 in 269629f
Also, could you make a new section "Unreleased" for the changelog? i.e. at the top add
Since these changes aren't in the 21.10 release |
I tested this locally and the sensitivity test passed. Can you make the changelog update, merge develop, and push again? |
@dion-w any update on this? |
Haven't been active here recently. Will hopefully be after a deadline on thursday. |
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 @dion-w !
@all-contributors please add @dion-w for bug reports and code |
I've put up a pull request to add @dion-w! 🎉 |
The notebook tests are failing because we need to provide the Bruggeman parameters for the electrode in cell [16] of https://github.com/pybamm-team/PyBaMM/blob/develop/examples/notebooks/parameterization/parameterization.ipynb |
after fixing the solid tortuosities these new values are needed to run the notebook
Codecov Report
@@ Coverage Diff @@
## develop #1773 +/- ##
========================================
Coverage 99.26% 99.26%
========================================
Files 345 345
Lines 18900 18900
========================================
Hits 18761 18761
Misses 139 139
Continue to review full report at Codecov.
|
Description
Solid tortuosity now uses solid bruggeman coefficient of respective electrode and not the bruggeman coefficients of the electrolyte.
Fixes #1734
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:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: