Issue 791 complete models #795
Conversation
Codecov Report
@@ Coverage Diff @@
## master #795 +/- ##
==========================================
+ Coverage 98.45% 98.53% +0.08%
==========================================
Files 179 181 +2
Lines 10009 10260 +251
==========================================
+ Hits 9854 10110 +256
+ Misses 155 150 -5
Continue to review full report at Codecov.
|
...tegration/test_models/test_full_battery_models/test_lithium_ion/test_compare_basic_models.py
Show resolved
Hide resolved
examples/notebooks/README.md
Outdated
| The following models are implemented and can easily be used or [compared](./models/lead-acid.ipynb). We always welcome [new models](https://pybamm.readthedocs.io/en/latest/tutorials/add-model.html)! | ||
| Several battery models are implemented and can easily be used or [compared](./models/lead-acid.ipynb). The notebooks below show the solution of each individual model. We always welcome [new models](https://pybamm.readthedocs.io/en/latest/tutorials/add-model.html)! | ||
|
|
||
| Once you are comfortable with the expression tree structure, a good starting point to understand the models in PyBaMM is to take a look at the [basic SPM](https://github.com/pybamm-team/PyBaMM/blob/master/pybamm/models/full_battery_models/lithium_ion/basic_spm.py) and [basic DFN](https://github.com/pybamm-team/PyBaMM/blob/master/pybamm/models/full_battery_models/lithium_ion/basic_dfn.py), since these show the entire model in one place and so are easier to understand. However, we recommend that you subsequently use the full models as they offer much greater flexibility for coupling different physical effects and visualising a greater range of variables. |
There was a problem hiding this comment.
At first I didn't exactly understand what showing the entire model in one place means.
After looking at the implementation of models, I think that means that the model parameters, variables, equations.. are set within the class definition explicitly, instead of happening hidden in the various sub models. Is that correct?
There was a problem hiding this comment.
that's correct, I will update to say that
| @@ -23,7 +23,7 @@ def __init__(self, param): | |||
|
|
|||
| def get_fundamental_variables(self): | |||
|
|
|||
| T_x_av = pybamm.PrimaryBroadcast(0, "current collector") | |||
| T_x_av = pybamm.PrimaryBroadcast(self.param.T_init, "current collector") | |||
There was a problem hiding this comment.
Is this likely to affect other models other than the new basic ones?
There was a problem hiding this comment.
yeah, this was a bug fix
There was a problem hiding this comment.
Examples like this that help understanding how models are actually implemented are welcome! Definitely really useful.
Am I right in understanding that the 2 models BasicSPM and BasicDFN are purely pedagogical?
If yes I think this could maybe be made more explicit in the README and the classes' docstrings.
Also, looking at the tests, I fail to understand why the basic models are expected to yield the same results as the default ones. I've been trying to understand the difference(s) between the basic SPM and the default SPM, and if I'm correct the .rhs dict is different between the two. How can then both models give the same results ?
The docstring of the BasicSPM class mentions that using this class comes at the cost of flexibility, but couldn't I create a BasicSPM model and modify its parameters afterwards?
|
Thanks for the review Thibault, yes the 2 basic models are purely pedagogical: people can see all the bits that go into those models in one place, and make their own copy to easily edit if they want to make changes to the model. The basic SPM should be (almost) exactly the same as the "full" SPM with default options, with the only difference being that the full SPM variables have the option of being discretised to become 3D variables, but the basic SPM ones don't. So after discretisation they should be exactly the same. The flexibility here means being able to combine different submodels (e.g. different thermal models, mechanics, degradation) in any arbitrary combination, which isn't possible with the basic SPM. We could do this with lots of if statements too but decided it would get too messy |
Description
Create a basic SPM and basic DFN in order to help introduce the battery models in a more user-friendly way.
Fixes #791
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 docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: