-
-
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
Newman tobias model #1423
Newman tobias model #1423
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1423 +/- ##
========================================
Coverage 98.27% 98.28%
========================================
Files 279 280 +1
Lines 16131 16165 +34
========================================
+ Hits 15853 15887 +34
Misses 278 278
Continue to review full report at Codecov.
|
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! Need to do changelog
@@ -95,14 +95,3 @@ def set_initial_conditions(self, variables): | |||
c_e = variables["Electrolyte concentration"] | |||
|
|||
self.initial_conditions = {c_e: self.param.c_e_init} | |||
|
|||
def set_boundary_conditions(self, variables): |
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 think at some point we moved these into the submodels to be clearer what the submodels are. It's not too much repeated code.
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.
Makes sense, I will put them back. I did it this way as you also need to provide boundary conditions for the constant concentration model so that grad(c_e)
ends up having the correct shape.
@@ -168,6 +168,10 @@ def positive_radius(x): | |||
|
|||
param["Negative particle radius [m]"] = negative_radius | |||
param["Positive particle radius [m]"] = positive_radius | |||
# Only get 3dp of accuracy in some tests at 1C with particle distribution | |||
# TODO: investigate if there is a bug or some way to improve the | |||
# implementation |
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.
is this related to what @brosaplanella was seeing?
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 it. you can get 4dp of accuracy everywhere else.
Description
Adds a new lithium-ion model based on the formulation in JS Newman and CW Tobias. "Theoretical Analysis of Current Distribution in Porous Electrodes". Journal of The Electrochemical Society,1962.
The model assumes uniform (constant) concentration in the electrolyte and includes a single ODE in each electrode for the average particle concentration. Options can be passed to allow different particle models, but there is only ever a single particle in each electrode (i.e. we solve for the x-averaged concentration).
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: