-
-
Notifications
You must be signed in to change notification settings - Fork 572
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
Issue 834 sei models #974
Issue 834 sei models #974
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #974 +/- ##
===========================================
+ Coverage 97.48% 97.65% +0.17%
===========================================
Files 222 243 +21
Lines 11791 12593 +802
===========================================
+ Hits 11494 12298 +804
+ Misses 297 295 -2
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.
Just a few style comments, but otherwise looks fine to me. Thanks guys!
pb.set_logging_level("INFO") | ||
# pb.settings.debug_mode = True | ||
|
||
options = {"sei": "reaction limited"} |
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.
Should we comment out all but one so users will know they only need one of them?
examples/scripts/sei_growth.py
Outdated
# } | ||
# ) | ||
|
||
# parameter_values.update({"Current function [A]": 0}) |
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.
If the parameter values are already in the default, I think we should delete them from here to keep it simple.
@@ -49,6 +49,11 @@ class BaseBatteryModel(pybamm.BaseModel): | |||
variable for instead of solving in PyBaMM. The entries of the lists | |||
are strings that correspond to the submodel names in the keys | |||
of `self.submodels`. | |||
* "sei" : str |
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.
We should give a reference for each model so people can check what the equations are.
@@ -36,6 +36,58 @@ def set_standard_output_variables(self): | |||
} | |||
) | |||
|
|||
# def set_reactions(self): |
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 needed anymore?
Description
Add SEI models
Just need to figure out the example (or remove?)
Fixes #834
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: