-
-
Notifications
You must be signed in to change notification settings - Fork 563
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
fix experiment bugs #1707
fix experiment bugs #1707
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1707 +/- ##
========================================
Coverage 99.24% 99.24%
========================================
Files 344 345 +1
Lines 19037 19049 +12
========================================
+ Hits 18894 18906 +12
Misses 143 143
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.
nice, thanks @tinosulzer !
|
||
# load models | ||
models = [ | ||
pybamm.lead_acid.LOQS(), | ||
pybamm.lead_acid.FOQS(), | ||
pybamm.lead_acid.Composite(), | ||
pybamm.lead_acid.Full(), | ||
pybamm.lead_acid.Full({"surface form": "differential"}), |
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.
revert to the default options?
@@ -713,17 +713,6 @@ def build_model(self): | |||
self.set_degradation_variables() | |||
self.set_summary_variables() | |||
|
|||
# Massive hack for consistent delta_phi = phi_s - phi_e with SPMe |
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.
bye bye!
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.
Where was the issue?
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 just made a new submodel for this. So now we have inverse butler-volmer for delta_phi_av and then another submodel for delta_phi. It's still a hack but now a "feature" instead of a "bug"
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 am also thinking of combining the interface and conductivity submodels into one. It's the most confusing part of the submodels at the moment, because the way the interface and conductivity submodels interact is different based on whether you do forward or inverse kinetics. Combining them would mean more duplicate code but I think clearer
Description
Fix some bugs related to how an experiment is set up
Fixes #1700
Fixes #1702
Fixes #479
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: